Detect changes in automount resources and queue reconciles for started workspaces - #1017

Merged
amisevsk merged 4 commits into
devfile:mainfrom
amisevsk:watch-automount-resources
Jan 17, 2023
Merged

Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom
amisevsk:watch-automount-resources

Conversation

@amisevsk

@amisevskamisevsk commented Jan 13, 2023

Copy link
Copy Markdown
Collaborator

What does this PR do?

Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.

There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.

What issues does this PR fix or reference?

Closes#914

Is it tested? How?

To test manually:

  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)

To verify the specific case in the issue:

  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.

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

@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk

amisevsk commented Jan 13, 2023

Copy link
Copy Markdown
CollaboratorAuthor

/retest

@codecov

codecovBot commented Jan 13, 2023

Copy link
Copy Markdown

Codecov Report

Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (8e87736) compared to base (fc8d007).
Patch coverage: 61.29% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dkwon17

dkwon17 commented Jan 16, 2023

Copy link
Copy Markdown
Collaborator

I was able to go through the testing steps successfully, but I have a couple of questions:

  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.

The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?

Also, I was able to test this PR by mounting a PVC:

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi

After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?

@AObuchow

AObuchow commented Jan 16, 2023

Copy link
Copy Markdown
Collaborator

Things seem to work well in my testing.

The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:

{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}

I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.

One small thing to note (that I think is outside the scope of this issue):
My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.

I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.

AObuchow
AObuchow approved these changes Jan 16, 2023
View reviewed changes

@AObuchowAObuchow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏

@amisevsk

amisevsk commented Jan 16, 2023

Copy link
Copy Markdown
CollaboratorAuthor

The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?

This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.

For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.

@openshift-ci

openshift-ciBot commented Jan 17, 2023

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@amisevsk
amisevsk merged commit dd105f3 into devfile:mainJan 17, 2023
@amisevsk
amisevsk deleted the watch-automount-resources branch January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow

@dkwon17dkwon17

Labels

approvedlgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces

3 participants

@amisevsk@dkwon17@AObuchow
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Detect changes in automount resources and queue reconciles for started workspaces - #1017
Merged
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Jan 17, 2023
Merged
Detect changes in automount resources and queue reconciles for started workspaces#1017
amisevsk merged 4 commits into
devfile:mainfrom 
amisevsk:watch-automount-resources
Conversation
@amisevsk
@amisevskamisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

Collaborator
What does this PR do?
Watches namespaces for changes to automounted resources (configmaps, secrets, PVCs -- including git credentials secrets) and queues reconciles for any started workspaces in the same namespace.
There is a new controller test to verify this functionality. To avoid erroneous passing tests, I had to reduce the timeout, which could potentially cause flakiness in the future.
What issues does this PR fix or reference?
Closes#914
Is it tested? How?
To test manually:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create an automount resource and check that it is picked up and mounted to the workspace (causing it to restart)
To verify the specific case in the issue:
  1. Create a workspace and wait for it to be running. Verify that the controller is no longer reconciling that workspace (i.e. there are no new events being triggered)
  2. Create a git credentials secret (does not need to be valid). Verify that workspace is immediately restarted to include new volume
  3. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
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
@amisevsk
Reorganize controllers code to separate eventHandlers into file
4c28d47
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Watch automount resources and queue reconciles for running workspaces
f4e0914
Watch for events related to automount resources (configmaps, secrets,
pvcs) and queue reconciles for all running workspaces when detected.
This ensures that changes to automount resources (e.g. updating a
git-credential secret) are picked up and included in workspaces without
requiring a manually triggered reconcile.
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add controller test to verify changes to automount secrets reconcile
03ea5c5
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Move automounted fixed object names into constants package
8e87736
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
amisevsk
commented
Jan 13, 2023

Copy link


Copy Markdown

CollaboratorAuthor
/retest
@codecov
codecovBot
commented
Jan 13, 2023

Copy link


Copy Markdown

Codecov Report
Base: 49.98% // Head: 50.23% // Increases project coverage by +0.24% 🎉
Coverage data is based on head (8e87736) compared to base (fc8d007).

