Use pvc uid instead of pvc name in glusterfs endpoint name. #70286

humblec · 2018-10-26T13:10:19Z

GlusterFS: At present, endpoints and services are created
for glusterfs pvcs are in form of glusterfs-dynamic-
however this could cause issue if user deletes a PVC and immediately
creates a new one with the same name, PV controller will try to delete
the old PV and its endpoint and at the same the controller will try to create new PV
and the same endpoint. Depending on which event reaches the
controller first, it may create new PV, check that endpoints exists,
then delete the old PVC and delete endpoints already used by the new PV.

This patch changes the endpoint/service name to below format:

glusterfs-dynamic-<PVC UUID>.

By the uniqueness of UUID, above mentioned issue will no longer be present.

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

humblec · 2018-10-26T13:10:44Z

/release-note-none

humblec · 2018-10-26T13:11:25Z

/assign @jsafrane

humblec · 2018-10-26T13:27:33Z

@obnoxxx as discussed in pr #69419 .

obnoxxx

Thanks Humble, the change looks mostly good, just small comments inline.

And the commit message needs improvement:
Generally a good commit message should be in a format of:

GlusterFS: short 1-line summary of what the patch does

optionally,
more lines explaining
the patch

Signed-off-by: ....

So important is: a blank line after initial line. And don't start with the explanation in the initial line but with what the patch does.

obnoxxx · 2018-10-26T13:59:39Z

pkg/volume/glusterfs/glusterfs.go

@@ -822,11 +822,11 @@ func (p *glusterfsVolumeProvisioner) CreateVolume(gid int) (r *v1.GlusterfsVolum
 		return nil, 0, "", fmt.Errorf("failed to get cluster nodes for volume %s: %v", volume, err)
 	}

-	// The 'endpointname' is created in form of 'glusterfs-dynamic-<claimname>'.
+	// The 'endpointname' is created in form of 'glusterfs-dynamic-<PVC UUID>'.


it's UID not UUID

obnoxxx · 2018-10-26T14:00:19Z

pkg/volume/glusterfs/glusterfs.go

 	// createEndpointService() checks for this 'endpoint' existence in PVC's namespace and
-	// If not found, it create an endpoint and service using the IPs we dynamically picked at time
+	// if not found, it create an endpoint and service using the IPs we dynamically picked at time


This is correct but actually not related to the topic of this patch.
So that comment fix might be a separate patch...

Put it as a seperate commit.

Thanks @humblec . I noticed there is another error in the line: it create --> it creates. Maybe we just skip this hunk from the PR and consider the separate PR #70306 for comment cleanup?

At present, endpoints and services are created for glusterfs pvcs are in form of glusterfs-dynamic-<PVC name> however this could cause issue if user deletes a PVC and immediately creates a new one with the same name, PV controller will try to delete the old PV and its endpoint and at the same the controller will try to create new PV and the same endpoint. Depending on which event reaches the controller first, it may create new PV, check that endpoints exists, then delete the old PVC and delete endpoints already used by the new PV. This patch changes the endpoint/service name to below format: `glusterfs-dynamic-<PVC UUID>`. By the uniqueness of UUID, above mentioned issue will no longer be present. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

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

humblec · 2018-10-26T14:33:27Z

So important is: a blank line after initial line. And don't start with the explanation in the initial line but with what the patch does.

@obnoxxx It was there in the PR title, however forgot in the commit message. Addressed it latest patch set. PTAL. Thanks.

humblec · 2018-10-26T14:36:10Z

/retest

obnoxxx

LGTM, thanks!

humblec · 2018-10-26T15:09:59Z

/retest

jsafrane · 2018-10-26T17:17:11Z

lgtm-ish, please squash the patches please.

obnoxxx · 2018-10-26T21:07:50Z

@jsafrane writes:

lgtm-ish, please squash the patches please.

Why is that? I explicitly asked @humblec to split the patches because the comment-fix is not part of the topic of the other patch. If you prefer to have that in a different PR then, I'd rather have a second PR than polluting commits with unrelated changes.

obnoxxx

spotted one more typo in the comment, that you are fixing, and was remembered of my patches previously posted in #68343 to clean up the comments, now resurrected in #70306..

obnoxxx · 2018-10-26T21:28:41Z

