Make createEndpointService() and deleteEndpointService() plugin interface methods. #45528

humblec · 2017-05-09T06:47:33Z

Why this change?

In some setups, after creation of dynamic PVs and before mounting/using these PVs in a pod, the endpoint/service got mistakenly deleted by the user/developer. By making these methods 'plugin' specific, we can call it from mounter if there are scenarios where the endpoint and service got wiped in between accidentally.

Signed-off-by: Humble Chirammal hchiramm@redhat.com

k8s-reviewable · 2017-05-09T06:47:39Z

This change is

k8s-ci-robot · 2017-05-09T06:47:42Z

Hi @humblec. 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 @k8s-bot 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.

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.

humblec · 2017-05-09T06:48:45Z

/release-note-none

humblec · 2017-05-09T06:49:07Z

/assign @jsafrane @rootfs

humblec · 2017-05-09T06:49:24Z

@k8s-bot ok to test

k8s-ci-robot · 2017-05-09T06:49:38Z

@humblec: you can't request testing unless you are a kubernetes member.

In response to this:

@k8s-bot ok to test

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.

rootfs · 2017-05-09T13:33:08Z

@k8s-bot ok to test

rootfs · 2017-05-09T13:35:50Z

These two methods are used by the provisioner, plugin doesn't need to create/delete endpoints for now.

If the use case is generic enough, we can consider adding endpoint methods as volume utilities.

humblec · 2017-05-09T13:40:13Z

@rootfs you are correct, these are provisioner methods. The main reason to make this change is due to below.
In some setups, after creation of PVs and before mounting of the PV in a pod, the endpoint got mistakenly deleted by the user. So, what I am planning with this change is calling this function at mount time, if some how the endpoint and service got wiped in between. I would have mentioned this in the commit message. I will update the commit message if you ack on this change.

humblec · 2017-05-09T14:22:11Z

@rootfs the PR note reflect the reason for this change. Please review. Thanks !

rootfs · 2017-05-12T18:31:49Z

