search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.5k stars 1.3k forks source link

Add Cluster ControlPlaneInitialized condition #3798

Closed ncdc closed 3 years ago

ncdc commented 3 years ago

User Story

As a developer, I would like to add a ControlPlaneInitialized condition to Clusters, so that MachineHealthCheck can use it when determining if a Machine has exceeded the allotted node startup timeout.

Detailed Description

We currently have cluster.status.controlPlaneInitialized, but it predates conditions and does not have a timestamp associated with it. Based on discussions in #3026, we want MachineHealthCheck to base nodeStartupTimeout calculations on the latter of (controlPlaneInitialized, machine creation timestamp).

This condition MUST be set to true as soon as we are able to establish connectivity with a workload cluster. We can keep the same logic we currently have for setting cluster.status.controlPlaneInitialized.

This condition MUST be a write-once value.

We also MUST remove cluster.status.controlPlaneInitialized from v1alpha4. As far as converting from 3 to 4, I'm not sure what we'd put in for LastTransitionTimestamp...

Anything else you would like to add:

3779 is similar/related

/kind feature /kind api-change /milestone v0.4.0 /priority important-soon

k8s-ci-robot commented 3 years ago

@ncdc: The provided milestone is not valid for this repository. Milestones in this repository: [Next, v0.3.11, v0.4.0]

Use /milestone clear to clear the milestone.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/3798): > > >**User Story** > >As a developer, I would like to add a ControlPlaneInitialized condition to Clusters, so that MachineHealthCheck can use it when determining if a Machine has exceeded the allotted node startup timeout. > >**Detailed Description** > >We currently have cluster.status.controlPlaneInitialized, but it predates conditions and does not have a timestamp associated with it. Based on discussions in #3026, we want MachineHealthCheck to base nodeStartupTimeout calculations on the latter of (controlPlaneInitialized, machine creation timestamp). > >**Anything else you would like to add:** > >#3779 is similar/related > >/kind feature >/kind api-change >/milestone v1alpha4 >/priority important-soon Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ncdc commented 3 years ago

cc @vincepri @JoelSpeed @detiber @fabriziopandini

vincepri commented 3 years ago

I assume we'll remove the boolean field?

ncdc commented 3 years ago

Yes!

ncdc commented 3 years ago

Depends on #3749 going in first

srm09 commented 3 years ago

@ncdc IIUC the intended MHC node startup timeout calculation would be as follows: [Current time - Max(CP.started, Machine.creationTime)] > Allotted node startup time

If the CP.Started timestamp value is not present(which would be the case for v1alpha3 object), then we set it to the current time during conversion. That way, for the first time, we are giving the machine the benefit of doubt here citing the lack of timestamp data being present.

Does this make sense?

ncdc commented 3 years ago

That's probably the only thing we can do, yes.

ncdc commented 3 years ago

/assign /lifecycle active

CecileRobertMichon commented 3 years ago

/unassign @ncdc