diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 92df4a44b..a27d79098 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -353,7 +353,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Add automount resources into devfile containers - err = automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspace.Namespace) + err = automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspace.Namespace, home.PersistUserHomeEnabled(workspace)) if shouldReturn, reconcileResult, reconcileErr := r.checkDWError(workspace, err, "Failed to process automount resources", metrics.ReasonBadRequest, reqLogger, &reconcileStatus); shouldReturn { return reconcileResult, reconcileErr } diff --git a/pkg/library/home/persistentHome.go b/pkg/library/home/persistentHome.go index e1b3b8c33..9f5eff1b1 100644 --- a/pkg/library/home/persistentHome.go +++ b/pkg/library/home/persistentHome.go @@ -92,10 +92,7 @@ func AddPersistentHomeVolume(workspace *common.DevWorkspaceWithConfig) (*v1alpha // - Persistent storage is required for the DevWorkspace // Returns false otherwise. func NeedsPersistentHomeDirectory(workspace *common.DevWorkspaceWithConfig) bool { - if !pointer.BoolDeref(workspace.Config.Workspace.PersistUserHome.Enabled, false) { - return false - } - if !storageStrategySupportsPersistentHome(workspace) { + if !PersistUserHomeEnabled(workspace) || !storageStrategySupportsPersistentHome(workspace) { return false } for _, component := range workspace.Spec.Template.Components { @@ -113,6 +110,10 @@ func NeedsPersistentHomeDirectory(workspace *common.DevWorkspaceWithConfig) bool return storage.WorkspaceNeedsStorage(&workspace.Spec.Template) } +func PersistUserHomeEnabled(workspace *common.DevWorkspaceWithConfig) bool { + return pointer.BoolDeref(workspace.Config.Workspace.PersistUserHome.Enabled, false) +} + // Returns true if the workspace's storage strategy supports persisting the user home directory. // The storage strategies which support home persistence are: per-user/common, per-workspace & async. // The ephemeral storage strategy does not support home persistence. diff --git a/pkg/library/lifecycle/prestart.go b/pkg/library/lifecycle/prestart.go index 0b02f4461..a828ab677 100644 --- a/pkg/library/lifecycle/prestart.go +++ b/pkg/library/lifecycle/prestart.go @@ -19,6 +19,7 @@ import ( "fmt" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/constants" ) // GetInitContainers partitions the components in a devfile's flattened spec into initContainer and non-initContainer lists @@ -71,9 +72,25 @@ func GetInitContainers(devfile dw.DevWorkspaceTemplateSpecContent) (initContaine } } + // This is necessary because if there is another initcontainer that runs before it, + // an automount resource can mount within /home/user and cause stow conflicts + // in init-persistent-home since the automount resource would be saved in the PVC. + initContainers = setHomeInitContainerToFront(initContainers) return initContainers, mainComponents, nil } +// Takes a slice of components which are intended to run as initContainers and moves the +// init-persistent-home component to the start of the slice. +func setHomeInitContainerToFront(initContainers []dw.Component) []dw.Component { + for i, container := range initContainers { + if container.Name == constants.HomeInitComponentName { + initContainers = append(initContainers[:i], initContainers[i+1:]...) + return append([]dw.Component{container}, initContainers...) + } + } + return initContainers +} + func checkPreStartEventCommandsValidity(initCommands []dw.Command) error { for _, cmd := range initCommands { commandType, err := getCommandType(cmd) diff --git a/pkg/library/lifecycle/prestart_test.go b/pkg/library/lifecycle/prestart_test.go index b36184cb0..82217412e 100644 --- a/pkg/library/lifecycle/prestart_test.go +++ b/pkg/library/lifecycle/prestart_test.go @@ -56,6 +56,9 @@ func TestGetInitContainers(t *testing.T) { loadPreStartTestCaseOrPanic(t, "prestart_exec_command.yaml"), loadPreStartTestCaseOrPanic(t, "prestart_apply_command.yaml"), loadPreStartTestCaseOrPanic(t, "init_and_main_container.yaml"), + loadPreStartTestCaseOrPanic(t, "persistent_home_initcontainer_first_initcontainer.yaml"), + loadPreStartTestCaseOrPanic(t, "persistent_home_initcontainer_second_initcontainer.yaml"), + loadPreStartTestCaseOrPanic(t, "persistent_home_initcontainer_only_initcontainer.yaml"), } for _, tt := range tests { diff --git a/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_first_initcontainer.yaml b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_first_initcontainer.yaml new file mode 100644 index 000000000..7339ae9a1 --- /dev/null +++ b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_first_initcontainer.yaml @@ -0,0 +1,45 @@ +name: "Should set the init-persistent-home init container when init-persistent-home is the first preStart event" + +input: + components: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + - name: init-persistent-home + container: + image: my-image + commands: + - id: test_preStart_command + apply: + component: test-container1 + - id: test_regular_command + exec: + component: test-container1 + command: "test_command" + - id: init-persistent-home + apply: + component: init-persistent-home + events: + preStart: + - "init-persistent-home" + - "test_preStart_command" + +output: + initContainers: + - name: init-persistent-home + container: + image: my-image + - name: test-container1 + container: + image: my-image + mainContainers: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + errRegexp: diff --git a/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_only_initcontainer.yaml b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_only_initcontainer.yaml new file mode 100644 index 000000000..4e0e13ba4 --- /dev/null +++ b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_only_initcontainer.yaml @@ -0,0 +1,38 @@ +name: "Should set the init-persistent-home init container when it is the only init container" + +input: + components: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + - name: init-persistent-home + container: + image: my-image + commands: + - id: test_regular_command + exec: + component: test-container1 + command: "test_command" + - id: init-persistent-home + apply: + component: init-persistent-home + events: + preStart: + - "init-persistent-home" + +output: + initContainers: + - name: init-persistent-home + container: + image: my-image + mainContainers: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + errRegexp: diff --git a/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_second_initcontainer.yaml b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_second_initcontainer.yaml new file mode 100644 index 000000000..c12144746 --- /dev/null +++ b/pkg/library/lifecycle/testdata/prestart/persistent_home_initcontainer_second_initcontainer.yaml @@ -0,0 +1,45 @@ +name: "Should set the init-persistent-home init container when init-persistent-home is the second preStart event" + +input: + components: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + - name: init-persistent-home + container: + image: my-image + commands: + - id: test_preStart_command + apply: + component: test-container1 + - id: test_regular_command + exec: + component: test-container1 + command: "test_command" + - id: init-persistent-home + apply: + component: init-persistent-home + events: + preStart: + - "test_preStart_command" + - "init-persistent-home" + +output: + initContainers: + - name: init-persistent-home + container: + image: my-image + - name: test-container1 + container: + image: my-image + mainContainers: + - name: test-container1 + container: + image: my-image + - name: test-container2 + container: + image: my-image + errRegexp: diff --git a/pkg/provision/automount/common.go b/pkg/provision/automount/common.go index 7f9ce2364..23ce1ce96 100644 --- a/pkg/provision/automount/common.go +++ b/pkg/provision/automount/common.go @@ -42,7 +42,7 @@ type Resources struct { EnvFromSource []corev1.EnvFromSource } -func ProvisionAutoMountResourcesInto(podAdditions *v1alpha1.PodAdditions, api sync.ClusterAPI, namespace string) error { +func ProvisionAutoMountResourcesInto(podAdditions *v1alpha1.PodAdditions, api sync.ClusterAPI, namespace string, persistentHome bool) error { resources, err := getAutomountResources(api, namespace) if err != nil { @@ -59,10 +59,16 @@ func ProvisionAutoMountResourcesInto(podAdditions *v1alpha1.PodAdditions, api sy } for idx, initContainer := range podAdditions.InitContainers { + + // Don't add automount resources if persistent home init container exists, because + // automount resources can conflict with the init container's stow command + if persistentHome && initContainer.Name == constants.HomeInitComponentName { + continue + } + podAdditions.InitContainers[idx].VolumeMounts = append(initContainer.VolumeMounts, resources.VolumeMounts...) podAdditions.InitContainers[idx].EnvFrom = append(initContainer.EnvFrom, resources.EnvFromSource...) } - if resources.Volumes != nil { podAdditions.Volumes = append(podAdditions.Volumes, resources.Volumes...) } diff --git a/pkg/provision/automount/common_persistenthome_test.go b/pkg/provision/automount/common_persistenthome_test.go new file mode 100644 index 000000000..159f05b95 --- /dev/null +++ b/pkg/provision/automount/common_persistenthome_test.go @@ -0,0 +1,93 @@ +// +// Copyright (c) 2019-2024 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package automount + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" +) + +func TestProvisionAutomountResourcesIntoPersistentHomeEnabled(t *testing.T) { + tests := []testCase{ + loadTestCaseOrPanic(t, "testdata/testProvisionsConfigmaps.yaml"), + loadTestCaseOrPanic(t, "testdata/testProvisionsProjectedVolumes.yaml"), + loadTestCaseOrPanic(t, "testdata/testProvisionsSecrets.yaml"), + } + + testPodAdditions := &v1alpha1.PodAdditions{ + Containers: []corev1.Container{{ + Name: "test-container", + Image: "test-image", + }}, + InitContainers: []corev1.Container{{ + Name: "init-persistent-home", + Image: "test-image", + }, { + Name: "test-container", + Image: "test-image", + }}, + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("%s (%s)", tt.Name, tt.TestPath), func(t *testing.T) { + podAdditions := testPodAdditions.DeepCopy() + testAPI := sync.ClusterAPI{ + Client: fake.NewClientBuilder().WithObjects(tt.Input.allObjects...).Build(), + } + + err := ProvisionAutoMountResourcesInto(podAdditions, testAPI, testNamespace, true) + + if !assert.NoError(t, err, "Unexpected error") { + return + } + assert.Truef(t, cmp.Equal(tt.Output.Volumes, podAdditions.Volumes, testDiffOpts), + "Volumes should match expected output:\n%s", + cmp.Diff(tt.Output.Volumes, podAdditions.Volumes, testDiffOpts)) + + for _, container := range podAdditions.Containers { + assert.Truef(t, cmp.Equal(tt.Output.VolumeMounts, container.VolumeMounts, testDiffOpts), + "Container VolumeMounts should match expected output:\n%s", + cmp.Diff(tt.Output.VolumeMounts, container.VolumeMounts, testDiffOpts)) + assert.Truef(t, cmp.Equal(tt.Output.EnvFrom, container.EnvFrom, testDiffOpts), + "Container EnvFrom should match expected output:\n%s", + cmp.Diff(tt.Output.EnvFrom, container.EnvFrom, testDiffOpts)) + } + + for _, container := range podAdditions.InitContainers { + if container.Name == "init-persistent-home" { + assert.Truef(t, container.VolumeMounts == nil || len(container.VolumeMounts) == 0, + "The init-persistent-home container should not have any volume mounts if persistent home is enabled") + } else { + assert.Truef(t, cmp.Equal(tt.Output.VolumeMounts, container.VolumeMounts, testDiffOpts), + "Container VolumeMounts should match expected output:\n%s", + cmp.Diff(tt.Output.VolumeMounts, container.VolumeMounts, testDiffOpts)) + assert.Truef(t, cmp.Equal(tt.Output.EnvFrom, container.EnvFrom, testDiffOpts), + "Container EnvFrom should match expected output:\n%s", + cmp.Diff(tt.Output.EnvFrom, container.EnvFrom, testDiffOpts)) + } + } + + }) + } +} diff --git a/pkg/provision/automount/common_test.go b/pkg/provision/automount/common_test.go index 9ca0fb0c8..b7518b4fd 100644 --- a/pkg/provision/automount/common_test.go +++ b/pkg/provision/automount/common_test.go @@ -125,7 +125,7 @@ func TestProvisionAutomountResourcesInto(t *testing.T) { } // Note: this test does not allow for returning AutoMountError with isFatal: false (i.e. no retrying) // and so is not suitable for testing automount features that provision cluster resources (yet) - err := ProvisionAutoMountResourcesInto(podAdditions, testAPI, testNamespace) + err := ProvisionAutoMountResourcesInto(podAdditions, testAPI, testNamespace, false) if tt.Output.ErrRegexp != nil { if !assert.Error(t, err, "Expected an error but got none") { return