pkg/volume/glusterfs/glusterfs.go

 	ep, err := kubeClient.Core().Endpoints(ns).Get(epName, metav1.GetOptions{})
 	if err != nil {
 		glog.Errorf("glusterfs: failed to get endpoints %s[%v]", epName, err)
-		return nil, err
+
+		class, err := volutil.GetClassForVolume(plugin.host.GetKubeClient(), spec.PersistentVolume)


recreate endpoint if:

spec != nil

err indicates the endpoint doesn't exist.

rootfs · 2017-05-12T18:33:08Z

pkg/volume/glusterfs/glusterfs.go

+
+		class, err := volutil.GetClassForVolume(plugin.host.GetKubeClient(), spec.PersistentVolume)
+		if err != nil {
+			return nil, fmt.Errorf("glusterfs: failed to get storageclass: %v", err)


PV may not be created via storage class. Tune the message a bit.

humblec · 2017-05-15T11:20:42Z

@rootfs Addressed the comments. Please review.

rootfs · 2017-05-15T12:58:30Z

pkg/volume/glusterfs/glusterfs.go

+		if spec != nil && spec.PersistentVolume.Annotations["kubernetes.io/createdby"] == "heketi-dynamic-provisioner" {
+			class, err := volutil.GetClassForVolume(plugin.host.GetKubeClient(), spec.PersistentVolume)
+			if err != nil {
+				return nil, fmt.Errorf("glusterfs: failed to get storageclass when recreating endpoint/service: %v", err)


If the PV has no storage class, don't treat it as an error.

rootfs · 2017-05-15T19:03:28Z

pkg/volume/glusterfs/glusterfs.go

+		if spec != nil && spec.PersistentVolume.Annotations["kubernetes.io/createdby"] == "heketi-dynamic-provisioner" {
+			class, err := volutil.GetClassForVolume(plugin.host.GetKubeClient(), spec.PersistentVolume)
+			if err != nil {
+				return nil, fmt.Errorf("glusterfs: failed to recreate endpoint/service, error: %v", err)


failed to get storage class

rootfs · 2017-05-15T19:04:14Z

pkg/volume/glusterfs/glusterfs.go

-		return nil, err
+	if err != nil && errors.IsNotFound(err) {
+		glog.Errorf("glusterfs: failed to get endpoint %s[%v]", epName, err)
+		if spec != nil && spec.PersistentVolume.Annotations["kubernetes.io/createdby"] == "heketi-dynamic-provisioner" {


make heketi-dynamic-provisioner a const and replace it in provision() too

rootfs · 2017-05-15T19:04:39Z

pkg/volume/glusterfs/glusterfs.go

+
+			cfg, err := parseClassParameters(class.Parameters, plugin.host.GetKubeClient())
+			if err != nil {
+				return nil, fmt.Errorf("glusterfs: failed to recreate endpoint/service, error: %v", err)


failed to parse parameters

rootfs · 2017-05-15T19:04:56Z

pkg/volume/glusterfs/glusterfs.go

+			scConfig := *cfg
+			cli := gcli.NewClient(scConfig.url, scConfig.user, scConfig.secretValue)
+			if cli == nil {
+				return nil, fmt.Errorf("glusterfs: failed to recreate endpoint/service, error: %v", err)


failed to create heketi client

rootfs · 2017-05-15T19:05:13Z

pkg/volume/glusterfs/glusterfs.go

+			volumeID := dstrings.TrimPrefix(source.Path, volPrefix)
+			volInfo, err := cli.VolumeInfo(volumeID)
+			if err != nil {
+				return nil, fmt.Errorf("glusterfs: failed to recreate endpoint/service, error: %v", err)


failed to get volume info

rootfs · 2017-05-15T19:05:30Z

pkg/volume/glusterfs/glusterfs.go

+
+			endpointIPs, err := getClusterNodes(cli, volInfo.Cluster)
+			if err != nil {
+				return nil, fmt.Errorf("glusterfs: failed to recreate endpoint/service, error: %v", err)


failed to get cluster nodes

rootfs · 2017-05-15T19:12:46Z

pkg/volume/glusterfs/glusterfs.go

-	if err != nil {
-		glog.Errorf("glusterfs: failed to get endpoints %s[%v]", epName, err)
-		return nil, err
+	if err != nil && errors.IsNotFound(err) {


a bit more paranoid but validate that epName is created using the same schema that provision uses.

rootfs · 2017-05-16T13:23:34Z

pkg/volume/glusterfs/glusterfs.go

+		claim := spec.PersistentVolume.Spec.ClaimRef.Name
+		checkEpName := dynamicEpSvcPrefix + claim
+		if epName != checkEpName {
+			return nil, fmt.Errorf("failed to get proper endpoint name, error: %v", err)


glusterfs: failed to get endpoints %s, error %v since in this case, failed validation indicates the volume is not provisioned by provision().

humblec · 2017-05-16T14:14:27Z

@k8s-bot verify test this

rootfs · 2017-05-16T14:15:31Z

/approve
@k8s-bot verify test this

@humblec squash commits, then lgtm

In some setups, after creation of dynamic PVs and before mounting/using these PVs in a pod, the endpoint/service got mistakenly deleted by the user/developer. By making these methods 'plugin' specific, we can call it from mounter if there are scenarios where the endpoint and service got wiped in between accidentally. Signed-off-by: Humble Chirammal hchiramm@redhat.com

rootfs · 2017-05-16T15:07:35Z

/lgtm

k8s-github-robot · 2017-05-16T15:07:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, rootfs

Needs approval from an approver in each of these OWNERS Files:

~~pkg/volume/glusterfs/OWNERS~~ [rootfs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

humblec · 2017-05-16T15:08:28Z

Thanks a lot @rootfs !

k8s-github-robot · 2017-05-16T16:11:59Z

Automatic merge from submit-queue (batch tested with PRs 45408, 45355, 45528)

k8s-github-robot · 2017-05-16T16:16:04Z

@humblec PR needs rebase

liggitt · 2017-06-16T14:29:41Z

pkg/volume/glusterfs/glusterfs.go

+		}
+		glog.Errorf("glusterfs: failed to get endpoint %s[%v]", epName, err)
+		if spec != nil && spec.PersistentVolume.Annotations["kubernetes.io/createdby"] == heketiAnn {
+			class, err := volutil.GetClassForVolume(plugin.host.GetKubeClient(), spec.PersistentVolume)


nodes cannot look up arbitrary secrets based on values in storage class parameters...

I also thought only the create/delete, attach/detach phases were supposed to resolve storage class info... isn't mount only supposed to use info in the PV?

liggitt · 2017-06-16T14:30:28Z

pkg/volume/glusterfs/glusterfs.go

+
+			// Give an attempt to recreate endpoint/service.
+
+			_, _, err = plugin.createEndpointService(ns, epName, endpointIPs, claim)


nodes do not have permission to create endpoints or services... this will fail on any cluster running RBAC

Signed-off-by: Huamin Chen <hchen@redhat.com>

@humblec

Automatic merge from submit-queue (batch tested with PRs 47726, 47693, 46909, 46812) manually revert #45528 **What this PR does / why we need it**: Revert #45528 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #47657 **Special notes for your reviewer**: @humblec @liggitt @saad-ali @kubernetes/kubernetes-release-managers **Release note**: ```release-note NONE ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 9, 2017

k8s-github-robot assigned pmorie and smarterclayton May 9, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 9, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 9, 2017

k8s-ci-robot assigned jsafrane and rootfs May 9, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 9, 2017

humblec force-pushed the glusterfs-code-refactor branch from 06fe9a2 to d32df0c Compare May 9, 2017 06:52

humblec changed the title ~~Make createEndpointService() and deleteEndpointService() plugin interface methods.~~ [WIP] Make createEndpointService() and deleteEndpointService() plugin interface methods. May 12, 2017

rootfs reviewed May 12, 2017

View reviewed changes

humblec force-pushed the glusterfs-code-refactor branch from b204b6c to 0879bac Compare May 15, 2017 10:06

humblec changed the title ~~[WIP] Make createEndpointService() and deleteEndpointService() plugin interface methods.~~ Make createEndpointService() and deleteEndpointService() plugin interface methods. May 15, 2017

humblec force-pushed the glusterfs-code-refactor branch from 0879bac to 0c61cf6 Compare May 15, 2017 10:23

rootfs reviewed May 15, 2017

View reviewed changes

humblec force-pushed the glusterfs-code-refactor branch from 01d4b4b to f4c6cca Compare May 16, 2017 05:25

rootfs reviewed May 16, 2017

View reviewed changes

humblec force-pushed the glusterfs-code-refactor branch from f4c6cca to 057944e Compare May 16, 2017 13:58

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2017

humblec force-pushed the glusterfs-code-refactor branch from 057944e to 936c81d Compare May 16, 2017 14:22

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2017

k8s-github-robot merged commit 11a6f19 into kubernetes:master May 16, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2017

liggitt reviewed Jun 16, 2017

View reviewed changes

liggitt mentioned this pull request Jun 16, 2017

gluster mounter runs unpermitted operations from nodes #47657

Closed

rootfs mentioned this pull request Jun 19, 2017

manually revert #45528 #47726

Merged

rootfs added a commit to rootfs/kubernetes that referenced this pull request Jun 19, 2017

manually revert kubernetes#45528

14c94fa

Signed-off-by: Huamin Chen <hchen@redhat.com>


		// Give an attempt to recreate endpoint/service.

		_, _, err = plugin.createEndpointService(ns, epName, endpointIPs, claim)

Make createEndpointService() and deleteEndpointService() plugin interface methods. #45528

Make createEndpointService() and deleteEndpointService() plugin interface methods. #45528

Conversation

humblec commented May 9, 2017 • edited

k8s-reviewable commented May 9, 2017

k8s-ci-robot commented May 9, 2017

humblec commented May 9, 2017

humblec commented May 9, 2017

humblec commented May 9, 2017

k8s-ci-robot commented May 9, 2017

rootfs commented May 9, 2017

rootfs commented May 9, 2017

humblec commented May 9, 2017 • edited

humblec commented May 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented May 16, 2017

rootfs commented May 16, 2017

rootfs commented May 16, 2017

k8s-github-robot commented May 16, 2017

humblec commented May 16, 2017

k8s-github-robot commented May 16, 2017

k8s-github-robot commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented May 9, 2017 •

edited

humblec commented May 9, 2017 •

edited

humblec commented May 9, 2017 •

edited