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

Follow up CSI resizing updates for 1.15 release #831

Merged
merged 2 commits into from
Apr 30, 2019
Merged

Follow up CSI resizing updates for 1.15 release #831

merged 2 commits into from
Apr 30, 2019

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Feb 13, 2019

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2019
@gnufied gnufied changed the title Fix some of the follow up comments Fix some of the follow up comments for CSI resizing. Feb 13, 2019
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/pm sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Feb 13, 2019
@gnufied
Copy link
Member Author

gnufied commented Feb 13, 2019

/sig storage

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 13, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 29, 2019
@gnufied
Copy link
Member Author

gnufied commented Apr 29, 2019

/assign @msau42

@gnufied
Copy link
Member Author

gnufied commented Apr 29, 2019

/kind api-change

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 29, 2019
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@msau42
Copy link
Member

msau42 commented Apr 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2019
@gnufied
Copy link
Member Author

gnufied commented Apr 29, 2019

/kind api-review

@liggitt
Copy link
Member

liggitt commented Apr 29, 2019

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Apr 29, 2019
@gnufied gnufied changed the title Fix some of the follow up comments for CSI resizing. Follow up CSI resizing updates for beta release Apr 29, 2019
saad-ali
saad-ali approved these changes Apr 29, 2019
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

Hold for API review, which should be fairly easy/straight forward. Please remove hold once API review is cleared.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 29, 2019
@gnufied
Copy link
Member Author

gnufied commented Apr 29, 2019 •
edited
Loading

For api review:

/assign @thockin @liggitt

@gnufied
Copy link
Member Author

gnufied commented Apr 30, 2019

/assign @thockin @liggitt

@liggitt
Copy link
Member

liggitt commented Apr 30, 2019

a few questions about the field name, the alpha/beta status, and about whether we need a NodeExpandSecretRef. Looks fine otherwise.

@liggitt
Copy link
Member

liggitt commented Apr 30, 2019

  • explicitly call out the new field will be alpha its first release
  • explicitly call out that we do not need a secret field for the NodeExpandVolume operation, and that the ControllerExpandSecretRef field will not be made accessible to the kubelet (via the node authorizer)

@gnufied gnufied changed the title Follow up CSI resizing updates for beta release Follow up CSI resizing updates for 1.15 release Apr 30, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019
@gnufied
Copy link
Member Author

gnufied commented Apr 30, 2019

@liggitt okay update the KEP to explicitly call out secrets not required from kubelet. Also noted that API change will remain in alpha. can you PTAL?

@msau42
Copy link
Member

msau42 commented Apr 30, 2019

I spoke to @davidz627 and @saad-ali and we are ok with keeping migration and resizing as alpha

liggitt
liggitt reviewed Apr 30, 2019
View reviewed changes
keps/sig-storage/20190129-csi-volume-resizing.md Outdated
}
```

Secrets will be fetched from StorageClass with parameters `csi.storage.k8s.io/controller-expansion-secret-name` and `csi.storage.k8s.io/controller-expansion-secret-namespace`. Resizing secrets will support same templating rules as attach and detach as documented - https://kubernetes-csi.github.io/docs/secrets-and-credentials.html#controller-publishunpublish-secret .
Copy link
Member

Choose a reason for hiding this comment

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

nit: csi.storage.k8s.io/controller-expand-secret-name, etc

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@liggitt
Copy link
Member

liggitt commented Apr 30, 2019

nit on csi parameter name, API changes lgtm otherwise

@liggitt liggitt removed the api-review Categorizes an issue or PR as actively needing an API review. label Apr 30, 2019
@gnufied
Update kep to keep CSI resizing in alpha in 1.15
2e9eadb 
Explicitly call out the fact that kubelet does not need secrets
@msau42
Copy link
Member

msau42 commented Apr 30, 2019

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 30, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali

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 merged commit 3b4fad5 into kubernetes:master Apr 30, 2019
@gnufied gnufied mentioned this pull request May 6, 2019
Merged
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

@msau42 msau42 msau42 left review comments

@saad-ali saad-ali saad-ali approved these changes

@childsb childsb Awaiting requested review from childsb

Assignees

@liggitt liggitt

@thockin thockin

@saad-ali saad-ali

@msau42 msau42

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 kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
API Reviews
Status: API review completed, 1.15
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gnufied @fejta-bot @msau42 @liggitt @k8s-ci-robot @saad-ali @justaugustus @thockin