-
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
Add local-path-provisioner
addon
#15062
Conversation
Can one of the admins verify this patch? |
thank you very much @resztak for this PR, do you mind sharing output of example of using this addon ? |
@medyagh I've added example of usage and tutorial. |
@presztak thank you very much for this contribution this looks good to me, once @spowelljr approves it |
ffd5149
to
763452a
Compare
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.
Would it be possible to add an integration test for this new addon?
deploy/addons/storage-provisioner-rancher/storage-provisioner-rancher.yaml.tmpl
Outdated
Show resolved
Hide resolved
6c81e58
to
7a866d4
Compare
@spowelljr I've added integration test. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
7a866d4
to
c59e0a9
Compare
/ok-to-test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Local path test is failing due to the
|
This comment has been minimized.
This comment has been minimized.
843116a
to
098f474
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
098f474
to
7852d84
Compare
@spowelljr @medyagh can you take another look on this? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
looks good to me, thank you @presztak for this great contribution and the persistence to get it to the finish line with both integration tests and tutorial . look forward to see more PRs from you
pkg/minikube/assets/addons.go
Outdated
"LocalPathProvisioner": "rancher/local-path-provisioner:v0.0.22", | ||
"Helper": "busybox:stable", |
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.
For security reasons these images should be pinned to a SHA, see others for example
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.
yes good call ! please do that @presztak you can see example of other addons
and to get the sha you can do "docker pull" and it will spit out the sha in the end.
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.
@spowelljr thanks for review! Actually SHA was present in original PR but then I've realized that images with hardcoded SHA would not work on arm architecture. Should we detect architecture and regarding this use image with proper SHA or can we solve this with different approach?
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.
The SHA will work on arm64 as it gives the SHA of the manifest, then the manifest directs it to download the correct arch. To confirm I just tried on an amd64 & arm64 machine
amd64:
Digest: sha256:e34c88ae0affb1cdefbb874140d6339d4a27ec4ee420ae8199cd839997b05246
Status: Downloaded newer image for rancher/local-path-provisioner:v0.0.22
docker.io/rancher/local-path-provisioner:v0.0.22
arm64:
Digest: sha256:e34c88ae0affb1cdefbb874140d6339d4a27ec4ee420ae8199cd839997b05246
Status: Downloaded newer image for rancher/local-path-provisioner:v0.0.22
docker.io/rancher/local-path-provisioner:v0.0.22
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.
@spowelljr Done.
7852d84
to
e527572
Compare
e527572
to
a51423d
Compare
This comment has been minimized.
This comment has been minimized.
kvm2 driver with docker runtime
Times for minikube start: 56.0s 52.2s 51.1s 50.8s 50.7s Times for minikube ingress: 25.2s 28.6s 28.2s 26.1s 28.1s docker driver with docker runtime
Times for minikube start: 24.4s 24.2s 24.3s 22.4s 24.7s Times for minikube ingress: 21.3s 22.8s 20.8s 21.8s 20.8s docker driver with containerd runtime
Times for minikube start: 19.9s 19.8s 23.9s 21.7s 23.0s Times for minikube ingress: 31.3s 30.3s 31.3s 31.3s 31.3s |
This comment has been minimized.
This comment has been minimized.
These are the flake rates of all failed tests.
Too many tests failed - See test logs for more details. To see the flake rates of all tests by environment, click here. |
Hi all, I was wondering if there's a way to enable this in my minikube install while it gets merged or should I try the workaround from #12165 (comment) Thanks all for your help and contributions! |
You can checkout this branch and build it from source with |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh, presztak, spowelljr 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 |
This PR adds local-path-provisioner as addon that supports multi-node setups. It resolves some issues which happend with standard storage-provisioner on multi-node cluster.
Fixes: #12165 #12360
In addition also fixes issue with setting default storage class. Currently is not working properly because
enableOrDisableStorageClasses
tries to set storage class as default before it is enabled.Example of usage:
local-path-provisioner
: