-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Custom volume options #1843
Custom volume options #1843
Conversation
via unsupported environment variable KIND_EXPERIMENTAL_DOCKER_VOLUME_OPTIONS
Welcome @mausch! |
Hi @mausch. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mausch The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold Also as I said these volumes are an internal detail subject to change. |
This would be a problem if the client library didn't support these options, right? But that doesn't seem to be the case I don't think? ContainerCreate can be passed a HostConfig which has a Mounts property which has the necessary VolumeOptions
Well, this is explicitly an unsupported feature with a big warning... meaning you could just remove this feature altogether if there is some actual reason to do so 🙂 |
func runArgsForNode(node *config.Node, clusterIPFamily config.ClusterIPFamily, name string, args []string) ([]string, error) { | ||
func runArgsForNode(logger log.Logger, node *config.Node, clusterIPFamily config.ClusterIPFamily, name string, args []string) ([]string, error) { | ||
varVolume := "type=volume,target=/var" | ||
if n := os.Getenv("KIND_EXPERIMENTAL_DOCKER_VOLUME_OPTIONS"); n != "" { |
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.
if you look in the recent history this was actually N volumes covering specific subsets of /var, we've temporarily reverted to one volume.
how is this handled if we switch back to N volumes ...?
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.
Good question! I see that happened in 8201cd9 .
If you had to switch back to N volumes you could just remove this environment variable altogether. I think it being explicitly unsupported gives you the right to do that at any moment and nobody has the right to complain about it ;-)
Tbh personally I'm mostly interested in /var/lib/containerd specifically, so if it were ever removed I'd maybe reimplement it (using a different env var name to avoid confusion!) and send another PR.
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
3+ months later and the project detail that caused the main criticism to this PR ("volumes are an internal detail subject to change") has not been changed. |
I'm sad that you have that impression, is not common to need such amount of disk space, actually you were the first requesting it, lots of CIs are running thousands of KIND clusters daily in less that 14 Gb without any issues /close Ben explained the technical problems to maintain a solutions like yours in the long term, anyway, we hope to see you again |
@aojea: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@aojea Hi Antonio, I think there's been a misunderstanding here, just wanted to clarify things. Anyway have a great 2021 🎉 |
lol, thanks for the clarification, I was totally getting it wrong :D |
#1834