fix: report the control plane as unavailable when it cannot be reached - #1567
fix: report the control plane as unavailable when it cannot be reached#1567s3rj1k wants to merge 3 commits into
Conversation
28bf1eb to
f10740c
Compare
| } | ||
|
|
||
| // truncateMessage keeps a condition message within what the CRD accepts, since | ||
| // a longer one would make every status patch fail validation. |
There was a problem hiding this comment.
which CRD has this limit? I haven't heard about it before
There was a problem hiding this comment.
https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/types.go#L1693
It should have been this limit MaxLength=32768, and the idea was to just truncate to sane amount, comment is wrong here, got incorrect after automatic summarization and rewrite, thanks!
There was a problem hiding this comment.
on a second thought, I just might go with MaxLength=32768
| // Connect and disconnect events need no option. WatchForProbeFailure exists to | ||
| // drive staleness grace periods, which this provider does not have. | ||
| if c.ClusterCache != nil { | ||
| builder = builder.WatchesRawSource(c.ClusterCache.GetClusterSource("k0scontrolplane", clusterToK0sControlPlane)) |
|
|
||
| // The cache probe only asks for the API root, which a server with | ||
| // broken storage still answers, so this read decides on its own. | ||
| setNotAvailable(controlplane.kcp, cpv1beta2.ControlPlaneNotAvailableReason, err.Error()) |
There was a problem hiding this comment.
single ping failure will set cluster not available. I think we need to have some retries here or some logic like readiness/liveness probes have
b65e307 to
4382ffb
Compare
c3904b4 to
e5afc78
Compare
K0sControlPlane only ever set Available to true, so a control plane that went away kept reporting the last good state. It now reports false once reads have kept failing, on a timer, since the cluster cache watches another endpoint. The hosted flavour reports the same way instead of flipping on the first failure, an unpaused cluster gets its control plane looked at again, and reads of a workload cluster keep the proxy and credentials its kubeconfig carries. Signed-off-by: s3rj1k <evasive.gyron@gmail.com>
The clusterclass upgrade test bumped spec.topology.version as soon as the control plane reported ready, but the topology webhook refuses that while any MachineDeployment is still rolling, so the test failed whenever the workers had not caught up. Wait for them the same way the webhook decides. Signed-off-by: s3rj1k <evasive.gyron@gmail.com>
e5afc78 to
6205640
Compare
|
LGTM, but I'd like @apedriza to have a look before merging. |
| // The count cannot be dropped here, since the key is the UID and this | ||
| // object was never read. reconcileDelete covers the ordinary path. |
There was a problem hiding this comment.
| // The count cannot be dropped here, since the key is the UID and this | |
| // object was never read. reconcileDelete covers the ordinary path. |
Seems it is missing a bit more context about what "the count" means. I guess you refer availabilityFailures but it's confusing at first glance. I would just remove it
|
|
||
| // availabilityFailureFloor is how many failed reads have to be seen by this process | ||
| // before an outage is reported, whatever the persisted anchor says about the time. | ||
| const availabilityFailureFloor = 2 |
There was a problem hiding this comment.
| const availabilityFailureFloor = 2 | |
| const availabilityFailureThreshold = 2 |
for consistency with other limit consts
| // internal error would. | ||
| if !initialized(controlplane.kcp.Status.Initialization.ControlPlaneInitialized) { | ||
| return | ||
| } |
There was a problem hiding this comment.
I think we could just assume controlplane is not available at the beginning of the func if it is not initialized. Ideally, any try of workload connection should be done once we now it is initialized
|
|
||
| if *kcp.Status.UpToDateReplicas != kcp.Spec.Replicas { | ||
| return true | ||
| if ptr.Deref(kcp.Status.UpToDateReplicas, 0) != kcp.Spec.Replicas { |
There was a problem hiding this comment.
requeue decision based on replica count is made in the scale logic. I wonder if we should move the current block here of focus this func to requeue based on Availability. If so, I would rename the func otherwise, I would centralize requeue based on replica status in one single point
| // While coming up the contract fallback says this better than any internal | ||
| // error would, and the condition defaults to Unknown at the epoch here. | ||
| if !initialized(kcp.Status.Initialization.ControlPlaneInitialized) { | ||
| return | ||
| } |
There was a problem hiding this comment.
same as in the other controller, controlplane initialized should determine if we compute availability
Signed-off-by: s3rj1k <evasive.gyron@gmail.com>
d2cb27d to
e499fa8
Compare
3 participants
Fixes a control plane that cannot be reached still reporting itself as available.