Disable automount resources for the `init-container-home` init container #1273

dkwon17 · 2024-06-11T21:26:29Z

What does this PR do?

Disable automount resources for the init-persistent-home initcontainer (an not the other initcontainers) if persistent home is enabled.

What issues does this PR fix or reference?

#1257

Is it tested? How?

Deploy DWO:

export DWO_IMG=quay.io/dkwon17/devworkspace-controller:init-container-mount
make install

In the DWOC named devworkspace-operator-config, enable persistent storage.

apiVersion: controller.devfile.io/v1alpha1
kind: DevWorkspaceOperatorConfig
metadata:
  name: devworkspace-operator-config
  namespace: eclipse-che
config:
  workspace:
    persistUserHome:
      enabled: true

Create a configmap that mounts to any directory:

kind: ConfigMap
apiVersion: v1
metadata:
  name: storageconf
  labels:
    controller.devfile.io/mount-to-devworkspace: 'true'
    controller.devfile.io/watch-configmap: 'true'
  annotations:
    controller.devfile.io/mount-as: subpath
    controller.devfile.io/mount-path: /home/user/.config/containers
data:
  storage.conf: test1234

Create a workspace and wait for it to start:

oc apply -f ./samples/code-latest.yaml

Verify that the first initcontainer is init-persistent-home, and that the configmap is mounted in all containers except for init-persistent-home:

$ oc get pod <pod name> -o jsonpath='{.spec.initContainers[0].name}'
init-persistent-home

$ oc get pod <pod name> -o jsonpath='{.spec.initContainers[0].volumeMounts}' | grep storageconf
// no match exists

$ oc get pod <pod name> -o jsonpath='{.spec.initContainers[1].volumeMounts}' | grep storageconf
// match exists

$ oc get pod <pod name> -o jsonpath='{.spec.initContainers[2].volumeMounts}' | grep storageconf
// match exists

$ oc get pod <pod name> -o jsonpath='{.spec.containers[*].volumeMounts}' | grep storageconf
// match exists

Verify that stow has run successfully in the dev container:

sh-4.4$ cat ~/.stow.log | tail -n 10 
LINK: go/pkg/mod/mvdan.cc/xurls/[email protected]/xurls_test.go => ../../../../../../../tooling/go/pkg/mod/mvdan.cc/xurls/[email protected]/xurls_test.go
    level of go/pkg/sumdb is 2
MKDIR: go/pkg/sumdb
    level of go/pkg/sumdb/sum.golang.org is 3
MKDIR: go/pkg/sumdb/sum.golang.org
    level of go/pkg/sumdb/sum.golang.org/latest is 4
LINK: go/pkg/sumdb/sum.golang.org/latest => ../../../../../tooling/go/pkg/sumdb/sum.golang.org/latest
Planning stow of package .... done
Processing tasks...
Processing tasks... done

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

openshift-ci · 2024-06-11T21:26:39Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dkwon17 · 2024-06-12T19:25:12Z

Hello @AObuchow could you please take a look? I just want to make sure the code is OK before I add new tests

dkwon17 · 2024-06-12T19:28:58Z

pkg/library/lifecycle/prestart.go

+		}
+	}
+	return initContainers
+}


Setting the init-persistent-home initcontainer to the front is necessary because if there is another initcontainer that runs before it, the automount resource can mount within /home/user and cause stow conflicts in init-persistent-home since the mounted configmap file would be saved in the PVC.

Basically, this function is here to have a way to ensure that init-persistent-home is the first initcontainer that runs, running before initcontainers like che-code-injector and project-clone

Thank you for the clarification, makes sense to me. It's probably worth leaving a similar comment explaining this in the code.

codecov · 2024-06-12T21:01:56Z

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.84%. Comparing base (adc8b5a) to head (1a5164c).
Report is 26 commits behind head on main.

❗ Current head 1a5164c differs from pull request most recent head f83b0ba

Please upload reports for the commit f83b0ba to get more accurate results.

Files	Patch %	Lines
pkg/library/lifecycle/prestart.go	50.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
+ Coverage   52.52%   52.84%   +0.32%     
==========================================
  Files          84       84              
  Lines        7642     7864     +222     
==========================================
+ Hits         4014     4156     +142     
- Misses       3335     3408      +73     
- Partials      293      300       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AObuchow

@dkwon17 looks great to me so far :) I think you're good to go ahead and add tests 🙏

pkg/provision/automount/common.go

pkg/library/lifecycle/prestart_test.go

AObuchow · 2024-06-19T18:09:08Z

@dkwon17 In the testing instructions, you said to grep for my-settings instead of storageconf: oc get pod <pod name> -o jsonpath='{.spec.containers[*].volumeMounts}' | grep my-settings. Was that intentional?

dkwon17 · 2024-06-19T18:46:10Z

@AObuchow sorry that was a mistake, it should be storageconf

AObuchow

Tested on OpenShift 4.15 & works as expected.

Looks good to me in its current state. Left some minor comments for consideration, but they are just suggestions. Great work @dkwon17 🥳!

