Refactor GlusterFS PV spec. #60195

humblec · 2018-02-22T09:19:54Z

Refactor GlusterFS PV spec.

This patch introduces glusterfsPersistentVolumeSource addition
to glusterfsVolumeSource. All fields remains same as glusterfsVolumeSource
with an addition of a new field
called `EndpointsNamespace` to define namespace of endpoint in the
spec.

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

Special notes for your reviewer:

Release note:

GlusterFS PersistentVolumes sources can now reference endpoints in any namespace using the `spec.glusterfs.endpointsNamespace` field. Ensure all kubelets are upgraded to 1.13+ before using this capability.

k8s-ci-robot · 2018-02-22T09:19:55Z

@humblec: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

humblec · 2018-02-26T16:23:22Z

@rootfs

humblec · 2018-10-29T17:07:17Z

/retest

humblec · 2018-11-05T15:54:18Z

@LiGgit hopefully last commit has addressed return from GetVolumeName() as suggested. PTAL .. Thanks !!

humblec · 2018-11-05T15:57:25Z

/retest

vendordiff.patch

liggitt · 2018-11-05T18:07:55Z

API lgtm, go ahead and squash in the review commits

needs ack from @kubernetes/sig-storage-pr-reviews on the GetVolumeName() change

liggitt · 2018-11-05T21:08:05Z

linting error as well:

pkg/volume/glusterfs/glusterfs.go:125:10: if block ends with a return statement, so drop this else and outdent its block

msau42 · 2018-11-05T21:16:33Z

staging/src/k8s.io/api/core/v1/types.go

@@ -191,7 +191,7 @@ type PersistentVolumeSource struct {
 	// exposed to the pod. Provisioned by an admin.
 	// More info: https://releases.k8s.io/HEAD/examples/volumes/glusterfs/README.md
 	// +optional
-	Glusterfs *GlusterfsVolumeSource `json:"glusterfs,omitempty" protobuf:"bytes,4,opt,name=glusterfs"`
+	Glusterfs *GlusterfsPersistentVolumeSource `json:"glusterfs,omitempty" protobuf:"bytes,4,opt,name=glusterfs"`


Sorry I'm late to the party. Is this a backwards compatible change? Will clients that programatically interact with the API start failing because the type has changed?

It is backwards compatible in the dimensions we guarantee.

The type of the go field does not impact what gets serialized to the wire, so the type change does not affect wire compatibility (against the API or against etcd).

The type of the go struct field is changing. Depending on how a client made use of this object, it might have to adjust how it constructed a struct of this type. If all it did was retrieve objects from the API and access fields (e.g. pv.Spec.Glusterfs.EndpointsName) no change would be required. Go struct compatibility is not currently guaranteed release-to-release.

Ah ok, so if someone synced their go-client to the latest, then it's possible they will have to recompile, but older applications using the old clients will continue to work. Is this something we need to call out in any release notes?

yes, worth a release note, both for the new capability, and for the client-go impact

This patch introduces glusterfsPersistentVolumeSource addition to glusterfsVolumeSource. All fields remains same as glusterfsVolumeSource with an addition of a new field called `EndpointsNamespace` to define namespace of endpoint in the spec. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

humblec · 2018-11-06T11:47:17Z

API lgtm, go ahead and squash in the review commits>

Thanks a lot @liggitt ! The commits are squashed and test are green :)..

humblec · 2018-11-06T12:49:42Z

/retest

liggitt · 2018-11-06T14:58:43Z

/lgtm
/approve

/hold
for sig-storage approval of GetVolumeName() change. unhold at will once that is acked.

msau42 · 2018-11-07T21:03:25Z

/approve

k8s-ci-robot · 2018-11-07T21:04:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, liggitt, msau42

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:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/core/OWNERS~~ [liggitt]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [msau42]
~~pkg/printers/OWNERS~~ [liggitt]
~~pkg/volume/glusterfs/OWNERS~~ [humblec]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/e2e/storage/OWNERS~~ [liggitt,msau42]

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

liggitt · 2018-11-08T07:12:51Z

/hold cancel

humblec · 2018-11-08T16:27:23Z

Thanks a lot @liggitt for detailed review ( as always) and quick revert on updated patchsets on this PR!! Thanks @msau42 @rootfs for approval and other thoughts !!

