Set observed gen in gen-reconciler by whaught · Pull Request #1261 · knative/pkg

whaught · 2020-04-29T17:45:18Z

Automatically bump ObservedGeneration in the generated reconciler and set status to unknown for unhandled errors.

looks for // +genducklogic to run this part of the code generation. This can serve as a jumping-off point to share other arbitrary logic among duck-type shaped resources.

knative-prow-robot · 2020-04-29T20:18:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: whaught
To complete the pull request process, please assign n3wscott
You can assign the PR to them by writing /assign @n3wscott in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

apis/OWNERS
codegen/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

whaught · 2020-04-30T18:59:09Z

/assign @n3wscott @mattmoor

antoineco · 2020-05-01T08:42:16Z

I like the idea but have 2 suggestions:

Make that behavior part of every reconciler instead of being opt-in. We don't require devs to decide whether they want the reconciler to call UpdateStatus on their behalf, so why would they decide to disable just those extra initializations? I think people who want the flexibility won't use reconciler-gen anyway.
If we insist on making it opt-in, please make your change in the existing tag (+genreconciler:initstatus=true, false if missing). That would avoid clutter and I think it's generally a better UX.

n3wscott

I wonder if this generated code belongs inside of the api and not the reconciler. If the reconciler can test that the resource under reconciliation implements some PostReconcile method that does the cleanups and testing, then you can use the correct condition sets and/or add a Mark* method to enable the interactions you are looking for.

n3wscott · 2020-05-01T14:59:00Z


 // +genclient
 // +genreconciler
+// +genducklogic


the only thing is that duck == interface, so geninterfacelogic does not make a ton of sense.

I won't pretend to be good at naming. I'm not sure I see the interface problem. The idea was "generate duck-type shaped common logic".

Can you suggest an alternative? @antoineco also suggested making it another option on +genreconciler if you have any thoughts on that (although that already has a :classname option)

n3wscott · 2020-05-01T15:01:02Z

 		}
 	}

+	{{if .genDuckLogic}}


the code will get here for deleted resources as well and it is not allowed to update generation for a deleted object

n3wscott · 2020-05-01T15:02:39Z

