Skip to content

Remove PV/PVC finalizers on downgrade from >= 1.10 to < 1.10#61299

Closed
liggitt wants to merge 1 commit intokubernetes:release-1.9from
liggitt:pvc-downgrade
Closed

Remove PV/PVC finalizers on downgrade from >= 1.10 to < 1.10#61299
liggitt wants to merge 1 commit intokubernetes:release-1.9from
liggitt:pvc-downgrade

Conversation

@liggitt
Copy link
Copy Markdown
Member

@liggitt liggitt commented Mar 16, 2018 •
edited
Loading

Codifies the need to remove storage protection finalizers on downgrade from >= 1.10 to < 1.10

See kubernetes/website#7731 and #60764 (comment)

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 16, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-github-robot k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Mar 16, 2018
@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 16, 2018
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

/sig storage
cc @kubernetes/sig-storage-test-failures @kubernetes/sig-release-test-failures

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Mar 16, 2018
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

/release-note-none

@k8s-ci-robot 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 Mar 16, 2018
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

/assign @saad-ali @rootfs @jpbetz

rootfs
rootfs reviewed Mar 16, 2018
View reviewed changes
test/e2e/lifecycle/cluster_upgrade.go Outdated
Copy link
Copy Markdown
Contributor

@rootfs rootfs Mar 16, 2018

Choose a reason for hiding this comment

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

no pvc namespace?

Copy link
Copy Markdown
Member Author

@liggitt liggitt Mar 16, 2018

Choose a reason for hiding this comment

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

correct, list across all namespaces

Copy link
Copy Markdown
Contributor

@krousey krousey Mar 16, 2018

Choose a reason for hiding this comment

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

metav1.NamespaceAll would be more clear

@jpbetz jpbetz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Mar 16, 2018
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

/retest

@verult
Copy link
Copy Markdown
Contributor

verult commented Mar 16, 2018

/assign @krousey
for downgrade test review

krousey
krousey suggested changes Mar 16, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@krousey krousey left a comment

Choose a reason for hiding this comment

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

Is there an issue created tracking the proper fix? If so, this PR should be tagged there so they know exactly what to rollback when they fix it.

test/e2e/lifecycle/cluster_upgrade.go Outdated
Copy link
Copy Markdown
Contributor

@krousey krousey Mar 16, 2018

Choose a reason for hiding this comment

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

metav1.NamespaceAll would be more clear

@liggitt liggitt mentioned this pull request Mar 16, 2018
Closed
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

Is there an issue created tracking the proper fix?

tagged in #61282

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 16, 2018

metav1.NamespaceAll would be more clear

updated

@krousey
Copy link
Copy Markdown
Contributor

krousey commented Mar 16, 2018

LGTM from me. Leaving it to branch/release team to merge.

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 17, 2018

@saad-ali @childsb for lgtm

@liggitt liggitt added this to the v1.9 milestone Mar 17, 2018
@jpbetz
Copy link
Copy Markdown
Contributor

jpbetz commented Mar 18, 2018

/hold
Hold all unmerged cherrypicks for cut of patch release on Monday.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2018
@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Mar 19, 2018

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@jpbetz @krousey @liggitt @rootfs @saad-ali

Action required: This pull request requires label changes. If the required changes are not made within 1 day, the pull request will be moved out of the v1.9 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Mar 19, 2018

discussed in sig-release call

as @krousey pointed out, the PVC cleanup failure in the downgrade tests did not block running of the remaining downgrade tests, which means the single failure in the downgrade test (namespace cleanup) is well-understood and not masking other failures.

given that we plan to resolve this finalizer issue in the 1.9 stream (in #61282 and #61316) and would need to revert this as part of that, I think it is better to just close this.

@liggitt liggitt closed this Mar 19, 2018
@liggitt liggitt deleted the pvc-downgrade branch April 22, 2019 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erictune erictune Awaiting requested review from erictune

@zmerlynn zmerlynn Awaiting requested review from zmerlynn

2 more reviewers

@rootfs rootfs rootfs left review comments

@krousey krousey krousey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@krousey krousey

@jpbetz jpbetz

@rootfs rootfs

@saad-ali saad-ali

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. milestone/incomplete-labels release-note-none Denotes a PR that doesn't merit a release note. sig/release Categorizes an issue or PR as relevant to SIG Release. 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

None yet

Milestone

v1.9

Development

Successfully merging this pull request may close these issues.

8 participants

@liggitt @k8s-ci-robot @verult @krousey @jpbetz @k8s-github-robot @rootfs @saad-ali