Patch coverage: 61.29% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1017 +/- ##
==========================================
+ Coverage 49.98% 50.23% +0.24% 
==========================================
Files 69 70 +1 Lines 5968 6006 +38 ==========================================
+ Hits 2983 3017 +34 - Misses 2759 2762 +3 - Partials 226 227 +1 
Impacted FilesCoverage Δ
pkg/provision/automount/gitconfig.go39.25% <0.00%> (ø)
controllers/workspace/eventhandlers.go50.00% <50.00%> (ø)
controllers/workspace/predicates.go61.81% <94.11%> (+14.44%)⬆️
controllers/workspace/devworkspace_controller.go63.00% <100.00%> (+2.36%)⬆️
pkg/provision/automount/templates.go91.78% <100.00%> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@dkwon17
dkwon17
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
I was able to go through the testing steps successfully, but I have a couple of questions:
  1. Create a second git-credentials secret (or modify the original) and verify that change is propagated to the devworkspace-merged-git-credentials secret immediately.
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
Also, I was able to test this PR by mounting a PVC:
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: my-pvc
labels:
controller.devfile.io/mount-to-devworkspace: 'true'
annotations:
controller.devfile.io/mount-path: /home/user/my-pvc
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi 
After starting a workspace, I updated the PVC by changing the mount path, which automatically restarted my running workspaces. Just wanted to highlight this because changing a PVC seems to immediately restart workspaces, but changes in configmap/secret's data does not, is this expected?
@AObuchow
AObuchow
commented
Jan 16, 2023

Copy link


Copy Markdown

Collaborator
Things seem to work well in my testing.
The git credentials case worked as I expected: I created a git credentials secret, then modified it to change the credentials, and the mounted credential file was updated without restarting the workspace (which I imagine is intended?). I also saw the following in the controller logs:
{
"level":"info",
"ts":1673904579.3065548,
"logger":"controllers.DevWorkspace",
"msg":"syncing merged git credentials secret: v1.Secret devworkspace-merged-git-credentials is not ready: Updated object",
"Request.Namespace":"devworkspace-controller",
"Request.Name":"plain-devworkspace",
"devworkspace_id":"workspace378ff1947b854bef"
}
I also tried creating a configmap with the controller.devfile.io/mount-to-devworkspace label. When it was created, the workspace was restarted, and I could see that the configmap was mounted within the workspace's filesystem.
One small thing to note (that I think is outside the scope of this issue):

My configmap set the per-workspace PVC size to a non-default value, and though the workspace restarted, the PVC size was not modified.
I think this is more related to the way the per-workspace pvc size configmaps are handled. I guess in the per-workspace storage provisioner, there's no check to see if the existing PVC for the workspace is the correct size. Deleting the PVC could result in data loss, so the current behaviour is probably for the better until #875 is resolved.
AObuchow
AObuchow
approved these changes
Jan 16, 2023
View reviewed changes
@AObuchowAObuchow
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me & great work on adding more controller tests 😎 🙏
@amisevsk
amisevsk
commented
Jan 16, 2023

Copy link


Copy Markdown

CollaboratorAuthor
The devworkspace-merged-git-credentials secret was updated immediately, but the workspace did not restart, is this expected?
This is expected -- the changes to the merged secret result in no changes to the workspace pod's spec, and so the pod is not restarted. Other changes (e.g. adding an automount PVC) change the pod spec (to mount the new object) and so the pod must be restarted to pick up the changes.
For the merged-git-credentials secret in specific, restarting the pod is not necessary. As the secret is mounted as files in the workspace, any changes to the secret's data will eventually be propagated down into the mounted file within the pod. This allows for rotating PATs automatically without requiring workspace restarts.
@openshift-ci
openshift-ciBot
commented
Jan 17, 2023

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, dkwon17
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@amisevsk
amisevsk
merged commit dd105f3
into
devfile:mainJan 17, 2023
@amisevsk
amisevsk
deleted the

watch-automount-resources

branch
January 17, 2023 15:57
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@AObuchowAObuchowAObuchow approved these changes
@dkwon17dkwon17dkwon17 approved these changes
@ibuziukibuziukAwaiting requested review from ibuziukibuziuk is a code owner

Assignees

@AObuchowAObuchow
@dkwon17dkwon17

Labels

approvedlgtm

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

DevWorkspace Operator should detect changes to automount volumes/secrets/configmaps and update running workspaces


3 participants

@amisevsk@dkwon17@AObuchow