-
Notifications
You must be signed in to change notification settings - Fork 1.6k
use anonymous volume for /var/lib/kubelet #773
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder 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 |
FYI @cjwagner this should obviate the need for the additional mount in the local prowjob tool. |
is 770 correct here? |
corrected to 771 |
// ensure pods etc. are not on container filesystem | ||
// TODO: we could do this in the image instead | ||
// However this would leave old images with this issue | ||
"-v", "/var/lib/kubelet", |
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.
hm, this would expose the kind nodes to the host /var/lib/kubelet that may already contain kubelet data, which may result in:
- the kubelet on the node re-using the existing kubelet certificates
- the kubelet on the node stomping kubelet data on the host
EDIT: 771
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.
hm, this would expose the kind nodes to the host /var/lib/kubelet that may already contain kubelet data, which may result in: ...
no, that's not what this does. this is an anonymous volume, not a bind mount.
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.
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.
my mistake.
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.
this flag format does a lot of in-band signaling 😅
soon moving this to higher level logic... going to need to re-work that a bit now 🙃
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
prow is broken at the moment. I've tested this locally and FWIW this is the same thing we do for |
fixes #771