Before it only checked spec.ReadOnly, but now it checks spec.pv.spec.glusterfs.readonly flag. This patch also correct a bug in old code where it returned mounter true even when pv is false. This is a follow up PR based on comment# kubernetes#60195 (comment) Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

…ble to gluster: AD controller kubelet volume manager actual/desired sw for attachable volumes volume reconstruction This is a followup PR based on comment# kubernetes#60195 (comment) Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

k8s-ci-robot requested review from dchen1107, jsafrane, krousey, shiywang and smarterclayton February 22, 2018 09:20

humblec force-pushed the glusterfs-pvspec-3 branch from 7131438 to 294cff7 Compare February 22, 2018 09:40

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

humblec force-pushed the glusterfs-pvspec-3 branch 2 times, most recently from 89f674b to 37d535f Compare February 26, 2018 06:29

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 26, 2018

humblec force-pushed the glusterfs-pvspec-3 branch 5 times, most recently from 04ba51f to c7229d5 Compare February 26, 2018 07:53

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

humblec force-pushed the glusterfs-pvspec-3 branch from 72c1962 to bef2bd7 Compare February 26, 2018 10:06

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

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

humblec changed the title ~~[WIP] Refactor GlusterFS PV spec.~~ Refactor GlusterFS PV spec. Apr 24, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2018

humblec force-pushed the glusterfs-pvspec-3 branch from bef2bd7 to b015786 Compare April 26, 2018 09:27

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

liggitt reviewed Nov 5, 2018

View reviewed changes

vendordiff.patch Outdated Show resolved Hide resolved

msau42 reviewed Nov 5, 2018

View reviewed changes

humblec force-pushed the glusterfs-pvspec-3 branch from 97de0a9 to 6d41f24 Compare November 6, 2018 09:46

humblec force-pushed the glusterfs-pvspec-3 branch from 6d41f24 to bdb051c Compare November 6, 2018 10:23

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Nov 6, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2018

k8s-ci-robot merged commit 3c10143 into kubernetes:master Nov 8, 2018

This was referenced Nov 9, 2018

Glusterfs: Return spec.pv.spec.glusterfs.readonly #70859

Closed

GlusterFS: Move GetVolumeName() to unimplemented func. #70860

Merged

sigma mentioned this pull request Dec 27, 2018

adjust to newer k8s APIs heketi/heketi#1272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GlusterFS PV spec. #60195

Refactor GlusterFS PV spec. #60195

humblec commented Feb 22, 2018 •

edited by liggitt

Loading

k8s-ci-robot commented Feb 22, 2018

humblec commented Feb 26, 2018

humblec commented Oct 29, 2018

humblec commented Nov 5, 2018

humblec commented Nov 5, 2018

liggitt commented Nov 5, 2018

liggitt commented Nov 5, 2018

msau42 Nov 5, 2018

liggitt Nov 5, 2018 •

edited

Loading

msau42 Nov 5, 2018

liggitt Nov 5, 2018

humblec commented Nov 6, 2018

humblec commented Nov 6, 2018

liggitt commented Nov 6, 2018

msau42 commented Nov 7, 2018

k8s-ci-robot commented Nov 7, 2018

liggitt commented Nov 8, 2018

humblec commented Nov 8, 2018

Refactor GlusterFS PV spec. #60195

Refactor GlusterFS PV spec. #60195

Conversation

humblec commented Feb 22, 2018 • edited by liggitt Loading

k8s-ci-robot commented Feb 22, 2018

humblec commented Feb 26, 2018

humblec commented Oct 29, 2018

humblec commented Nov 5, 2018

humblec commented Nov 5, 2018

liggitt commented Nov 5, 2018

liggitt commented Nov 5, 2018

msau42 Nov 5, 2018

Choose a reason for hiding this comment

liggitt Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

msau42 Nov 5, 2018

Choose a reason for hiding this comment

liggitt Nov 5, 2018

Choose a reason for hiding this comment

humblec commented Nov 6, 2018

humblec commented Nov 6, 2018

liggitt commented Nov 6, 2018

msau42 commented Nov 7, 2018

k8s-ci-robot commented Nov 7, 2018

liggitt commented Nov 8, 2018

humblec commented Nov 8, 2018

humblec commented Feb 22, 2018 •

edited by liggitt

Loading

liggitt Nov 5, 2018 •

edited

Loading