Makeuse of PVC namespace when provisioning gluster volumes. #34705

humblec · 2016-10-13T11:10:00Z

Depends on #34611

This change is

k8s-ci-robot · 2016-10-13T11:10:01Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

humblec · 2016-10-13T14:54:16Z

@rootfs @jsafrane PTAL.

rootfs · 2016-10-13T19:07:16Z

examples/experimental/persistent-volume-provisioning/README.md

  resturl: "http://127.0.0.1:8081"
  restuser: "admin"
  secretNamespace: "default"
  secretName: "heketi-secret"
 ```
-
-* `endpoint`: `glusterfs-cluster` is the endpoint name which includes GlusterFS trusted pool IP addresses. This parameter is mandatory. We need to also create a service for this endpoint, so that the endpoint will be persisted. This service can be without a selector to tell Kubernetes we want to add its endpoints manually.  Please note that, glusterfs plugin looks for the endpoint in the pod namespace, so it is mandatory that the endpoint and service have to be created in Pod's namespace for successful mount of gluster volumes in the pod.
+* `endpoint`: This option is deprecated as the provisioner now create endpoint and service dynamically to support multi cluster setup.


just remove it? it is a parameter.

rootfs · 2016-10-13T19:08:39Z

pkg/volume/glusterfs/glusterfs.go

-		for _, a := range s.Addresses {
-			addr[a.IP] = struct{}{}
+	var addrlist []string
+	if b.hosts != nil {


add b.hosts == nil test, return error; keep rest as it is.

rootfs · 2016-10-13T19:11:44Z

pkg/volume/glusterfs/glusterfs.go

+	if pvSpec.Glusterfs.EndpointsName != "" {
+		dynamicEndpoint = pvSpec.Glusterfs.EndpointsName
+	}
+	glog.V(1).Infof("glusterfs: dynamic endpoint and namespace : [%v/%v]", dynamicEndpoint, dynamicNamespace)


V(1) -> V(3 or 4)
swap endpoint and namespace; usually we print namespace/endpoint

rootfs · 2016-10-13T19:14:49Z

pkg/volume/glusterfs/glusterfs.go

+				glog.Errorf("glusterfs: error when deleting endpoint/service :%s", err)
+			}
+		} else {
+			glog.V(3).Infof("glusterfs: cluster is not empty")


instead of logging cluster is not empty, log when the endpoint is deleted.

rootfs · 2016-10-13T19:15:29Z

pkg/volume/glusterfs/glusterfs.go

+	return endpoint, service, nil
+}
+
+func (d *glusterfsVolumeDeleter) DeleteEndpointService(namespace string, epServiceName string) (err error) {


since DeleteEndpointService is not used outside of deleter, make it lower case deleteEndpointService

Delete service first then endpoint. If endpoint is deleted then kubelet crashes, we leak a hanging service.

Done. afaict, if we delete svc first , it will delete the ep as well. So I kept ep deletion followed by svc to make sure both are gone. Any way reverted the change and only keeping service delete.

rootfs · 2016-10-13T19:19:03Z

pkg/volume/glusterfs/glusterfs.go

+		ipaddr := dstrings.Join(nodei.NodeAddRequest.Hostnames.Storage, "")
+		dynamicHostIps = append(dynamicHostIps, ipaddr)
+	}
+	glog.V(1).Infof("glusterfs: hostlist :%v", dynamicHostIps)


rootfs · 2016-10-13T19:19:25Z

pkg/volume/glusterfs/glusterfs.go

+	}
+	glog.V(1).Infof("glusterfs: hostlist :%v", dynamicHostIps)
+	if len(dynamicHostIps) == 0 {
+		glog.Errorf("glusterfs: no endpoint hosts found %s ", err)


no hosts found %v

rootfs · 2016-10-13T19:19:57Z

pkg/volume/glusterfs/glusterfs.go

+	for _, node := range clusterinfo.Nodes {
+		nodei, err := cli.NodeInfo(string(node))
+		if err != nil {
+			glog.Errorf("glusterfs: failed to get hostip %s ", err)


failed to get host ip %v

rootfs · 2016-10-13T19:20:22Z

pkg/volume/glusterfs/glusterfs.go

@@ -528,20 +563,113 @@ func (p *glusterfsVolumeProvisioner) CreateVolume() (r *api.GlusterfsVolumeSourc
 		glog.Errorf("glusterfs: failed to create gluster rest client")
 		return nil, 0, fmt.Errorf("failed to create gluster REST client, REST server authentication failed")
 	}
-	volumeReq := &gapi.VolumeCreateRequest{Size: sz, Durability: gapi.VolumeDurabilityInfo{Type: durabilitytype, Replicate: gapi.ReplicaDurability{Replica: replicacount}}}
+	volumeReq := &gapi.VolumeCreateRequest{Size: sz, Durability: gapi.VolumeDurabilityInfo{Type: durabilityType, Replicate: gapi.ReplicaDurability{Replica: replicaCount}}}
 	volume, err := cli.VolumeCreate(volumeReq)
 	if err != nil {
 		glog.Errorf("glusterfs: error creating volume %s ", err)


error creating volume %v

rootfs · 2016-10-13T19:30:27Z

pkg/volume/glusterfs/glusterfs.go

 	return &api.GlusterfsVolumeSource{
-		EndpointsName: p.endpoint,
+		EndpointsName: endpoint.Name,


i don't see Name in api.Endpoints

@rootfs it resolves from ObjectMeta. For ex:61917ff#diff-e97253dd603331ffca81131a4b67264fR626

rootfs · 2016-10-13T19:32:59Z

@jsafrane

rootfs

pass baton to @jsafrane @childsb

rootfs · 2016-10-17T13:24:01Z

pkg/volume/glusterfs/glusterfs.go

+	//Deleter takes endpoint and endpointnamespace from pv spec.
+	pvSpec := d.spec.Spec
+	var dynamicEndpoint, dynamicNamespace string
+	if pvSpec.ClaimRef.Namespace != "" {


@jsafrane I don't think of any empty namespace, but is any special meaning when we get an empty namespace?

rootfs · 2016-10-17T13:27:47Z

@k8s-bot gci gce e2e test this

humblec · 2016-10-18T09:39:45Z

@rootfs @jsafrane looks like we need @k8s-bot gci gke e2e test this, Could you please help ?

rootfs · 2016-10-18T12:53:36Z

@childsb ^^^

childsb · 2016-10-18T14:37:03Z

@k8s-bot gci gke e2e test this

humblec · 2016-10-18T15:02:21Z

@rootfs @childsb iic, it looks like, the failure is not due to the patch https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/34705/pull-kubernetes-e2e-gke-gci/102/?log .

gnufied · 2016-10-18T15:32:47Z

This looks like #33388 flake.

humblec · 2016-10-18T15:35:20Z

Thanks @gnufied !

childsb · 2016-10-18T15:36:45Z

flake on #33388
@k8s-bot gce e2e test this

humblec · 2016-10-19T00:28:11Z

@rootfs @childsb Can you please tag this PR for retriggering the tests?

humblec · 2016-10-19T07:28:24Z

Looks like the GCE e2e tests are stuck on this , its running for >13 hours now.

jsafrane · 2016-10-19T08:22:33Z

@k8s-bot test this issue: #IGNORE

k8s-ci-robot · 2016-10-19T08:48:19Z

Jenkins verification failed for commit b5733fb828fece9ab232a1182090e397f9559491. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jsafrane · 2016-10-19T08:57:19Z

@k8s-bot test this issue: #IGNORE

humblec · 2016-10-19T09:47:29Z

Thanks @jsafrane !!. The flake failure looks to be same which @gnufied mentioned.

humblec · 2016-10-19T10:44:49Z

@childsb @rootfs can you please approve this PR ?

rootfs · 2016-10-19T12:56:26Z

lgtm, @jsafrane @childsb ?

jsafrane · 2016-10-14T12:42:46Z

pkg/volume/glusterfs/glusterfs.go

+	var addrlist []string
+	if b.hosts == nil {
+		return fmt.Errorf("glusterfs: endpoint is nil")
+	} else {


you do not need extra else branch after return above, you ca save yourself one indentation level.

jsafrane · 2016-10-14T12:42:49Z

pkg/volume/glusterfs/glusterfs.go

+	if b.hosts == nil {
+		return fmt.Errorf("glusterfs: endpoint is nil")
+	} else {
+			addr := make(map[string]struct{})


addr seems not used, it's filled with addresses, is it read anywhere?

jsafrane · 2016-10-14T12:43:20Z

pkg/volume/glusterfs/glusterfs.go

+
+			// Avoid mount storm, pick a host randomly.
+			// Iterate all hosts until mount succeeds.
+			for _, ip := range addrlist {


"pick a host randomly" - the code above does not look too much random

jsafrane · 2016-10-14T12:45:43Z

pkg/volume/glusterfs/glusterfs.go

+	var dynamicEndpoint, dynamicNamespace string
+	if pvSpec.ClaimRef.Namespace != "" {
+		dynamicNamespace = pvSpec.ClaimRef.Namespace
+	}


please check if pvSpec.ClaimRef is nil (and throw an error if it is) and throw an error when ClaimRef.Namespace is empty

jsafrane · 2016-10-14T13:00:19Z

pkg/volume/glusterfs/glusterfs.go

+	}
+	_, err = p.plugin.host.GetKubeClient().Core().Endpoints(namespace).Create(endpoint)
+	if err != nil && errors.IsAlreadyExists(err) {
+		glog.V(1).Infof("glusterfs: endpoint %s already exist", endpoint)


please log also namespace when logging endpoint or service name (this applies to most of logging below)

jsafrane · 2016-10-14T13:01:18Z

pkg/volume/glusterfs/glusterfs.go

+	}
+	glog.V(3).Infof("glusterfs: dynamic namespace and endpoint : [%v/%v]", dynamicNamespace, dynamicEndpoint)
+	//If there are no volumes exist in the cluster, the endpoint and svc
+	//will be deleted.


This logic is suspicious. Let there be two namespaces, nsA and nsB, both with one dynamically created endpoint, nsA/ep1 (used by pv1) and nsB/ep2 (used by pv2). Now, when pv1 is deleted, heketi returns that there is still a volume (pv2) in the cluster and nsA/ep1 is not deleted. Subsequently, when pv2 is deleted, heketi cluster is empty and ep2 is deleted.

ep1 is never deleted. Who/when is supposed to delete it? You should delete all these stray endpoints in all namespaces.

Also, at the time you check len(clusterinfo.Volumes) below, there may already be a new volume just created by another provisioning goroutine.

IMO, the safest (and race free) way is to create an endpoint/service for each provisioned PV. When you delete a PV, you always delete associated endpoint + service.

It could be possible to share one EP + service in a namespace for all PVs, however you need to be extra careful not to delete it in corner cases when a new PV is being provisioned and some old PV is being deleted at the same time.

Thanks @jsafrane for the review. Reg# creating an ep/svc for each PV, It was discussed that, it may be a tedious approach. Reg# the stale EPs in the some of the Namespaces, even today once the admin create an endpoint in one or more NS, he has to manually delete the ep/svc if not required. I am open to take any path though :)

This way, endpoints/services are deleted from some namespaces, while they are kept in others. It's IMO very bad user experience, it should behave consistently and it should be well documented.

jsafrane · 2016-10-19T13:13:02Z

There are minor issues I can live with (logging, claimref check), however this PR will most probably create endpoints in user namespaces that nobody deletes when they are not needed. I'd rather have it done right instead of patching it later.

jsafrane · 2016-10-19T16:48:09Z

@k8s-bot test this issue: #IGNORE

k8s-ci-robot · 2016-10-19T17:02:55Z

Jenkins unit/integration failed for commit 5a60a5da2005fcc854210caa28eee5cd22db2de9. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jsafrane · 2016-10-19T17:34:24Z

@k8s-bot test this issue: #IGNORE

k8s-ci-robot · 2016-10-19T18:40:44Z

Jenkins GCE etcd3 e2e failed for commit 7f9faef. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jsafrane · 2016-10-19T18:46:58Z

pkg/volume/glusterfs/glusterfs.go

+		glog.Errorf("glusterfs: failed to delete service %s in namespace %s error %v", epServiceName, namespace, err)
+		return fmt.Errorf("error deleting service %v in namespace [%s]", err, namespace)
+	}
+	glog.V(1).Infof("glusterfs: service/endpoint [%s] in namespace [%s] deleted successfully", epServiceName, namespace)


shouldn't we delete also the endpoint here? I know, it may be garbage collected automatically, however we created it, we should delete it.

@jsafrane at earlier patch set, I deleted ep followed with svc deletion. Then @rootfs suggested to remove service first. If we do so, endpoint is gone at the same moment. so only deleting svc now.

@humblec @jsafrane removing service first can survive a sudden crash without leaking endpoint, since hanging endpoint is going to be cleaned up. But I would suggest removing endpoint as well - in case next time the endpoint is to be created and provisioner hit the line and prematurely exit without creating service.

_, err = p.plugin.host.GetKubeClient().Core().Endpoints(namespace).Create(endpoint) if err != nil && errors.IsAlreadyExists(err)

@rootfs if we delete the svc first, the ep is gone at the same moment and the very next line which is delete endpoint give error saying non existence of ep. so I kept only svc deletion. Also when we delete svc from commandline, the ep is deleted internally. afaict, we only need to delete svc.

jsafrane · 2016-10-19T18:47:24Z

pkg/volume/glusterfs/glusterfs.go

+		return nil, nil, fmt.Errorf("error creating endpoint %v", err)
+	}
+	service = &api.Service{
+		ObjectMeta: api.ObjectMeta{Name: epServiceName, Namespace: namespace},


please add gluster.kubernetes.io/provisioned-for-pvc label to Service

jsafrane · 2016-10-19T18:48:04Z

pkg/volume/glusterfs/glusterfs.go

+		return nil, 0, fmt.Errorf("no hosts found %v", err)
+	}
+
+	// The 'endpointname' is created in form of 'cluster-<id>' where 'id' is the cluster id.


cluster-<id> is no longer used

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

jsafrane · 2016-10-19T19:03:27Z

@k8s-bot test this issue: #IGNORE

jsafrane · 2016-10-19T19:07:15Z

lgtm

k8s-ci-robot · 2016-10-19T20:09:48Z

Jenkins GCI GKE smoke e2e failed for commit 0d080f9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-10-20T08:28:31Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 13, 2016

humblec force-pushed the gluster-pvc-namespace-1 branch 2 times, most recently from 091489c to 0fc1655 Compare October 13, 2016 11:20

k8s-github-robot assigned rootfs Oct 13, 2016

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

humblec force-pushed the gluster-pvc-namespace-1 branch from 0fc1655 to 4128f74 Compare October 13, 2016 11:31

rootfs reviewed Oct 13, 2016

View reviewed changes

humblec force-pushed the gluster-pvc-namespace-1 branch from 4128f74 to 61917ff Compare October 14, 2016 06:11

rootfs reviewed Oct 17, 2016

View reviewed changes

humblec force-pushed the gluster-pvc-namespace-1 branch from 61917ff to b5733fb Compare October 18, 2016 17:46

humblec force-pushed the gluster-pvc-namespace-1 branch from b5733fb to da4f713 Compare October 19, 2016 08:54

jsafrane reviewed Oct 19, 2016

View reviewed changes

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

humblec force-pushed the gluster-pvc-namespace-1 branch from da4f713 to 5a60a5d Compare October 19, 2016 16:35

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2016

humblec force-pushed the gluster-pvc-namespace-1 branch from 5a60a5d to 7f9faef Compare October 19, 2016 17:29

jsafrane added sig/storage Categorizes an issue or PR as relevant to SIG Storage. team/cluster release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 19, 2016

jsafrane reviewed Oct 19, 2016

View reviewed changes

Use PVC namespace when provisioning GlusterFS volumes.

0d080f9

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

humblec force-pushed the gluster-pvc-namespace-1 branch from 7f9faef to 0d080f9 Compare October 19, 2016 19:02

jsafrane added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2016

k8s-github-robot merged commit ed60ee4 into kubernetes:master Oct 20, 2016

humblec mentioned this pull request Nov 17, 2016

Introduce endpointnamespace in glustervolumesource #33020

Closed

This was referenced Dec 15, 2016

Endpoint changes with k8s 1.5 gluster/gluster-kubernetes#87

Closed

Endpoint changes with k8s 1.5 heketi/heketi#628

Closed

Makeuse of PVC namespace when provisioning gluster volumes. #34705

Makeuse of PVC namespace when provisioning gluster volumes. #34705

Conversation

humblec commented Oct 13, 2016 • edited by k8s-oncall

k8s-ci-robot commented Oct 13, 2016

humblec commented Oct 13, 2016

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

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

rootfs commented Oct 13, 2016

rootfs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Oct 17, 2016 • edited

humblec commented Oct 18, 2016 • edited

rootfs commented Oct 18, 2016

childsb commented Oct 18, 2016

humblec commented Oct 18, 2016 • edited

gnufied commented Oct 18, 2016

humblec commented Oct 18, 2016

childsb commented Oct 18, 2016

humblec commented Oct 19, 2016 • edited

humblec commented Oct 19, 2016

jsafrane commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

jsafrane commented Oct 19, 2016

humblec commented Oct 19, 2016

humblec commented Oct 19, 2016

rootfs commented Oct 19, 2016

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

jsafrane Oct 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Oct 19, 2016

jsafrane commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

jsafrane commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

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 Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Oct 19, 2016

jsafrane commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-github-robot commented Oct 20, 2016

humblec commented Oct 13, 2016 •

edited by k8s-oncall

rootfs commented Oct 17, 2016 •

edited

humblec commented Oct 18, 2016 •

edited

humblec commented Oct 18, 2016 •

edited

humblec commented Oct 19, 2016 •

edited

jsafrane Oct 14, 2016 •

edited

humblec Oct 19, 2016 •

edited