pkg/volume/glusterfs/glusterfs.go

 	// createEndpointService() checks for this 'endpoint' existence in PVC's namespace and
-	// If not found, it create an endpoint and service using the IPs we dynamically picked at time
+	// if not found, it create an endpoint and service using the IPs we dynamically picked at time


Thanks @humblec . I noticed there is another error in the line: it create --> it creates. Maybe we just skip this hunk from the PR and consider the separate PR #70306 for comment cleanup?

jsafrane · 2018-10-29T09:50:48Z

/lgtm
/approve

k8s-ci-robot · 2018-10-29T09:50:57Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/volume/glusterfs/OWNERS~~ [humblec,jsafrane]

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

humblec · 2018-10-29T11:13:17Z

/kind bug

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 26, 2018

k8s-ci-robot requested review from jingxu97 and jsafrane October 26, 2018 13:10

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 26, 2018

k8s-ci-robot assigned jsafrane Oct 26, 2018

humblec mentioned this pull request Oct 26, 2018

glusterfs: Allow admin to enable or disable custom endpoint/service names #69419

Merged

obnoxxx suggested changes Oct 26, 2018

View reviewed changes

humblec force-pushed the pvc-uuid branch from 9e70f40 to 78055d1 Compare October 26, 2018 14:28

humblec added 2 commits October 26, 2018 20:00

Fix a typo in source code comment

a6759a7

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

humblec force-pushed the pvc-uuid branch from 78055d1 to a6759a7 Compare October 26, 2018 14:32

humblec changed the title ~~Use pvc uuid instead of pvc name in glusterfs endpoint name.~~ Use pvc uid instead of pvc name in glusterfs endpoint name. Oct 26, 2018

obnoxxx approved these changes Oct 26, 2018

View reviewed changes

obnoxxx mentioned this pull request Oct 26, 2018

Glusterfs: improve some comments #70306

Merged

obnoxxx suggested changes Oct 26, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 29, 2018

k8s-ci-robot merged commit eb1aa87 into kubernetes:master Oct 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pvc uid instead of pvc name in glusterfs endpoint name. #70286

Use pvc uid instead of pvc name in glusterfs endpoint name. #70286

humblec commented Oct 26, 2018 •

edited

humblec commented Oct 26, 2018

humblec commented Oct 26, 2018

humblec commented Oct 26, 2018

obnoxxx left a comment

obnoxxx Oct 26, 2018

obnoxxx Oct 26, 2018

humblec Oct 26, 2018

obnoxxx Oct 26, 2018

humblec commented Oct 26, 2018 •

edited

humblec commented Oct 26, 2018

obnoxxx left a comment

humblec commented Oct 26, 2018

jsafrane commented Oct 26, 2018

obnoxxx commented Oct 26, 2018

obnoxxx left a comment

obnoxxx Oct 26, 2018

jsafrane commented Oct 29, 2018

k8s-ci-robot commented Oct 29, 2018

humblec commented Oct 29, 2018

Use pvc uid instead of pvc name in glusterfs endpoint name. #70286

Use pvc uid instead of pvc name in glusterfs endpoint name. #70286

Conversation

humblec commented Oct 26, 2018 • edited

humblec commented Oct 26, 2018

humblec commented Oct 26, 2018

humblec commented Oct 26, 2018

obnoxxx left a comment

Choose a reason for hiding this comment

obnoxxx Oct 26, 2018

Choose a reason for hiding this comment

obnoxxx Oct 26, 2018

Choose a reason for hiding this comment

humblec Oct 26, 2018

Choose a reason for hiding this comment

obnoxxx Oct 26, 2018

Choose a reason for hiding this comment

humblec commented Oct 26, 2018 • edited

humblec commented Oct 26, 2018

obnoxxx left a comment

Choose a reason for hiding this comment

humblec commented Oct 26, 2018

jsafrane commented Oct 26, 2018

obnoxxx commented Oct 26, 2018

obnoxxx left a comment

Choose a reason for hiding this comment

obnoxxx Oct 26, 2018

Choose a reason for hiding this comment

jsafrane commented Oct 29, 2018

k8s-ci-robot commented Oct 29, 2018

humblec commented Oct 29, 2018

humblec commented Oct 26, 2018 •

edited

humblec commented Oct 26, 2018 •

edited