Customizing host path for dynamically provisioned PersistentVolumes #6156

nanikjava · 2019-12-23T18:57:30Z

This fix contains few things:

Used k8s.gcr.io/debian-base-amd64:v2.0.0 as base image to build storage-provisioner
Modify RBAC permission used to cluster-admin

k8s-ci-robot · 2019-12-23T18:57:39Z

Hi @nanikjava. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2019-12-23T18:57:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nanikjava
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

minikube-bot · 2019-12-23T18:58:02Z

Can one of the admins verify this patch?

codecov-io · 2019-12-23T19:02:33Z

Codecov Report

Merging #6156 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6156   +/-   ##
=======================================
  Coverage   38.38%   38.38%           
=======================================
  Files         123      123           
  Lines        8318     8318           
=======================================
  Hits         3193     3193           
  Misses       4706     4706           
  Partials      419      419

nanikjava · 2019-12-23T19:03:32Z

deploy/addons/storage-provisioner/storage-provisioner.yaml.tmpl

@@ -31,7 +31,7 @@ metadata:
 roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
-  name: system:persistent-volume-provisioner
+  name: cluster-admin


This permission works fine but uncertain whether this is the correct one to use. Tried using the following

system:controller:expand-controller
system:controller:persistent-volume-binder

but it always complain permission issue either with persistentvolume/persistentvolumeclaims.

Tried defining customised RBAC inside storage-provisioner.yaml.tmp also does not work as it keep on thrown permission issue with persistentvolume(claims) and endpoints (using ClusterRole similar to defined here https://github.com/pragkent/aliyun-disk-provisioner/blob/38680a9607b0d33567be1bf0a7c57b26f0960549/deploy/rbac.yaml)

cmd/storage-provisioner/main.go

RA489 · 2020-01-08T05:44:24Z

/ok-to-test

tstromberg · 2020-01-08T23:00:52Z

deploy/storage-provisioner/Dockerfile

-FROM scratch
-COPY out/storage-provisioner storage-provisioner
-CMD ["/storage-provisioner"]
+FROM k8s.gcr.io/debian-base-amd64:v2.0.0


Can this stay scratch?

I think we need to provide a build container for the copied file, right now it cheerfully builds it on the host ?

Use the normal MINIKUBE_BUILD_IN_DOCKER shenaningans, I don't see what it has do with path though...

Can this stay scratch?

Can I resolve this conversation ? as it's clear now in the slack channel conversation that we cannot use scratch ?

As long as you build static binaries (using CGO_ENABLED=0), then scratch should be fine.

What issues are you seeing ? We know that a dynamic executable doesn't work with scratch...
But that was probably not the intention, but just a side-effect when redoing the make earlier.

I added the build container in #6257 if you want to have a look.

cmd/storage-provisioner/main.go

afbjorklund · 2020-01-14T16:56:51Z

Is it possible to merge this PR with the previous one #5400 - with the same content ?

This fix contains few things: * Used k8s.gcr.io/debian-base-amd64:v2.0.0 as base image to build storage-provisioner * Modify RBAC permission used to cluster-admin * Build storage-provisioner as static binary

minikube-bot · 2020-01-16T09:21:52Z

Error: running mkcmp
2020/01/16 09:21:51 Executing run 1/3...
2020/01/16 09:21:51 Running: [/home/performance-monitor/minikube/out/minikube start]...
2020/01/16 09:21:51 0.324773: * minikube v1.7.0-beta.0 on Debian 9.11
2020/01/16 09:21:51 0.039983: * Selecting 'kvm2' driver from user configuration (alternates: [none docker])
! Unable to update kvm2 driver: download failed: https://github.com/kubernetes/minikube/releases/download/v1.7.0-beta.0/docker-machine-driver-kvm2?checksum=file:https://github.com/kubernetes/minikube/releases/download/v1.7.0-beta.0/docker-machine-driver-kvm2.sha256: invalid checksum: Error downloading checksum file: bad response code: 404
2020/01/16 09:21:52 0.575668: * Downloading driver docker-machine-driver-kvm2:
*
X Unable to start VM. Please investigate and run 'minikube delete' if possible

Error: [KVM2_NOT_FOUND] new host: Driver "kvm2" not found. Do you have the plugin binary "docker-machine-driver-kvm2" accessible in your PATH?
Suggestion: Please install the minikube kvm2 VM driver, or select an alternative --vm-driver
Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/kvm2/
: exit status 1

minikube-pr-bot · 2020-01-22T21:37:59Z

All Times minikube: [ 112.810345 133.827582 114.781896]
All Times Minikube (PR 6156): [ 223.115197 229.509844 224.994223]

Average minikube: 120.473274
Average Minikube (PR 6156): 225.873088

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6156) |
+----------------------+-----------+--------------------+
| minikube v           |  0.283592 |           0.280954 |
| Creating kvm2        | 40.670139 |         159.997635 |
| Preparing Kubernetes | 47.222829 |          40.386523 |
| Pulling images       |  4.121378 |           2.178521 |
| Launching Kubernetes | 25.088212 |          18.466889 |
| Waiting for cluster  |  2.565540 |           2.554372 |
+----------------------+-----------+--------------------+

