-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Refine v1beta2 KCP available condition #11451
🌱 Refine v1beta2 KCP available condition #11451
Conversation
} | ||
} | ||
} | ||
|
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 about if etcdIsManaged
?
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.
🤔 should we even report something then? should be something from the thing which manages it or not?
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.
We are reporting status for both etcd and control plane components in case etcd is managed
In case etcd is NOT managed we report only about control plane components
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.
I will make this more explicit, but the TLDR is that everything just works because etcdmember list is empty when etcd is not managed
ObjectMeta: metav1.ObjectMeta{Name: "m4"}, | ||
Status: clusterv1.MachineStatus{ | ||
NodeRef: nil, | ||
V1Beta2: &clusterv1.MachineV1Beta2Status{Conditions: []metav1.Condition{apiServerPodHealthy, controllerManagerPodHealthy, schedulerPodHealthy, etcdPodHealthy, etcdMemberHealthy}}, |
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.
We can cover this case if you want, but we will never set these conditions to true without a NodeRef. Usually these conditions will be unknown then (if I see correctly)
(at least a disclaimer here in the test that this will never happen would be good though to not mislead folks reading this test case)
Probably we should consider having a test case to cover the case without noderef and unknown conditions
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.
added a note with also the explanation why adding a use case with unknown conditions does not provide a good signal
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.
All good for this test case.
Would it be good to have an additional test case with the realistic case of a Machine without nodeRef and unknown conditions? (because this is something that actually happens, so it would be good to cover and validate what the resulting condition will be)
287ceb6
to
29afd3e
Compare
29afd3e
to
cf4145a
Compare
/lgtm |
LGTM label has been added. Git tree hash: e6f7fc8852972725872d609e7a6e35d99c1cd24f
|
Thx! /approve |
[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 |
What this PR does / why we need it:
Improve KCP available condition so:
Part of #11105
/area provider/control-plane-kubeadm