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

✨ Add predicate for Generation change on update event #553

Merged
merged 1 commit into from
Aug 13, 2019
Merged

✨ Add predicate for Generation change on update event #553

merged 1 commit into from
Aug 13, 2019

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Aug 7, 2019

Adds a GenerationChangedPredicate predicate similar to the ResourceVersionChangedPredicate that will filter out update events if an object's metadata.generation is unchanged.

This predicate is needed since the CRD status subresource is more commonly enabled now which allows users to filter out updates that have no spec changes.

Since this is a common ask, this predicate is better suited as a helper in the predicate library rather than having users write it themselves or being somewhere downstream in the operator-sdk.

Side note: For CRD objects the Generation increment is skipped on metatdata updates as expected. But this behavior isn't consistent for other APIs e.g Deployments where annotation updates will cause a Generation bump. kubernetes/kubernetes#67428 (comment)
I wasn't sure if this is worth pointing out in the comments though.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2019
@k8s-ci-robot k8s-ci-robot requested review from droot and pwittrock August 7, 2019 06:51
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2019
@hasbro17 hasbro17 changed the title ✨ Add predicate for Generation change on update event ✨ Add predicate for Generation change on update event Aug 7, 2019
@hasbro17
Copy link
Contributor Author

hasbro17 commented Aug 7, 2019

/assign @droot

droot
droot reviewed Aug 7, 2019
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Looks good, have a couple of minor comments.

pkg/predicate/predicate.go Outdated
@@ -116,3 +116,32 @@ func (ResourceVersionChangedPredicate) Update(e event.UpdateEvent) bool {
}
return true
}

// GenerationChangedPredicate implements a default update predicate function on generation change
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be good to add the details that you specified in the PR description especially mentioning status sub-resource. That sort of explains when this predicate is useful.

Copy link

Choose a reason for hiding this comment

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

I would also like to see an update to the examples to show how to use the predicate? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean showing how to use one of the pre-defined predicates as an argument to the controller's Watch()?
I guess I could add that since I don't see it in any of the other examples:
https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/controller#ex-Controller

There's also an existing example in the predicate godoc, outlining how to define a new predicate.
Interestingly enough it's using generation change as the example.
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/predicate/example_test.go#L27-L33
https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/predicate#ex-Funcs

I could update that to be more complete like the one written here. Or leave that as it is if it's meant to be just a short example.

@droot Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep that example as-is.

In addition to a "why", I think this needs a "caveats" section -- like "this means you won't self-correct when someone else wipes your status".

pkg/predicate/predicate_test.go Outdated Show resolved Hide resolved
@hasbro17 hasbro17 force-pushed the add-generation-predicate branch from f67d7fa to bdc1712 Compare August 12, 2019 05:48
@hasbro17
Copy link
Contributor Author

Updated the godocs for GenerationChangedPredicate to explain how the Generation field works and the purpose of the predicate, along with a caveats section on the gotchas with using the predicate.
PTAL to see if the comments need to be more elaborate or concise. Or if there are any other caveats.

@shawn-hurley Based on the comments above I haven't updated the existing example. Let me know if you think we still need another example.

@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, hasbro17

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2019
@k8s-ci-robot k8s-ci-robot merged commit d7467fc into kubernetes-sigs:master Aug 13, 2019
@hasbro17 hasbro17 deleted the add-generation-predicate branch August 15, 2019 00:24
@DirectXMan12 DirectXMan12 mentioned this pull request Aug 28, 2019
Closed
metral added a commit to pulumi/pulumi-kubernetes-operator that referenced this pull request Jul 10, 2020
@metral
stack-cntlr(Reconcile): skip extra loop if status updated
abe714d 
See:
 - operator-framework/operator-sdk#2795
 - kubernetes-sigs/kubebuilder#1103
 - kubernetes-sigs/controller-runtime#553
 - https://book-v1.book.kubebuilder.io/basics/status_subresource.html
@hiddeco hiddeco mentioned this pull request Sep 12, 2020
Closed
@zcahana zcahana mentioned this pull request Nov 9, 2020
Merged
@danaradg danaradg mentioned this pull request Apr 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shawn-hurley shawn-hurley shawn-hurley left review comments

@droot droot droot left review comments

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@DirectXMan12 DirectXMan12

@droot droot

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hasbro17 @DirectXMan12 @k8s-ci-robot @droot @shawn-hurley