-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Convert PVC storage path to /var/tmp #7865
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @jradtilbrook! |
Hi @jradtilbrook. Thanks for your PR. I'm waiting for a kubernetes 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: jradtilbrook 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 |
Codecov Report
@@ Coverage Diff @@
## master #7865 +/- ##
==========================================
- Coverage 36.68% 35.57% -1.12%
==========================================
Files 146 148 +2
Lines 8978 9297 +319
==========================================
+ Hits 3294 3307 +13
- Misses 5292 5593 +301
- Partials 392 397 +5
|
Can one of the admins verify this patch? |
/check-cla |
The CLA has been signed |
/ok-to-test |
kvm2 Driver |
/check-cla |
/ok-to-test |
need any help with the CLA? |
Sorry @tstromberg it wasn't working and I forgot to come back to it. I believe I've done it now |
The logs are so verbose I'm not quite sure what the error is. If you can provide any more detail that would be much appreciated! 🙏 |
KVM had the folowing failures that did not seem related:
Likewise docker had:
Our integration tests are pretty flaky at the moment. They all seem unrelated, but I'll re-run the tests to see if any of these are consistent. |
/ok-to-test |
@tstromberg Is there anything I can do to assist? |
I'm going to have to give this some thought on how to roll this out: If a user upgrades their minikube, but have an older cluster running - they will end up with a new storage provisioner running on an old ISO. How should we we handle that? |
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.
cc @tstromberg
We could add a template field to path
in storage-provisioner.yaml.tmpl
path: {{.TmpHostPath}}
and we could set TmpHostPath=/tmp
if minikube version < 1.11 and TmpHostPath=/var/tmp
if minikube version >= 1.11.
WDYT?
This also needs to be synchronized with the kicbase implementation: 302a6b0 "Add minimal minikube-automount to the kic image" As well as documented properly, for current users of the "none" drivers. https://minikube.sigs.k8s.io/docs/handbook/persistent_volumes/ As I wrote in the issue #7511 (comment), I don't think this change is worth it... The /tmp location was never meant for storage... And /var/tmp might not be ideal. |
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. |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
@sharifelgamal I noticed you removed the stale label. I'm wondering if I should just close this as stale. I don't have time to work on it and have fixed my own issue with a bind mount on my host manually for now |
@jradtilbrook |
@medyagh I enabled a systemd mount unit that bind mounts the directory onto |
Closing by request until author has a chance to revisit this. |
Fixes #7511
This changes the mount volume of the storage provisioner to
/var/tmp
on the host system.On modern linux systems this is a normal mount point, as compared to
/tmp
which is generally atmpfs
which is removed upon reboots so persistant volume claims are not actually persistent.