Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor GlusterFS PV spec. #60195

Merged
merged 1 commit into from Nov 8, 2018
Merged

Refactor GlusterFS PV spec. #60195

merged 1 commit into from Nov 8, 2018

Conversation

humblec
Copy link
Contributor

@humblec humblec commented Feb 22, 2018 •
edited by liggitt 

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
Copy link
Contributor

@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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 22, 2018
@k8s-ci-robot 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 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 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 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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2018
@humblec
Copy link
Contributor Author

humblec commented Feb 26, 2018

@rootfs

@k8s-ci-robot 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 humblec changed the title [WIP] Refactor GlusterFS PV spec. Refactor GlusterFS PV spec. Apr 24, 2018
@k8s-ci-robot 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
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2018
@humblec
Copy link
Contributor Author

humblec commented Oct 29, 2018

/retest

@humblec
Copy link
Contributor Author

humblec commented Nov 5, 2018

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

@humblec
Copy link
Contributor Author

humblec commented Nov 5, 2018

/retest

liggitt
liggitt reviewed Nov 5, 2018
View reviewed changes
vendordiff.patch Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Nov 5, 2018

API lgtm, go ahead and squash in the review commits

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

@liggitt
Copy link
Member

liggitt commented Nov 5, 2018

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
msau42 reviewed Nov 5, 2018
View reviewed changes
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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@liggitt liggitt Nov 5, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@humblec
Refactor GlusterFS PV spec.
bdb051c 
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
Copy link
Contributor Author

humblec commented Nov 6, 2018

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

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

@humblec
Copy link
Contributor Author

humblec commented Nov 6, 2018

/retest

@liggitt 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
@liggitt
Copy link
Member

liggitt commented Nov 6, 2018

/lgtm
/approve

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

@k8s-ci-robot 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
@msau42
Copy link
Member

msau42 commented Nov 7, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

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

liggitt commented Nov 8, 2018

/hold cancel

@k8s-ci-robot 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 k8s-ci-robot merged commit 3c10143 into kubernetes:master Nov 8, 2018
@humblec
Copy link
Contributor Author

humblec commented Nov 8, 2018

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 !!

humblec added a commit to humblec/kubernetes that referenced this pull request Nov 9, 2018
@humblec
Glusterfs: Return spec.pv.spec.glusterfs.readonly
d0fdf9d 
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>
This was referenced Nov 9, 2018
Closed
Merged
humblec added a commit to humblec/kubernetes that referenced this pull request Nov 9, 2018
@humblec
GetVolumeName() is used in a few places, all of which are not applica…
e9ed219 
…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>
@sigma sigma mentioned this pull request Dec 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@roycaihw roycaihw roycaihw left review comments

@rootfs rootfs rootfs left review comments

@msau42 msau42 msau42 left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@jsafrane jsafrane Awaiting requested review from jsafrane

@krousey krousey Awaiting requested review from krousey

@shiywang shiywang Awaiting requested review from shiywang

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@liggitt liggitt

@jsafrane jsafrane

@rootfs rootfs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@humblec @k8s-ci-robot @rootfs @techtonik @ramyala @liggitt @msau42 @roycaihw @jsafrane @k8s-github-robot