✨ Add v1beta2 condition to Cluster controller #11364
Conversation
fabriziopandini
added
the
area/provider/core
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
| // ClusterInfrastructureDeletedV1Beta2Reason surfaces when a referenced infrastructure object has been deleted. | ||
| // Note: controllers can't identify if the infrastructure object was deleted by the controller itself, e.g. | ||
| // during the deletion workflow, or by a users. | ||
| ClusterInfrastructureDeletedV1Beta2Reason = ObjectDeletedV1Beta2Reason |
There was a problem hiding this comment.
how is this semantically different from the one above?
There was a problem hiding this comment.
We use "Deleted" when we know for certain that the object existed at some point, we use "DoesNotExists" when we don't know if it ever existed
| ClusterControlPlaneInitializedV1Beta2Condition = "ControlPlaneInitialized" | ||
|
|
||
| // ClusterControlPlaneInitializedV1Beta2Reason surfaces when the cluster control plane is initialized. | ||
| ClusterControlPlaneInitializedV1Beta2Reason = "Initialized" |
There was a problem hiding this comment.
have we discussed using "AsExpected" for the happy path?
There was a problem hiding this comment.
As far as I remember, no.
I don't have strong opinions, but now we always use pairs of reasons like Initialized/NotInitialized, ScalingUp/NotScalingUp etc., which IMO is nicely consistent and also accurate and specific to the corresponding.
If we opt in to a generic reason for "all good" we are loosing those properties; also, I'm not sure if we can use it across the board, because e.g. "AsExpected" probably can't be used with conditions like ScalingUp or Remediating
| // Note: Stand-alone MachineSets and stand-alone Machines are not included in this condition. | ||
| ClusterWorkersAvailableV1Beta2Condition = "WorkersAvailable" | ||
|
|
||
| // ClusterWorkersAvailableNoWorkersV1Beta2Reason surfaces when no MachineDeployment and MachinePool exist for the Cluster. |
There was a problem hiding this comment.
ClusterWorkersAvailableNoWorkersV1Beta2Reason reason is used only of there are no objects to use from inferring workers Available.
If there is at least one MachineDeployment/MachinePool, this condition will aggregate Available condition from those objects, or, if the available conditions is not yet reported by those objects, it will consider available as unknown (and report this).
If you look at this from another angle, WorkersAvailable at cluster level should not look at replicas directly, but it should rely on whatever definition of Available is surfaced by other objects.
|
/approve /assign @enxebre |
|
[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
|
/hold for squash (keeping commit separated to help @enxebre in his final pass) |
k8s-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 664d2f4c90aa3c7d21ce64e1e1c6d217e3a3c3b7
|
fabriziopandini
force-pushed
the
update-v1beta2-status-cluster-controller
branch
from
b96b96e to
f766393
Compare
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@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:
Which issue(s) this PR fixes:
Rif #11105