Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Verify generation in readiness checks #10920

Merged
merged 12 commits into from
Jan 9, 2024
Merged

Verify generation in readiness checks #10920

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d94c509
Verify generation in readiness checks
May 2, 2022
f067073
Merge branch 'main' into readiness-generation-check
Sep 1, 2022
4f99c86
ready checker- remove duplicate statefulset generational check
Sep 1, 2022
d008340
ready checker- comment update
Sep 1, 2022
0bf46a6
Merge branch 'main' into readiness-generation-check
Sep 2, 2022
a1a1aaf
Update pkg/kube/ready.go
muang0 Feb 26, 2023
fcc0332
Update pkg/kube/ready.go
muang0 Feb 26, 2023
dbb21fc
Update pkg/kube/ready.go
muang0 Feb 26, 2023
4cb62d1
Update pkg/kube/ready.go
muang0 Feb 26, 2023
141fa4a
Update pkg/kube/ready.go
muang0 Feb 26, 2023
dcd54d5
Merge branch 'main' into readiness-generation-check
joejulian May 10, 2023
1a3e9a9
addressing comment
stefanmcshane Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 64 additions & 18 deletions pkg/kube/ready.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,6 @@ type ReadyChecker struct {
// IsReady will fetch the latest state of the object from the server prior to
// performing readiness checks, and it will return any error encountered.
func (c *ReadyChecker) IsReady(ctx context.Context, v *resource.Info) (bool, error) {
var (
// This defaults to true, otherwise we get to a point where
// things will always return false unless one of the objects
// that manages pods has been hit
ok = true
err error
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was interesting. Several of us have talked about this and it's hard to grok what ok = true does here.

Connected with @sudermanjr who connected with @joejulian about this. Joe worked to split ready out from wait from Helm 2 to Helm 3, and seems this was not really noticed during that work. This appears to be a carry over from helm v2.

What I would like to know is, does this PR work without this change? Is anyone available to try? If so, I'd clean this up as a separate PR so it could be reverted on it's own if needed later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR works without this change when I tested with the affected handling of ReplicaSets and ReplicationController,
but of course the code looks a bit weirder..
So fixing that in a separate PR should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to club this change in with this PR rather than two separate PRs?

Echoing @bjosv's thoughts- feels weird to use those vars just for the [*corev1.ReplicationController, *extensionsv1beta1.ReplicaSet, *appsv1beta2.ReplicaSet, *appsv1.ReplicaSet] checks

@scottrigby

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, too, would rather do this in a separate PR (the reason I didn't remove them in the first place). Let's keep this PR to the single subject.

switch value := AsVersioned(v).(type) {
case *corev1.Pod:
pod, err := c.client.CoreV1().Pods(v.Namespace).Get(ctx, v.Name, metav1.GetOptions{})
Expand Down Expand Up @@ -180,11 +173,30 @@ func (c *ReadyChecker) IsReady(ctx context.Context, v *resource.Info) (bool, err
if !c.statefulSetReady(sts) {
return false, nil
}
case *corev1.ReplicationController, *extensionsv1beta1.ReplicaSet, *appsv1beta2.ReplicaSet, *appsv1.ReplicaSet:
ok, err = c.podsReadyForObject(ctx, v.Namespace, value)
}
if !ok || err != nil {
return false, err
case *corev1.ReplicationController:
rc, err := c.client.CoreV1().ReplicationControllers(v.Namespace).Get(ctx, v.Name, metav1.GetOptions{})
if err != nil {
return false, err
}
if !c.replicationControllerReady(rc) {
return false, nil
}
ready, err := c.podsReadyForObject(ctx, v.Namespace, value)
if !ready || err != nil {
return false, err
}
case *extensionsv1beta1.ReplicaSet, *appsv1beta2.ReplicaSet, *appsv1.ReplicaSet:
rs, err := c.client.AppsV1().ReplicaSets(v.Namespace).Get(ctx, v.Name, metav1.GetOptions{})
if err != nil {
return false, err
}
if !c.replicaSetReady(rs) {
return false, nil
}
ready, err := c.podsReadyForObject(ctx, v.Namespace, value)
if !ready || err != nil {
return false, err
}
}
return true, nil
}
Expand Down Expand Up @@ -272,6 +284,16 @@ func (c *ReadyChecker) volumeReady(v *corev1.PersistentVolumeClaim) bool {
}

func (c *ReadyChecker) deploymentReady(rs *appsv1.ReplicaSet, dep *appsv1.Deployment) bool {
// Verify the replicaset readiness
if !c.replicaSetReady(rs) {
return false
}
// Verify the generation observed by the deployment controller matches the spec generation
if dep.Status.ObservedGeneration != dep.ObjectMeta.Generation {
c.log("Deployment is not ready: %s/%s. observedGeneration (%s) does not match spec generation (%s).", dep.Namespace, dep.Name, dep.Status.ObservedGeneration, dep.ObjectMeta.Generation)
muang0 marked this conversation as resolved.
Show resolved Hide resolved
return false
}

expectedReady := *dep.Spec.Replicas - deploymentutil.MaxUnavailable(*dep)
if !(rs.Status.ReadyReplicas >= expectedReady) {
c.log("Deployment is not ready: %s/%s. %d out of %d expected pods are ready", dep.Namespace, dep.Name, rs.Status.ReadyReplicas, expectedReady)
Expand All @@ -281,6 +303,12 @@ func (c *ReadyChecker) deploymentReady(rs *appsv1.ReplicaSet, dep *appsv1.Deploy
}

func (c *ReadyChecker) daemonSetReady(ds *appsv1.DaemonSet) bool {
// Verify the generation observed by the daemonSet controller matches the spec generation
if ds.Status.ObservedGeneration != ds.ObjectMeta.Generation {
c.log("DaemonSet is not ready: %s/%s. observedGeneration (%s) does not match spec generation (%s).", ds.Namespace, ds.Name, ds.Status.ObservedGeneration, ds.ObjectMeta.Generation)
muang0 marked this conversation as resolved.
Show resolved Hide resolved
return false
}

// If the update strategy is not a rolling update, there will be nothing to wait for
if ds.Spec.UpdateStrategy.Type != appsv1.RollingUpdateDaemonSetStrategyType {
return true
Expand Down Expand Up @@ -351,18 +379,18 @@ func (c *ReadyChecker) crdReady(crd apiextv1.CustomResourceDefinition) bool {
}

func (c *ReadyChecker) statefulSetReady(sts *appsv1.StatefulSet) bool {
// Verify the generation observed by the statefulSet controller matches the spec generation
if sts.Status.ObservedGeneration != sts.ObjectMeta.Generation {
c.log("Statefulset is not ready: %s/%s. observedGeneration (%s) does not match spec generation (%s).", sts.Namespace, sts.Name, sts.Status.ObservedGeneration, sts.ObjectMeta.Generation)
muang0 marked this conversation as resolved.
Show resolved Hide resolved
return false
}

// If the update strategy is not a rolling update, there will be nothing to wait for
if sts.Spec.UpdateStrategy.Type != appsv1.RollingUpdateStatefulSetStrategyType {
c.log("StatefulSet skipped ready check: %s/%s. updateStrategy is %v", sts.Namespace, sts.Name, sts.Spec.UpdateStrategy.Type)
return true
}

// Make sure the status is up-to-date with the StatefulSet changes
if sts.Status.ObservedGeneration < sts.Generation {
c.log("StatefulSet is not ready: %s/%s. update has not yet been observed", sts.Namespace, sts.Name)
return false
}

// Dereference all the pointers because StatefulSets like them
var partition int
// 1 is the default for replicas if not set
Expand Down Expand Up @@ -403,6 +431,24 @@ func (c *ReadyChecker) statefulSetReady(sts *appsv1.StatefulSet) bool {
return true
}

func (c *ReadyChecker) replicationControllerReady(rc *corev1.ReplicationController) bool {
// Verify the generation observed by the replicationController controller matches the spec generation
if rc.Status.ObservedGeneration != rc.ObjectMeta.Generation {
c.log("ReplicationController is not ready: %s/%s. observedGeneration (%s) does not match spec generation (%s).", rc.Namespace, rc.Name, rc.Status.ObservedGeneration, rc.ObjectMeta.Generation)
muang0 marked this conversation as resolved.
Show resolved Hide resolved
return false
}
return true
}

func (c *ReadyChecker) replicaSetReady(rs *appsv1.ReplicaSet) bool {
// Verify the generation observed by the replicaSet controller matches the spec generation
if rs.Status.ObservedGeneration != rs.ObjectMeta.Generation {
c.log("ReplicaSet is not ready: %s/%s. observedGeneration (%s) does not match spec generation (%s).", rs.Namespace, rs.Name, rs.Status.ObservedGeneration, rs.ObjectMeta.Generation)
muang0 marked this conversation as resolved.
Show resolved Hide resolved
return false
}
return true
}

func getPods(ctx context.Context, client kubernetes.Interface, namespace, selector string) ([]corev1.Pod, error) {
list, err := client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
LabelSelector: selector,
Expand Down