tstromberg · 2020-02-05T19:29:01Z

Looks good, do you mind addressing the merge conflict?

tstromberg · 2020-02-05T19:32:51Z

/ok-to-test

minikube-pr-bot · 2020-02-05T19:50:31Z

All Times Minikube (PR 6156): [ 97.411594 96.564944 94.135690]
All Times minikube: [ 98.147437 97.062703 96.001295]

Average minikube: 97.070478
Average Minikube (PR 6156): 96.037410

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6156) |
+----------------------+-----------+--------------------+
| minikube v           |  0.302838 |           0.290483 |
| Creating kvm2        | 20.305448 |          19.510336 |
| Preparing Kubernetes | 49.737568 |          50.309433 |
| Pulling images       |  4.020470 |           2.958651 |
| Launching Kubernetes | 19.934128 |          19.747588 |
| Waiting for cluster  |  1.074359 |           1.394866 |
+----------------------+-----------+--------------------+

GrahamDumpleton · 2020-08-12T13:00:15Z

Can someone explain to me the justification for having the file system directory created use the PVC name instead of the PV name. In other words, the change at line 60 from:

path := path.Join(p.pvDir, options.PVName)

to:

path := path.Join(p.pvDir, options.PVC.Name)

This means that two different applications in different namespaces can't use the same persistent volume claim name because if they do, they end up using the same directory on the filesystem, and thus they corrupt each other if directories/file names they use overlap inside the volume. This can break so many applications. It basically means you can't deploy multiple instances of the one application in different namespaces.

This introduces a serious bug and is not a feature.

GrahamDumpleton · 2020-08-12T13:05:29Z

The obvious solution to avoid for this would have been to use path of form root/namespace/pvc-name if wanted more user friendly name.

Eelis · 2020-08-12T13:08:24Z

This introduces a serious bug

Ah good point, I hadn't considered this.

The obvious solution to avoid for this would have been to use path of form root/namespace/pvc-name if wanted more user friendly name.

Sounds good to me!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 23, 2019

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 23, 2019

k8s-ci-robot requested review from medyagh and tstromberg December 23, 2019 18:58

nanikjava commented Dec 23, 2019

View reviewed changes

Eelis reviewed Dec 24, 2019

View reviewed changes

cmd/storage-provisioner/main.go Outdated Show resolved Hide resolved

nanikjava force-pushed the f-fix-5144 branch from 9290ab2 to 5360793 Compare December 25, 2019 00:02

nanikjava mentioned this pull request Dec 25, 2019

Feature request: Allow customizing host path used for dynamically provisioned PersistentVolumes #5144

Open

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 8, 2020

tstromberg approved these changes Jan 8, 2020

View reviewed changes

tstromberg reviewed Jan 8, 2020

View reviewed changes

cmd/storage-provisioner/main.go Show resolved Hide resolved

AndrienkoAleksandr mentioned this pull request Jan 10, 2020

