Skip to content

Set observed gen in gen-reconciler #1261

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

Closed
whaught wants to merge 2 commits into from
Closed

Set observed gen in gen-reconciler #1261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0783f07
Set observed gen in gen-reconciler
whaught Apr 29, 2020
e4e185e
Create a new comment tag for duck logic
whaught Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apis/test/example/v1alpha1/foo_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

// +genclient
// +genreconciler
// +genducklogic
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 1, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@whaught whaught May 1, 2020

Choose a reason for hiding this comment

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

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)

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// Foo is for testing.
Expand Down
8 changes: 8 additions & 0 deletions codegen/cmd/injection-gen/generators/packages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ func isNonNamespaced(t *types.Type) bool {
return nonNamespaced
}

func genDuckLogic(t *types.Type) bool {
comments := append(append([]string{}, t.SecondClosestCommentLines...), t.CommentLines...)
_, duckLogic := types.ExtractCommentTags("+", comments)["genducklogic"]
return duckLogic
}

func vendorless(p string) string {
if pos := strings.LastIndex(p, "/vendor/"); pos != -1 {
return p[pos+len("/vendor/"):]
Expand Down Expand Up @@ -418,6 +424,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp

reconcilerClass, hasReconcilerClass := extractReconcilerClassTag(t)
nonNamespaced := isNonNamespaced(t)
genDuck := genDuckLogic(t)

packagePath := filepath.Join(packagePath, strings.ToLower(t.Name.Name))

Expand Down Expand Up @@ -506,6 +513,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp
reconcilerClass: reconcilerClass,
hasReconcilerClass: hasReconcilerClass,
nonNamespaced: nonNamespaced,
genDuckLogic: genDuck,
})

return generators
Expand Down
26 changes: 26 additions & 0 deletions codegen/cmd/injection-gen/generators/reconciler_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type reconcilerReconcilerGenerator struct {
reconcilerClass string
hasReconcilerClass bool
nonNamespaced bool
genDuckLogic bool

groupGoName string
groupVersion clientgentypes.GroupVersion
Expand Down Expand Up @@ -75,6 +76,7 @@ func (g *reconcilerReconcilerGenerator) GenerateType(c *generator.Context, t *ty
"class": g.reconcilerClass,
"hasClass": g.hasReconcilerClass,
"nonNamespaced": g.nonNamespaced,
"genDuckLogic": g.genDuckLogic,
"controllerImpl": c.Universe.Type(types.Name{
Package: "knative.dev/pkg/controller",
Name: "Impl",
Expand Down Expand Up @@ -106,6 +108,7 @@ func (g *reconcilerReconcilerGenerator) GenerateType(c *generator.Context, t *ty
// Deps
"clientsetInterface": c.Universe.Type(types.Name{Name: "Interface", Package: g.clientsetPkg}),
"resourceLister": c.Universe.Type(types.Name{Name: g.listerName, Package: g.listerPkg}),
"conditionSet": c.Universe.Type(types.Name{Name: "ConditionSet", Package: "knative.dev/pkg/apis"}),
// K8s types
"recordEventRecorder": c.Universe.Type(types.Name{Name: "EventRecorder", Package: "k8s.io/client-go/tools/record"}),
// methods
Expand Down Expand Up @@ -211,6 +214,11 @@ type reconcilerImpl struct {
// Check that our Reconciler implements controller.Reconciler
var _ controller.Reconciler = (*reconcilerImpl)(nil)

{{if .genDuckLogic}}
// Creates the {{.conditionSet|raw}} to manipulate resource status conditions
var condSet = apis.NewLivingConditionSet()
{{end}}

`

var reconcilerNewReconciler = `
Expand Down Expand Up @@ -320,6 +328,24 @@ func (r *reconcilerImpl) Reconcile(ctx {{.contextContext|raw}}, key string) erro
}
}

{{if .genDuckLogic}}
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 1, 2020

Choose a reason for hiding this comment

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

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

// Bump observed generation to denote that we have processed this
// generation regardless of success or failure.
resource.Status.ObservedGeneration = resource.Generation

if resource.Status.ObservedGeneration != original.Status.ObservedGeneration && reconcileEvent != nil {
originalRc := original.Status.GetCondition(apis.ConditionReady)
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) {
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 1, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@whaught whaught May 1, 2020

Choose a reason for hiding this comment

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

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

logger.Warnw("A reconconiler observed a new generation without updating the resource status")
condSet.Manage(&resource.Status).MarkUnknown(
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 1, 2020

Choose a reason for hiding this comment

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

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

apis.ConditionReady, "", "unsucessfully observed a new generation")
}
}
{{end}}

// Synchronize the status.
if equality.Semantic.DeepEqual(original.Status, resource.Status) {
// If we didn't change anything then don't call updateStatus.
Expand Down