Skip to content
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

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

presztak
Copy link
Member

@presztak presztak commented Oct 3, 2022

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:

  1. Enable addon:
$ minikube addons enable storage-provisioner-rancher
❗  storage-provisioner-rancher is a 3rd party addon and not maintained or verified by minikube maintainers, enable at your own risk.
    ▪ Using image docker.io/rancher/local-path-provisioner:v0.0.22
    ▪ Using image docker.io/busybox:stable
🌟  The 'storage-provisioner-rancher' addon is enabled

$ kubectl get pods -n local-path-storage
NAME                                     READY   STATUS    RESTARTS   AGE
local-path-provisioner-7f58b4649-tnkj2   1/1     Running   0          34s

$ kubectl get sc
NAME                   PROVISIONER                RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
local-path (default)   rancher.io/local-path      Delete          WaitForFirstConsumer   false                  3m23s
standard               k8s.io/minikube-hostpath   Delete          Immediate              false                  3m59s
  1. Create pvc and pod with following yaml:
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
 name: test-pvc
spec:
 accessModes:
   - ReadWriteOnce
 resources:
   requests:
     storage: 64Mi
---
apiVersion: v1
kind: Pod
metadata:
 name: test-local-path
spec:
 nodeSelector:
   "kubernetes.io/hostname": "minikube-m02"
 containers:
 - name: busybox
   image: busybox:stable
   command: ["sh", "-c", "echo 'local-path-provisioner' > /test/file1"]
   volumeMounts:
   - name: data
     mountPath: /test
 volumes:
 - name: data
   persistentVolumeClaim:
     claimName: test-pvc
$ kubectl get pvc
NAME       STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
test-pvc   Bound    pvc-f07e253b-fea7-433a-b0ac-1bcea3f77076   64Mi       RWO            local-path     5m19s

$ kubectl get pods -o wide
NAME              READY   STATUS      RESTARTS   AGE     IP           NODE           NOMINATED NODE   READINESS GATES
test-local-path   0/1     Completed   0          5m19s   10.244.1.5   minikube-m02   <none>           <none>
  1. On the second node we are able to see created file with content local-path-provisioner:
$ minikube ssh -n minikube-m02 "cat /opt/local-path-provisioner/pvc-f07e253b-fea7-433a-b0ac-1bcea3f77076_default_test-pvc/file1"
local-path-provisioner

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 3, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2022
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@medyagh
Copy link
Member

medyagh commented Oct 3, 2022

thank you very much @resztak for this PR, do you mind sharing output of example of using this addon ?
maybe consider adding an entry to our website either https://minikube.sigs.k8s.io/docs/tutorials/
or

@presztak
Copy link
Member Author

presztak commented Oct 4, 2022

@medyagh I've added example of usage and tutorial.

@medyagh
Copy link
Member

medyagh commented Oct 5, 2022

@presztak thank you very much for this contribution this looks good to me, once @spowelljr approves it

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2022
Copy link
Member

@spowelljr spowelljr left a 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?

@presztak presztak force-pushed the rancher_local_path branch 2 times, most recently from 6c81e58 to 7a866d4 Compare January 14, 2023 12:20
@presztak
Copy link
Member Author

@spowelljr I've added integration test.

@presztak presztak requested review from spowelljr and removed request for prezha and sharifelgamal January 14, 2023 13:06
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 9, 2023
@spowelljr spowelljr force-pushed the rancher_local_path branch from 7a866d4 to c59e0a9 Compare June 14, 2023 03:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2023
@spowelljr spowelljr removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 14, 2023
@spowelljr
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 14, 2023
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@spowelljr
Copy link
Member

Local path test is failing due to the test-local-path pod crashing with:

  Warning  BackOff    4s (x5 over 45s)  kubelet            Back-off restarting failed container busybox in pod test-local-path_default(0bb63a24-bb38-4834-b57f-4259596ac7e0)

@minikube-pr-bot

This comment has been minimized.

@presztak presztak force-pushed the rancher_local_path branch 2 times, most recently from 843116a to 098f474 Compare September 13, 2023 21:15
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@presztak
Copy link
Member Author

@spowelljr @medyagh can you take another look on this?

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

Copy link
Member