pkg/library/lifecycle/prestart.go

AObuchow · 2024-06-17T21:22:33Z

pkg/provision/automount/common_persistenthome_test.go

+					cmp.Diff(tt.Output.EnvFrom, container.EnvFrom, testDiffOpts))
+			}
+
+			for _, container := range podAdditions.InitContainers {


Somewhat redundant since it's already implicitly being checked in the preStart event test, but we could also explicitly check that the first container in the podAdditions.initContainers[] is the init-persistent-home initContainer (since this is a change introduced in this PR).

I wasn't able to do that check here because the function that the this test is for:

devworkspace-operator/pkg/provision/automount/common_persistenthome_test.go

Line 59 in 9874cd4

err := ProvisionAutoMountResourcesInto(podAdditions, testAPI, testNamespace, true)

doesn't reorder the initcontainers

My mistake, makes sense 👍

pkg/library/lifecycle/prestart.go

AObuchow · 2024-06-19T19:07:49Z

@dkwon17 also before merging please squash your fixup commits & mention in the "Add test" commit that the tests are related to automount functionality with home persistence.

…tainer if persistent home is enabled Fix devfile#1257 Disabling automount resources for the init-persistent-home initconatiner is done to avoid possible stow conflicts which can be introduced by automount resources. Signed-off-by: David Kwon <[email protected]>

Signed-off-by: David Kwon <[email protected]>

AObuchow

LGTM 👍 Great job @dkwon17 :)

openshift-ci · 2024-06-24T15:01:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow, dkwon17, ibuziuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AObuchow,dkwon17]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added do-not-merge/work-in-progress approved labels Jun 11, 2024

dkwon17 force-pushed the init-container-mount branch from cd920bc to 1a5164c Compare June 12, 2024 19:21

dkwon17 marked this pull request as ready for review June 12, 2024 19:24

dkwon17 requested review from AObuchow and ibuziuk as code owners June 12, 2024 19:24

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 12, 2024

dkwon17 commented Jun 12, 2024

View reviewed changes

AObuchow reviewed Jun 13, 2024

View reviewed changes

pkg/provision/automount/common.go Outdated Show resolved Hide resolved

AObuchow reviewed Jun 14, 2024

View reviewed changes

pkg/library/lifecycle/prestart_test.go Outdated Show resolved Hide resolved

AObuchow approved these changes Jun 19, 2024

View reviewed changes

openshift-ci bot assigned AObuchow Jun 19, 2024

openshift-ci bot added the lgtm label Jun 19, 2024

ibuziuk approved these changes Jun 21, 2024

View reviewed changes

openshift-ci bot assigned ibuziuk Jun 21, 2024

dkwon17 force-pushed the init-container-mount branch from d804278 to 9874cd4 Compare June 21, 2024 19:32

openshift-ci bot removed the lgtm label Jun 21, 2024

dkwon17 added 2 commits June 21, 2024 15:41

Add test cases for init-persistent-home container automounts

f83b0ba

Signed-off-by: David Kwon <[email protected]>

dkwon17 force-pushed the init-container-mount branch from 9874cd4 to f83b0ba Compare June 21, 2024 19:41

AObuchow approved these changes Jun 24, 2024

View reviewed changes

openshift-ci bot added the lgtm label Jun 24, 2024

dkwon17 merged commit fb3a8fe into devfile:main Jun 24, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable automount resources for the `init-container-home` init container #1273

Disable automount resources for the `init-container-home` init container #1273

dkwon17 commented Jun 11, 2024 •

edited

Loading

openshift-ci bot commented Jun 11, 2024

dkwon17 commented Jun 12, 2024

dkwon17 Jun 12, 2024 •

edited

Loading

AObuchow Jun 13, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

AObuchow left a comment

AObuchow commented Jun 19, 2024

dkwon17 commented Jun 19, 2024

AObuchow left a comment

AObuchow Jun 17, 2024

dkwon17 Jun 21, 2024

AObuchow Jun 24, 2024

AObuchow commented Jun 19, 2024

AObuchow left a comment

openshift-ci bot commented Jun 24, 2024

Disable automount resources for the init-container-home init container #1273

Disable automount resources for the init-container-home init container #1273

Conversation

dkwon17 commented Jun 11, 2024 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Jun 11, 2024

dkwon17 commented Jun 12, 2024

dkwon17 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

AObuchow Jun 13, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow commented Jun 19, 2024

dkwon17 commented Jun 19, 2024

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow Jun 17, 2024

Choose a reason for hiding this comment

dkwon17 Jun 21, 2024

Choose a reason for hiding this comment

AObuchow Jun 24, 2024

Choose a reason for hiding this comment

AObuchow commented Jun 19, 2024

AObuchow left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2024

Disable automount resources for the `init-container-home` init container #1273

Disable automount resources for the `init-container-home` init container #1273

dkwon17 commented Jun 11, 2024 •

edited

Loading

dkwon17 Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading