-
Notifications
You must be signed in to change notification settings - Fork 867
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
fix: Add volume for plugin and tmp folder #3546
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3546 +/- ##
==========================================
- Coverage 81.83% 78.19% -3.65%
==========================================
Files 135 158 +23
Lines 20688 18397 -2291
==========================================
- Hits 16931 14386 -2545
- Misses 2883 3104 +221
- Partials 874 907 +33 ☔ View full report in Codecov by Sentry. |
Go Published Test Results2 160 tests 2 160 ✅ 2m 53s ⏱️ Results for commit 64a30cc. ♻️ This comment has been updated with latest results. |
E2E Tests Published Test Results 4 files 4 suites 3h 23m 55s ⏱️ For more details on these failures, see this check. Results for commit 64a30cc. ♻️ This comment has been updated with latest results. |
ephemeral-storage: 300Mi | ||
volumeMounts: | ||
- name: plugin-bin | ||
mountPath: /home/argo-rollouts/plugin-bin |
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.
By default, the plugin folder for Rollout is at the root afaik. But I am using Dockerfile.dev, so maybe it is different with that one.
Edit: Dockerfile.dev is indeed different and I updated it to the same workdir on my branch, but you could do it in this PR too.
Here is the patch I currently have to be able to run plugins.
/tmp
should be added, otherwise the volume will be read-only and it it used for socket creation.
apiVersion: apps/v1
kind: Deployment
metadata:
name: argo-rollouts
spec:
template:
spec:
containers:
- name: argo-rollouts
volumeMounts:
- mountPath: /home/argo-rollouts/plugin-bin
name: plugin-bin
readOnly: false
- mountPath: /tmp
name: tmp
readOnly: false
volumes:
- name: plugin-bin
emptyDir: {}
- name: tmp
emptyDir: {}
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.
Updated in da2dea5
resources: | ||
limits: | ||
ephemeral-storage: 1Gi | ||
volumeMounts: |
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.
tmp
volume added but not mounted.
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.
Fixed in 64a30cc
Signed-off-by: Tommy Chen <tommy351@gmail.com>
Quality Gate passedIssues Measures |
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.
LGTM
Signed-off-by: Tommy Chen <tommy351@gmail.com>
The latest manifests (v1.7.0-rc1) throws the following error during plugin download. The reason probably is the stricter security context introduced in #3424. I added a new volume for downloaded plugin files.
It seems files inFixed by addingemptyDir
is not executable even ifsecurityContext.fsGroup
is set. I'm still investigating.tmp
volume.Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.