@medyagh medyagh left a 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2023
Comment on lines 218 to 219
"LocalPathProvisioner": "rancher/local-path-provisioner:v0.0.22",
"Helper": "busybox:stable",
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spowelljr Done.

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15062) |
+----------------+----------+---------------------+
| minikube start | 52.2s    | 51.0s               |
| enable ingress | 27.2s    | 29.4s               |
+----------------+----------+---------------------+

Times for minikube start: 56.0s 52.2s 51.1s 50.8s 50.7s
Times for minikube (PR 15062) start: 51.3s 50.5s 52.6s 50.2s 50.4s

Times for minikube ingress: 25.2s 28.6s 28.2s 26.1s 28.1s
Times for minikube (PR 15062) ingress: 29.7s 28.7s 30.2s 28.6s 29.6s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15062) |
+----------------+----------+---------------------+
| minikube start | 24.0s    | 24.7s               |
| enable ingress | 21.5s    | 20.8s               |
+----------------+----------+---------------------+

Times for minikube start: 24.4s 24.2s 24.3s 22.4s 24.7s
Times for minikube (PR 15062) start: 24.8s 24.4s 24.1s 25.5s 24.8s

Times for minikube ingress: 21.3s 22.8s 20.8s 21.8s 20.8s
Times for minikube (PR 15062) ingress: 20.8s 20.8s 20.8s 20.8s 20.8s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15062) |
+----------------+----------+---------------------+
| minikube start | 21.7s    | 22.4s               |
| enable ingress | 31.1s    | 30.9s               |
+----------------+----------+---------------------+

Times for minikube start: 19.9s 19.8s 23.9s 21.7s 23.0s
Times for minikube (PR 15062) start: 21.3s 24.2s 23.8s 20.6s 22.1s

Times for minikube ingress: 31.3s 30.3s 31.3s 31.3s 31.3s
Times for minikube (PR 15062) ingress: 30.3s 31.3s 31.3s 30.3s 31.3s

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
QEMU_macOS TestFunctional/parallel/FileSync (gopogh) 4.94 (chart)
QEMU_macOS TestFunctional/parallel/NonActiveRuntimeDisabled (gopogh) 4.94 (chart)
QEMU_macOS TestFunctional/parallel/UpdateContextCmd/no_clusters (gopogh) 4.94 (chart)
QEMU_macOS TestFunctional/parallel/UpdateContextCmd/no_minikube_cluster (gopogh) 4.94 (chart)
QEMU_macOS TestFunctional/parallel/Version/components (gopogh) 4.94 (chart)
QEMU_macOS TestFunctional/serial/LogsCmd (gopogh) 4.94 (chart)
QEMU_macOS TestIngressAddonLegacy/serial/ValidateIngressAddonActivation (gopogh) 4.94 (chart)
QEMU_macOS TestIngressAddonLegacy/StartLegacyK8sCluster (gopogh) 4.94 (chart)
Hyper-V_Windows TestStartStop/group/old-k8s-version/serial/VerifyKubernetesImages (gopogh) 5.17 (chart)
QEMU_macOS TestFunctional/parallel/CpCmd (gopogh) 5.56 (chart)
QEMU_macOS TestFunctional/parallel/CertSync (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/DockerEnv/bash (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageBuild (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListJson (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListTable (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageLoadFromFile (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageSaveToFile (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageTagAndLoadDaemon (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/NodeLabels (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/DeployApp (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/Format (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/HTTPS (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/JSONOutput (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/List (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/URL (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/SSHCmd (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/StatusCmd (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/TunnelCmd/serial/AccessDirect (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/TunnelCmd/serial/AccessThroughDNS (gopogh) 6.17 (chart)
QEMU_macOS TestFunctional/parallel/TunnelCmd/serial/DNSResolutionByDig (gopogh) 6.17 (chart)
More tests... Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

@andresmmujica
Copy link

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!

@spowelljr
Copy link
Member

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 make if you have Go installed

@spowelljr spowelljr removed the do-not-merge/failing-test Indicates that a PR should not merge because it has a failing test label Sep 26, 2023
@spowelljr spowelljr merged commit efbb49e into kubernetes:master Sep 26, 2023
15 of 16 checks passed
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage provisioner broken for multinode mode
8 participants