Don't delete subns if anchor has conflict #1150
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @yiqigao217 |
k8s-ci-robot
added
size/M
|
Wouldn't hurt to create issues and link them in the TODOs but otherwise lgtm. /lgtm feel free to remove hold if you don't plan to link issues. |
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, rjbez17 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 |
See issue kubernetes-retired#1149. This affects the ability to turn a subnamespace into a full namespace. If you remove the subnamespaceOf annotation, move the namespace to a new parent, and *then* clean up the anchor, everything's ok. But if you delete the anchor _before_ moving the namespace, the namespace gets deleted, which is clearly wrong. This change is the minimal safe change needed to fix this in v0.5. In v0.6, we should restructure these functions to be more heavily based on the explicit state of the anchor, not the implied state. Tested: new e2e test fails (hangs, actually) without this change and passes with it. All e2e tests pass on GKE 1.17.
adrianludwin
force-pushed
the
subns-conflict
branch
from
815d9a5 to
d6d4a2d
Compare
|
The change was just to fix the format of a log message |
The "conflict" case is a change - that's the bug I'm trying to fix. The "missing" case shouldn't be a change - we've just verified that the namespace is truly missing, so there's nothing to delete. |
This change fixes todos in the cherrypick for kubernetes-retired#1150 (see issue kubernetes-retired#1149). It simplifies and restructures a lot of the logic to make it easier to follow while looking at less data (e.g. a lot more focus on anchor.Status.State). Tested: all e2e tests pass on GKE 1.17.
Discussed offline, I was confused with allowing user to delete anchors when it has /lgtm |
This change fixes todos in the cherrypick for kubernetes-retired#1150 (see issue kubernetes-retired#1149). It simplifies and restructures a lot of the logic to make it easier to follow while looking at less data (e.g. a lot more focus on anchor.Status.State). Tested: all e2e tests pass on GKE 1.17.
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
This change fixes todos in the cherrypick for kubernetes-retired#1150 (see issue kubernetes-retired#1149). It simplifies and restructures a lot of the logic to make it easier to follow while looking at less data (e.g. a lot more focus on anchor.Status.State). It also adds a lots more documentation. Tested: all e2e tests pass on GKE 1.18 when combined with fixes to the e2e tests (PRs kubernetes-retired#1160, kubernetes-retired#1162, kubernetes-retired#1163 and kubernetes-retired#1164).
This change fixes todos in the cherrypick for kubernetes-retired#1150 (see issue kubernetes-retired#1149). It simplifies and restructures a lot of the logic to make it easier to follow while looking at less data (e.g. a lot more focus on anchor.Status.State). It also adds a lots more documentation. Tested: all e2e tests pass on GKE 1.18 when combined with fixes to the e2e tests (PRs kubernetes-retired#1160, kubernetes-retired#1162, kubernetes-retired#1163 and kubernetes-retired#1164).
See issue #1149. This affects the ability to turn a subnamespace into a
full namespace. If you remove the subnamespaceOf annotation, move the
namespace to a new parent, and then clean up the anchor, everything's
ok. But if you delete the anchor before moving the namespace, the
namespace gets deleted, which is clearly wrong.
This change is the minimal safe change needed to fix this in v0.5. In
v0.6, we should restructure these functions to be more heavily based on
the explicit state of the anchor, not the implied state.
Tested: new e2e test fails (hangs, actually) without this change and
passes with it. All e2e tests pass on GKE 1.17.