🐛 KCP should defer remediation when a control plane machine is still provisioning #9734
Conversation
k8s-ci-robot
added
area/control-plane
|
cc @sbueringer |
| func (c *ControlPlane) HasHealthyMachineStillProvisioning() bool { | ||
| return len(c.Machines.Filter(collections.Not(collections.HasUnhealthyCondition), collections.And(collections.Not(collections.HasNode())))) > 0 |
There was a problem hiding this comment.
Just to confirm, this is checking if any of the machines available don't have a node associated AND are not marked as unhealthy (yet)?
There was a problem hiding this comment.
Yes
The idea is that we want to wait for all the machines to be provisioned before remediation, but we should preserve the capability to remediate a machine that fails to provision
There was a problem hiding this comment.
Improved the filter and added unit tests
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: e39a29b97024ffb55dbd0b8f9171e4bb509f4e28
|
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a92f6d44d7b9c0cafd8241663442cba9e6d8e29a
|
|
/test pull-cluster-api-e2e-full-main |
| t.Run("Remediation deletes unhealthy machine failed to provision - 4 CP (during 3 CP rolling upgrade)", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| m1 := createMachine(ctx, g, ns.Name, "m1-unhealthy-", withMachineHealthCheckFailed(), withWaitBeforeDeleteFinalizer(), withoutNodeRef()) |
There was a problem hiding this comment.
just to clarify, does this withoutNodeRef() makes any actual difference from the test above "Remediation deletes unhealthy machine - 4 CP (during 3 CP rolling upgrade)"?
I assume they both would run the same code path because of withMachineHealthCheckFailed. I can still see the value of the test case, but just wanted to clarify that's the case.
There was a problem hiding this comment.
Yes, they are testing the same code path.
However, considering that this PR defers remediation in case a healthy machine is still provisioning, I figured out that having a test preventing any regression when a machine is still provisioning (withoutNodeRef), but unhealty, could help.
k8s-ci-robot
removed
the
lgtm
|
Thx!! /lgtm merge/approve pending squash |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 75959fb18b3255faeae3ab13540c7b013a225a0c
|
fabriziopandini
force-pushed
the
defer-remediation-when-provisioning
branch
from
a2e25e8 to
2cb95f7
Compare
k8s-ci-robot
removed
the
lgtm
|
rebased + squashed |
|
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
fabriziopandini
force-pushed
the
defer-remediation-when-provisioning
branch
from
2cb95f7 to
34c09b4
Compare
|
Thank you! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 8b2d3fd3f51ed46ce9495c7b02dc06aea85a4e7a
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
With this PR KCP defers remediation when another control plane machine is still provisioning
Which issue(s) this PR fixes:
Fixes #9398
/area control-plane