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.

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

Mention management of metadata.generation and status.observedGeneration. #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evankanderson
Copy link
Member

Document management of status.observedGeneration and client behavior from this comment.

TODO: experiment whether mutating webhooks could actually clear Conditions during PATCH of spec. If so, update webhooks and then we can remove this section.

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 25, 2021
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 25, 2021
@duglin duglin removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021
@@ -10,6 +8,20 @@ general Kubernetes recommendation with a `severity` field, and does not include
described in Resource Overview MUST have a `conditions` field in `status`, which
MUST be a list of `Condition` objects described by the following table.

## Client Determination of Readiness

Some servers (such as Kubernetes) may separate updates of the resource `spec`
Copy link
Contributor

Choose a reason for hiding this comment

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

not 100% sure but I think you mean for the "may" here to be "MAY" (meaning normative), right?

The stuff after the semi-colon doesn't flow right to me. What about:


Server implementations MAY separate updates of the resource spec and the status fields.
Whether an implementation does so or not, any update to the spec of a resource MUST result in the spec.metadata.generation field being updated (according to the Kubernetes rules). Implementation MUST update the status.observedGeneration field when all updates related to a corresponding spec.metadata.generation set of field values have been applied.

When determining whether a resource is "Ready", client implementations are expected to not only check the status.conditions values, but also whether the status.observedGeneration and spec.metadata.generation values are equal.


However, the more I think about this, the more it bothers me. ;-) Let's take the case where a resource is Ready but for whatever reason the "spec" section is updated a lot, but in ways that don't really impact much... e.g. maybe annotations are added/removed because some 3rd party tool is using that to control some non-Knative aspects of the overall system. Should the two "generation" values being different really make clients believe that the resource isn't ready and therefore not do things like send events to it? Having clients switch between thinking the resource is ready and not continually even tho all of the Conditions say "true" sounds like we're asking for things to be racey and intermittent - and give spotty performance.

Why not keep it simple and have clients only check the conditions and then the server can decide when to change the conditions? If it determines that it wants to change it on ANY change to the spec, then it can do so - even w/o actually doing the reconciliation.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW - clients should look at the generation values and compare them to understand if the changes applied to spec have been fullfilled.

This doesn't preclude clients from sending trafffic - but you're sending traffic to something that doesn't match the desired spec.

@dprotaso
Copy link
Member

TODO: experiment whether mutating webhooks could actually clear Conditions during PATCH of spec. If so, update webhooks and then we can remove this section.

I believe Webhooks being invoked for spec changes can't update the status. They are separate invocations since /status is a subresource

@dprotaso
Copy link
Member

@evankanderson do you still want this PR to merge? I didn't see a reply to @duglin's comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants