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

Remove alpha annotation for volume node affinity #61816

Merged
merged 4 commits into from Mar 31, 2018
Merged

Remove alpha annotation for volume node affinity #61816

merged 4 commits into from Mar 31, 2018

Conversation

wackxu
Copy link
Contributor

@wackxu wackxu commented Mar 28, 2018 •
edited

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #61785

Special notes for your reviewer:
/assign @msau42

Release note:

ACTION REQUIRED: Alpha annotation for PersistentVolume node affinity has been removed.  Update your PersistentVolumes to use the beta PersistentVolume.nodeAffinity field before upgrading to this release

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 28, 2018
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 28, 2018
@hzxuzhonghu
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 28, 2018
msau42
msau42 reviewed Mar 28, 2018
View reviewed changes
pkg/apis/core/helper/helpers.go
// and converts it to the NodeAffinity type in core.
// TODO: update when storage node affinity graduates to beta
func GetStorageNodeAffinityFromAnnotation(annotations map[string]string) (*core.NodeAffinity, error) {
if len(annotations) > 0 && annotations[core.AlphaStorageNodeAffinityAnnotation] != "" {
Copy link
Member

Choose a reason for hiding this comment

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

core.AlphaStorageNodeAffinityAnnotation should also be removed from types.go (and you'll have to regenerate all the files too...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@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 Mar 29, 2018
@msau42
Copy link
Member

msau42 commented Mar 29, 2018

Did you also run make update for the generated files?

@wackxu
Copy link
Contributor Author

wackxu commented Mar 29, 2018

@msau42 Yes, but nothing changed

@msau42
Copy link
Member

msau42 commented Mar 30, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2018
msau42
msau42 reviewed Mar 30, 2018
View reviewed changes
pkg/scheduler/factory/factory.go
return
}

oldAffinity := oldPV.Spec.NodeAffinity
Copy link
Member

Choose a reason for hiding this comment

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

Technically this isn't needed because the beta NodeAffinity is immutable. But if we decide in the future to make it mutable, then we won't forget to update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I think I can remove this next pr.

@msau42
Copy link
Member

msau42 commented Mar 30, 2018

Can you change the release note to:

ACTION REQUIRED: Alpha annotation for PersistentVolume node affinity has been removed.  Update your PersistentVolumes to use the beta PersistentVolume.nodeAffinity field before upgrading to this release

@msau42
Copy link
Member

msau42 commented Mar 30, 2018

/assign @thockin

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 30, 2018
@thockin
Copy link
Member

thockin commented Mar 30, 2018

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, thockin, wackxu

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 Mar 30, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 54997, 61869, 61816, 61909, 60525). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit ac8a02f into kubernetes:master Mar 31, 2018
@wackxu wackxu deleted the rman branch March 31, 2018 03:44
@wackxu wackxu mentioned this pull request Mar 31, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Apr 5, 2018
Merge pull request #61974 from wackxu/rmlan
aaed11f 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

remove pvc node affinity update check since beta NodeAffinity is immu…

…table



**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:

xref #61816 (comment)

**Special notes for your reviewer**:
/assign @msau42 

**Release note**:

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@eparis eparis Awaiting requested review from eparis

@rootfs rootfs Awaiting requested review from rootfs

@thockin thockin Awaiting requested review from thockin

@yujuhong yujuhong Awaiting requested review from yujuhong

Assignees

@thockin thockin

@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 lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. 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
6 participants
@wackxu @hzxuzhonghu @msau42 @thockin @k8s-ci-robot @k8s-github-robot