Fix setup of configmap/secret/projected/downwardapi #64855
Conversation
k8s-ci-robot
added
sig/storage
k8s-ci-robot
added
release-note-none
|
btw - I chose not to manually call teardown on other errors because that code path is harder to test and very less likely to occur. I think, this should be good enough for majority of cases. |
| if err := wrapped.SetUpAt(dir, fsGroup); err != nil { | ||
| return err | ||
| } | ||
| if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil { |
There was a problem hiding this comment.
My concern is that any of the steps after this can fail and we won't clean up the result of this method. Maybe we can add a clean up routine to execute if any of the subsequent steps fail?
There was a problem hiding this comment.
Yeah - so as I commented above, I omitted that case on purpose and there were 2 reasons of that:
- It is much harder to test other failures and hence much smaller chance of them happening in real world.
- cleaning up after both wrapped emptydir and nested mount points has been created is messier and since configmap/secrets are such core k8s constructs, I want to approach that cautiously. Problem is - first we have to remove the mount points we created and then unmount emptydir and I am worried we do not have enough test coverage around that area. :(
There was a problem hiding this comment.
Ok, I guess this will take care of the worst cases. So it is better then nothing.
gnufied
force-pushed
the
fix-configmap-cleanup-on-fail
branch
from
f4dc5a5 to
ea6539c
Compare
k8s-ci-robot
added
the
lgtm
saad-ali
added
kind/bug
|
@gnufied the verify job errors are legit. the node-e2e are probably flakes. |
|
@dims yeah pushing a fix shortly. |
|
/retest Review the full test history for this PR. Silence the bot with an |
3 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/lgtm cancel (stop the bot from trying again and again) |
k8s-ci-robot
removed
the
lgtm
|
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process Pull Request Labels
|
Only call setup after they are found; otherwise we are left with orphan directories that are never cleaned up.
gnufied
force-pushed
the
fix-configmap-cleanup-on-fail
branch
from
ea6539c to
f44d1b9
Compare
|
@dims fixed the problems with verify. can you lgtm please? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, gnufied, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 63905, 64855). If you want to cherry-pick this change to another branch, please follow the instructions here. |
k8s-ci-robot
added
release-note
…5-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64855: Fix setup of configmap/secret/projected/downwardapi Cherry pick of #64855 on release-1.10. #64855: Fix setup of configmap/secret/projected/downwardapi ```release-note Fix setup of configmap/secret/projected/downwardapi volumes ```
Only call setup after they are found; otherwise
we are left with orphan directories that are never
cleaned up.
Fixes #64788 and #64779
cc @aveshagarwal @saad-ali
/sig storage