+		rc := resource.Status.GetCondition(apis.ConditionReady)
+		// if a new generation is observed and reconciliation reported an error event
+		// the reconciler should change the ready state. By default we will set unknown.
+		if equality.Semantic.DeepEqual(originalRc, rc) {


but this could be that it went from ready true to ready true if the update was something minor

This only runs if reconcileEvent != nil should we should be in a (potential) error case.

n3wscott · 2020-05-01T15:03:37Z

+		// the reconciler should change the ready state. By default we will set unknown.
+		if equality.Semantic.DeepEqual(originalRc, rc) {
+			logger.Warnw("A reconconiler observed a new generation without updating the resource status")
+			condSet.Manage(&resource.Status).MarkUnknown(


I don't think you should change the ready condition directly. It needs a dependent condition that cases the unreadiness.

whaught · 2020-05-01T15:58:20Z

I like the idea but have 2 suggestions:

Make that behavior part of every reconciler instead of being opt-in. We don't require devs to decide whether they want the reconciler to call UpdateStatus on their behalf, so why would they decide to disable just those extra initializations? I think people who want the flexibility won't use reconciler-gen anyway.

If we insist on making it opt-in, please make your change in the existing tag (+genreconciler:initstatus=true, false if missing). That would avoid clutter and I think it's generally a better UX.

The reason this isn't part of every reconciler is because not ever reconciler follows the duck-type code shape. Specifically some eventing resources don't have observed generation and this code fails for those.

The reason this is opt-in is partly to allow to allow generating for non-duck resource but also for safety as we switch to it. I want to migrate resources to it one-by one by adding this tag and removing their reconciler's handling of observed gen. This will let me test it safely as I go through that process. Unfortunately +genreconciler:classname is already a thing to allow reconcilers the option of specifying a type to reconciler. Adding more options to that tag might get pretty cluttered (we could comma separate them).

antoineco · 2020-05-01T17:26:03Z

@whaught that's a fair point, I didn't consider not every type implements these.

If the tag's elements are key=value separated by colons, I think we should be safe adding new parameters to it.

whaught · 2020-05-01T18:58:01Z

@whaught that's a fair point, I didn't consider not every type implements these.

If the tag's elements are key=value separated by colons, I think we should be safe adding new parameters to it.

The tags are not key=value. You can read the parsing here:

pkg/codegen/cmd/injection-gen/generators/packages.go

Line 191 in 83edd9f

func extractReconcilerClassTag(t *types.Type) (string, bool) {

it currently expects "genreconciler:class". We could change this and then go update existing usages of it, make it key value, and comma separate multiple tags. I'm not sure if this is really a worthy migration in the scope of what this PR is trying to accomplish for the cost of just adding a new tag.

antoineco · 2020-05-01T19:27:12Z

@whaught the current expected format is +genreconciler:class=<class>, which looks to me like a format that could very easily accept new parameters separated by colon or commas. Here are two examples:

+genreconciler:class=eventing.knative.dev/broker.class,initstatus=true
+genreconciler:class=eventing.knative.dev/broker.class:initstatus=true

Besides, there are at most 4 types that currently set this class parameter in the code base.

I simply can't see a case for a new tag here, but I'm also happy to be proven wrong.

whaught · 2020-05-01T19:55:40Z

@antoineco you're right about the existing format. Although a parser would need to be written to allow for other options. The argument is merely that a new tag doesn't cost anything and allows for options of its own. This isn't intended to be the only common generated logic, but a starting point.

The existing pattern in k8s which is being invoked is here:

pkg/vendor/k8s.io/gengo/types/comments.go

Line 36 in 099343d

// +foo=value1

you'll see that k8s doesn't expect implementors to be using comment tags this way. We sort of cheat by looking for the hardcoded tag "+genreconciler:class"

In the future I hope to be able to include other common factors like initializing defaults/conversions and common metrics on conditions.
Eventually I was forseeing something like +genducklogic:observegen=true,commonmetrics=true,initdefaults=true

Just thought it might get cluttered, and in line with what @n3wscott said, these aren't properties of a reconciler (which makes no resource shape assumptions) but are properties of the API duck resource.

I'm happy to go the other way on this if folks are passionate.

whaught · 2020-05-01T21:42:31Z

/hold I'm working on hacking together a potential alternative to this

whaught · 2020-05-04T21:00:50Z

/close

pulled this logic into #1284 with an interface that makes it more easily testable

knative-prow-robot · 2020-05-04T21:00:57Z

@whaught: Closed this PR.

Details

In response to this:

/close

pulled this logic into #1284 with an interface that makes it more easily testable

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2020-05-04T21:00:58Z

@whaught: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Set observed gen in gen-reconciler

0783f07

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 29, 2020

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 29, 2020

knative-prow-robot requested review from mattmoor and n3wscott April 29, 2020 17:45

whaught changed the title ~~[WIP} Set observed gen in gen-reconciler~~ [WIP] Set observed gen in gen-reconciler Apr 29, 2020

Create a new comment tag for duck logic

e4e185e

knative-prow-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 29, 2020

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 29, 2020

whaught changed the title ~~[WIP] Set observed gen in gen-reconciler~~ Set observed gen in gen-reconciler Apr 30, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 30, 2020

knative-prow-robot assigned mattmoor and n3wscott Apr 30, 2020

whaught mentioned this pull request Apr 30, 2020

ObservedGeneration should only be set if Status is fully updated #1226

Closed

n3wscott reviewed May 1, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2020

whaught mentioned this pull request May 1, 2020

Create an interface for retrieving duck-type fields #1275

Merged

knative-prow-robot closed this May 4, 2020

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2020

whaught deleted the genreconciler branch May 7, 2020 00:19

Conversation

whaught commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot commented Apr 29, 2020

Uh oh!

whaught commented Apr 30, 2020

Uh oh!

antoineco commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

n3wscott May 1, 2020

Choose a reason for hiding this comment

Uh oh!

whaught May 1, 2020

Choose a reason for hiding this comment

Uh oh!

n3wscott May 1, 2020

Choose a reason for hiding this comment

Uh oh!

n3wscott May 1, 2020

Choose a reason for hiding this comment

Uh oh!

whaught May 1, 2020

Choose a reason for hiding this comment

Uh oh!

n3wscott May 1, 2020

Choose a reason for hiding this comment

Uh oh!

whaught commented May 1, 2020

Uh oh!

antoineco commented May 1, 2020

Uh oh!

whaught commented May 1, 2020

Uh oh!

antoineco commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whaught commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whaught commented May 1, 2020

Uh oh!

whaught commented May 4, 2020

Uh oh!

knative-prow-robot commented May 4, 2020

Uh oh!

knative-prow-robot commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

whaught commented Apr 29, 2020 •

edited

Loading

antoineco commented May 1, 2020 •

edited

Loading

antoineco commented May 1, 2020 •

edited

Loading

whaught commented May 1, 2020 •

edited

Loading