-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🌱 Drop usage of v1beta1 conditions #12109
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.
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
base: main
Are you sure you want to change the base?
🌱 Drop usage of v1beta1 conditions #12109
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test? |
@fabriziopandini: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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-sigs/prow repository. |
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-conformance-ci-latest-main |
2 similar comments
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-main |
/test pull-cluster-api-e2e-conformance-ci-latest-main |
8816570
to
5e3fd7f
Compare
/test pull-cluster-api-e2e-conformance-ci-latest-main |
5e3fd7f
to
754995b
Compare
/test pull-cluster-api-e2e-conformance-ci-latest-main |
…conditions management only)
# Conflicts: # internal/controllers/machine/machine_controller_test.go
…erv1.MachineHealthCheckSucceededV1Beta1Condition in controllers
…trolplanev1.ControlPlaneComponentsHealthyV1Beta1Condition in controllers
754995b
to
e06c3bc
Compare
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-latestk8s-main Looks like just a flake |
} | ||
} | ||
|
||
func withUnhealthyAPIServerPod() machineOption { | ||
return func(machine *clusterv1.Machine) { | ||
v1beta1conditions.MarkFalse(machine, controlplanev1.MachineAPIServerPodHealthyV1Beta1Condition, controlplanev1.ControlPlaneComponentsUnhealthyV1Beta1Reason, clusterv1.ConditionSeverityError, "") | ||
conditions.Set(machine, metav1.Condition{Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyCondition, Status: metav1.ConditionFalse, Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningReason}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "KubeadmControlPlaneMachinePodRunningReason" doesn't seem to make sense here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest, using another const name or change the const value to something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is false, so "Running" seems like the wrong reason :) (it should be some failed reason instead, I don't mind which one)
conditions.Set(machine, metav1.Condition{Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyCondition, Status: metav1.ConditionFalse, Reason: controlplanev1.KubeadmControlPlaneMachinePodFailedReason})
@@ -358,8 +358,7 @@ func (r *KubeadmConfigReconciler) reconcile(ctx context.Context, scope *Scope, c | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Audited for usages of the deprecated v1beta1 condition packages.
These are the results:
- test/e2e: 1x v1beta1conditions.IsFalse + 1x v1beta1conditions.Get directly below
- CAPD:
- 2x v1beta1conditions.GetReason
- 1x v1beta1conditions.Has
- a few times v1beta1conditions.IsTrue
- a few times v1beta1conditions.Get
I suspect CAPD is expected at this point. Can you please check that this remaining work is properly tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I got it right:
- test/e2e: 1x v1beta1conditions.IsFalse -> MachinePool 😢
- test/e2e: 1x v1beta1conditions.Get directly below -> It is a check for MS MachinesCreated condition, which doesn't have an equivalent in v1beta2
- I suspect CAPD is expected at this point -> Correct, it is tracked
@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-sigs/prow repository. I understand the commands that are listed here. |
What this PR does / why we need it:
Do not review, work in progress, I'm using this PR mostly to check everything looks good in CI but most probably I will later re-submit in a few smaller PR
Which issue(s) this PR fixes:
Fixes #
/area conditions