Extra option --PersistentVolumeName and --HostPathSource to CheCTL eclipse-che/che#15157

Closed

nanikjava force-pushed the f-fix-5144 branch from 5360793 to 9453b37 Compare January 15, 2020 23:41

Customizing host path for dynamically provisioned PersistentVolumes

9b455aa

This fix contains few things: * Used k8s.gcr.io/debian-base-amd64:v2.0.0 as base image to build storage-provisioner * Modify RBAC permission used to cluster-admin * Build storage-provisioner as static binary

nanikjava force-pushed the f-fix-5144 branch from 9453b37 to 9b455aa Compare January 16, 2020 01:24

nanikjava mentioned this pull request Jan 16, 2020

Error scheduling default/mysql: pod has unbound immediate PersistentVolumeClaims; retrying #6246

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2020

tstromberg approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into f-fix-5144

0d29a2a

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2020

tstromberg mentioned this pull request Feb 5, 2020

[WIP] Make dynamic persistent volumes path readable and configurable #5400

Closed

tstromberg merged commit 88693f2 into kubernetes:master Feb 5, 2020

This was referenced Feb 5, 2020

none at head: The ClusterRoleBinding "storage-provisioner" is invalid: cannot change roleRef #6510

Closed

Revert role change from cluster-admin->system:persistent-volume-provisioner #6511

Merged

dimara mentioned this pull request Feb 6, 2020

latest storage-provisioner: cannot get resource "endpoints" in API group "" in the namespace "kube-system" #6523

Closed

GrahamDumpleton mentioned this pull request Aug 13, 2020

provisioner: Data corruption if PVC Name is used across namespaces #8987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customizing host path for dynamically provisioned PersistentVolumes #6156

Customizing host path for dynamically provisioned PersistentVolumes #6156

nanikjava commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

minikube-bot commented Dec 23, 2019

codecov-io commented Dec 23, 2019 •

edited

Loading

nanikjava Dec 23, 2019 •

edited

Loading

RA489 commented Jan 8, 2020

tstromberg Jan 8, 2020

afbjorklund Jan 8, 2020

nanikjava Jan 9, 2020

afbjorklund Jan 9, 2020 •

edited

Loading

afbjorklund commented Jan 14, 2020

minikube-bot commented Jan 16, 2020

minikube-pr-bot commented Jan 22, 2020

tstromberg commented Feb 5, 2020

tstromberg commented Feb 5, 2020

minikube-pr-bot commented Feb 5, 2020

GrahamDumpleton commented Aug 12, 2020

GrahamDumpleton commented Aug 12, 2020 •

edited

Loading

Eelis commented Aug 12, 2020

Customizing host path for dynamically provisioned PersistentVolumes #6156

Customizing host path for dynamically provisioned PersistentVolumes #6156

Conversation

nanikjava commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

minikube-bot commented Dec 23, 2019

codecov-io commented Dec 23, 2019 • edited Loading

Codecov Report

nanikjava Dec 23, 2019 • edited Loading

Choose a reason for hiding this comment

RA489 commented Jan 8, 2020

tstromberg Jan 8, 2020

Choose a reason for hiding this comment

afbjorklund Jan 8, 2020

Choose a reason for hiding this comment

nanikjava Jan 9, 2020

Choose a reason for hiding this comment

afbjorklund Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

afbjorklund commented Jan 14, 2020

minikube-bot commented Jan 16, 2020

minikube-pr-bot commented Jan 22, 2020

tstromberg commented Feb 5, 2020

tstromberg commented Feb 5, 2020

minikube-pr-bot commented Feb 5, 2020

GrahamDumpleton commented Aug 12, 2020

GrahamDumpleton commented Aug 12, 2020 • edited Loading

Eelis commented Aug 12, 2020

codecov-io commented Dec 23, 2019 •

edited

Loading

nanikjava Dec 23, 2019 •

edited

Loading

afbjorklund Jan 9, 2020 •

edited

Loading

GrahamDumpleton commented Aug 12, 2020 •

edited

Loading