-
Notifications
You must be signed in to change notification settings - Fork 66
Add ability to configure custom init containers #1532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
I tested with the steps provided in the PR description and can confirm it works as expected ✔️ |
2e3e20f to
1ff60c7
Compare
|
/retest |
| if !home.PersistUserHomeEnabled(workspace) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't take this clause into account
cc @dkwon17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, it appears to align with the documentation:
Note: If
persistUserHome.enabledisfalse, anyinit-persistent-homecontainer is ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep it simple, I agree with @tolusha ,
Note: If persistUserHome.enabled is false, any init-persistent-home container is ignored.
Unless I'm mistaken, the above documentation is introduced in this PR, so removing the clause is okay on my end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkwon17 you're right about the documentation. It comes from this:
devworkspace-operator/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
Lines 230 to 243 in 1110865
| type PersistentHomeConfig struct { | |
| // Determines whether the `/home/user/` directory in workspaces should persist between | |
| // workspace shutdown and startup. | |
| // Must be used with the 'per-user'/'common' or 'per-workspace' storage class in order to take effect. | |
| // Disabled by default. | |
| Enabled *bool `json:"enabled,omitempty"` | |
| // Determines whether the init container that initializes the persistent home directory should be disabled. | |
| // When the `/home/user` directory is persisted, the init container is used to initialize the directory before | |
| // the workspace starts. If set to true, the init container will not be created. | |
| // This field is not used if the `workspace.persistUserHome.enabled` field is set to false. | |
| // Enabled by default. | |
| DisableInitContainer *bool `json:"disableInitContainer,omitempty"` | |
| } | |
I understand that PersistentHomeConfig.Enabled and PersistentHomeConfig.DisableInitContainer serve different purposes and work together.
Without this check, custom init-persistent-home container will be processed even when Enabled: false, which is inconsistent.
|
@akurinnoy thank you for the PR, I want to suggest a different approach for this feature: After all of the initconatiners have been accumulated:
We can do a strategic merge around here for the init containers, where the patch is the content of We already do something similar here with container contributions, the code uses strategic merge from the devfile api to merge containers WDYT cc @rohanKanojia @tolusha ? |
Signed-off-by: Oleksii Kurinnyi <[email protected]>
…erate_all Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Co-authored-by: Rohan Kumar <[email protected]> Signed-off-by: Oleksii Kurinnyi <[email protected]>
Co-authored-by: Rohan Kumar <[email protected]> Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Co-authored-by: Anatolii Bazko <[email protected]> Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
b0edb84 to
bd28716
Compare
Signed-off-by: Oleksii Kurinnyi <[email protected]>
|
@akurinnoy : Hello, I noticed E2E failure seems related to changes in this PR: |
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
|
/retest |
Signed-off-by: Oleksii Kurinnyi <[email protected]>
3c883f0 to
1110865
Compare
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Co-authored-by: David Kwon <[email protected]> Signed-off-by: Oleksii Kurinnyi <[email protected]>
93244c2 to
665662c
Compare
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, tolusha The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Good job! |
What does this PR do?
This PR adds the ability for cluster administrators to configure custom init containers that run in all workspace pods via the DWOC.
So, now administrators can:
config.workspace.initContainers.init-persistent-homelogic by providing a custom container with the same name.What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-9373
https://issues.redhat.com/browse/CRW-9367
Is it tested? How?
Deploy the DevWorkspace Operator with these changes
Test 1: Custom init-persistent-home script execution
Apply a DWOC with a custom init container:
Create a test workspace and wait for it to start
Wait for workspace to run:
Verify the init container was injected:
POD_NAME=$(kubectl get pods -n $WORKSPACE_NAMESPACE -l controller.devfile.io/devworkspace_name=test1-custom-home -o jsonpath='{.items[0].metadata.name}')Verify marker file
Expected output
Verify config file
Expected output
Cleanup
kubectl delete devworkspace test1-custom-home -n $WORKSPACE_NAMESPACETest 2: Default init-persistent-home behavior
Reset DWOC to default (no custom init)
Create workspace with persistent home enabled
Wait for workspace to run:
Verify default
stowlogic still workskubectl logs -n $WORKSPACE_NAMESPACE -l controller.devfile.io/devworkspace_name=test2-backward-compat -c init-persistent-homeExpected output
Cleanup
kubectl delete devworkspace test2-backward-compat -n $WORKSPACE_NAMESPACETest 3: Validation of init-persistent-home configuration
Apply
DWOCwith invalid commandCreate workspace
Wait a few seconds for reconciliation
Verify workspace fails with validation error
Expected output
Expected output
Cleanup
kubectl delete devworkspace test3-invalid-command -n $WORKSPACE_NAMESPACETest 4: Multiple Custom Init Containers
Apply DWOC with multiple init containers
Create workspace
Wait for workspace to run:
Verify all init containers were injected and ran in order
POD_NAME=$(kubectl get pods -n $WORKSPACE_NAMESPACE -l controller.devfile.io/devworkspace_name=test4-multiple-init -o jsonpath='{.items[0].metadata.name}')Verify all 3 init containers are present
Expected output
Verify the sequence log shows all 3 steps
Expected output
Verify marker files from each init container
Expected output
Expected output
Cleanup
kubectl delete devworkspace test4-multiple-init -n $WORKSPACE_NAMESPACEPR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che