@KnicKnic: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Hi @KnicKnic. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

/retest

/assign @brianpursley

/assign @seans3

@KnicKnic See kubernetes/kubectl#962 (comment). I'm trying to understand the scenario where the current wait functionality doesn't work. Can you provide an example?

@KnicKnic See kubernetes/kubectl#962 (comment). I'm trying to understand the scenario where the current wait functionality doesn't work. Can you provide an example?

I originally replied, here, I have moved the post to kubernetes/kubectl#962 (comment)

Thank you for the very detailed explanation @KnicKnic . I think it makes sense and illustrates the problem.

So basically whenever there is an observedGeneration < generation then you know there is some change pending that hasn't been observed yet. You can't be 100% certain that whatever change is pending is relevant to what you are waiting for, but by waiting for the observed generation to catch up to the generation, then you can know that you are looking at the latest status conditions and avoid a potential "false positive" when waiting for that condition.

It looks like your PR will handle cases where the resource might not have an observedGeneration (that's good), or in case where the observedGeneration is either at the status or the condition level.

I see that observedGeneration is at the status level for deployments, replicasets, and statefulsets. I am not aware of where it is at the condition level. I don't have a problem with that per-se, but would want to get others opinions if this is introducing a new convention that is not already in use. Feel free to correct me if I am wrong about this, because I'm just not too familiar with observedGeneration.

Aside from that, I think this seems like an OK change. It looks like you might be missing a test case around testing when observedGeneration is on status, not condition (I'll leave a comment in the review).

I'd like to hear some of the other reviewers chime in with their thoughts on this.

I see that observedGeneration is at the status level for deployments, replicasets, and statefulsets. I am not aware of where it is at the condition level. I don't have a problem with that per-se, but would want to get others opinions if this is introducing a new convention that is not already in use. Feel free to correct me if I am wrong about this, because I'm just not too familiar with observedGeneration.

@brianpursley observedGeneration is used in kubernetes(itself) on a condition in not many places
storageVersion uses it -

One thing about allowing observedgeneration to be different per condition. It allows 2 scenarios (that I can think of).

1. If a CRD has conditions that can be queried quickly, and those that can be queried slowly (to build the state). It allows the controller to quickly bump the quick conditions & their observedGeneration and bump the other ones slower.
   1. This allows quick updates for quick conditions
2. It allows a controller to be split up and have multiple actors act upon a single crd. Each can update their conditions seperately

My scenarios was that I am using ApiMachinery conditions for my controller so I should respect the observedGeneration in those objects. Also to future proof against 1 & 2.

I see that observedGeneration is at the status level for deployments, replicasets, and statefulsets. I am not aware of where it is at the condition level. I don't have a problem with that per-se, but would want to get others opinions if this is introducing a new convention that is not already in use. Feel free to correct me if I am wrong about this, because I'm just not too familiar with observedGeneration.

@brianpursley observedGeneration is used in kubernetes(itself) on a condition in not many places
storageVersion uses it -

kubernetes/staging/src/k8s.io/apiserver/pkg/storageversion/updater.go

Line 64 in 4efd564

ObservedGeneration: sv.Generation,

apiMachinery uses it -

kubernetes/staging/src/k8s.io/apimachinery/pkg/api/meta/conditions.go

Line 54 in a69a4a4

existingCondition.ObservedGeneration = newCondition.ObservedGeneration

One thing about allowing observedgeneration to be different per condition. It allows 2 scenarios (that I can think of).

1. If a CRD has conditions that can be queried quickly, and those that can be queried slowly (to build the state). It allows the controller to quickly bump the quick conditions & their observedGeneration and bump the other ones slower.

   1. This allows quick updates for quick conditions

2. It allows a controller to be split up and have multiple actors act upon a single crd. Each can update their conditions seperately

My scenarios was that I am using ApiMachinery conditions for my controller so I should respect the observedGeneration in those objects. Also to future proof against 1 & 2.

@dixudx @pwittrock Any thoguhts?

@brianpursley
ObservedGeneration is in the template recommended by sig machinery for conditions to help standardize usage of conditions. see KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1623-standardize-conditions . The actual implementation is at

@KnicKnic Thanks, I see that now. I guess I didn't notice that b/c it is in so many other Status types and only one place there.

Given that ObservedGeneration can occur at both Status and Condition and it looks like you're handling both cases, I think this looks fine.

/lgtm

/assign @seans3

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, KnicKnic, seans3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retest

I think these failures are from Docker Hub rate limiting?

I'm still getting caught up from a break so not sure if this has been encountered recently.

cc @spiffxp @BenTheElder @thejoycekung

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/97408/pull-kubernetes-conformance-kind-ga-only-parallel/1357023665123758080

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/97408/pull-kubernetes-node-e2e/1357023665153118208/

/retest