Create a common method for processing reconciliation on generic KRShaped by whaught · Pull Request #1284 · knative/pkg

whaught · 2020-05-04T20:34:46Z

Issue #1226

Creates a post-process function for reasoning generically about KRShaped resources
- Automatically bumps ObservedGeneration
- Sets Ready=Unknown if an error is encountered and the reconciler did not update conditions.

This is not yet invoked, the next PR will integrate this with genreconciler.

Co-authored-by: Matt Moore <mattmoor@vmware.com>

whaught · 2020-05-04T20:41:08Z

/assign @grantr @n3wscott

whaught · 2020-05-05T00:48:41Z

/retest

antoineco · 2020-05-05T15:12:49Z

A general question about this PR: why would we want to force the Ready condition to Unknown instead of letting the user decide what to do with it in case of error?

antoineco · 2020-05-06T08:45:36Z

+}
+
+func getDefaultConditionSet(resource duckv1.KRShaped) (apis.ConditionSet, error) {
+	switch resource.GetTopLevelConditionType() {


Why didn't we use the term Happy instead of TopLevel here? Not saying it's good or bad, but it's just the term condition sets use right now.

@n3wscott on #1298 :

I would call this GetTopLevelConditionType

happy was a tongue and cheek joke for the internal code.

I myself am a bit indifferent. The first iteration had "GetHappyConditionType()"

Alright, glad that Scott could confirm 👍

antoineco · 2020-05-06T08:50:55Z

+	newStatus := resource.GetStatus()
+	if newStatus.ObservedGeneration != resource.GetObjectMeta().GetGeneration() {
+		// Reset ready to unknown. The reconciler is expected to overwrite this.
+		manager := condSet.Manage(newStatus)
+		manager.MarkUnknown(
+			manager.GetTopLevelCondition().Type, failedGenerationBump, "unsucessfully observed a new generation")
+	}


Don't we want to also call InitializeConditions here?

My fear was that I don't necessarily want to clobber all the conditions and I believe they should have been initialized in the admission webhook at this point?

Controllers call that method usually, webhooks only initialize the spec if I'm not mistaken (not every type has an admission webhook, for some third-party types the OpenAPI schema present in the CRD is sufficient).

I'm OK leaving that as it is, I was just curious what you thought about it.

This is also not a complete condition set (we don't make any assumptions about the dependant sub-conditions here)

hmm maybe we will want the full conditionset so that we can do this and at some point have the preprocessing do all the init stuff. I may reconsider this as another PR?

antoineco · 2020-05-06T13:32:00Z

/lgtm

n3wscott · 2020-05-06T22:13:55Z

This is great, nice work. I think this will blend well with our controllers and let us drop some easily forgotten copy/paste logic from the current reconcilers.

👏

/lgtm
/approve

Co-authored-by: Victor Agababov <vagababov@gmail.com>

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

Co-authored-by: Matt Moore <mattmoor@vmware.com>

knative-metrics-robot · 2020-05-07T00:06:17Z

The following is the coverage report on the affected files.
Say /test pull-knative-pkg-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
apis/condition_set.go	97.5%	96.7%	-0.8
reconciler/reconcile_common.go	Do not exist	91.7%

vagababov · 2020-05-07T00:08:06Z

/lgtm
/approve

grantr

Not sure why I'm an approver in reconciler/ and @n3wscott isn't...

/lgtm

knative-prow-robot · 2020-05-07T00:08:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, n3wscott, vagababov, whaught

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~apis/OWNERS~~ [n3wscott]
~~reconciler/OWNERS~~ [grantr,vagababov]

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

whaught and others added 9 commits May 1, 2020 15:21

Split KResource into a file. Include an interface.

5aa9856

Update apis/duck/v1/kresource_type.go

8948106

Co-authored-by: Matt Moore <mattmoor@vmware.com>

type

484ba2f

Merge branch 'and1' of github.com:whaught/pkg into and1

ff5f133

Merge remote-tracking branch 'upstream/master' into and1

ad28421

rename to KRShaped

af872c1

Merge remote-tracking branch 'upstream/master' into and1

28f1033

Create common logic for bumping obsgen

1533333

Merge remote-tracking branch 'upstream/master' into and1

2e9c4ee

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2020

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

knative-prow-robot requested review from dgerd and tcnghia May 4, 2020 20:34

knative-prow-robot assigned grantr and n3wscott May 4, 2020

whaught mentioned this pull request May 4, 2020

Set observed gen in gen-reconciler #1261

Closed

grantr reviewed May 4, 2020

View reviewed changes

Comment thread reconciler/reconcile_common.go Outdated

vagababov reviewed May 4, 2020

View reviewed changes

Comment thread reconciler/reconcile_common.go Outdated

Comment thread reconciler/reconcile_common_test.go Outdated

whaught added 2 commits May 4, 2020 15:44

fix typo

8a9b56d

remove interface cast

1197a30

vagababov reviewed May 4, 2020

View reviewed changes

Comment thread reconciler/reconcile_common_test.go Outdated

missed a spot

a379c4b

antoineco reviewed May 5, 2020

View reviewed changes

whaught added 4 commits May 5, 2020 09:03

Update to use ObjectMetaAccessor

ac9806f

switch alias to duckv1

9e36f80

fix var names

9cf316b

include a reason

cf92f7e

whaught added 3 commits May 5, 2020 15:37

missed a spot

9ddd78f

Merge remote-tracking branch 'upstream/master' into and1

e5741b1

update deps

be00d8b

antoineco reviewed May 6, 2020

View reviewed changes

whaught added 2 commits May 6, 2020 06:09

fix nits

66f8162

fix gomod deps

b029b20

knative-prow-robot assigned antoineco May 6, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

whaught added 2 commits May 6, 2020 14:57

Merge remote-tracking branch 'upstream/master' into and1

083bec0

update with new KRshaped

18cd45a

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

vagababov reviewed May 6, 2020

View reviewed changes

Comment thread reconciler/reconcile_common_test.go Outdated

Comment thread reconciler/reconcile_common_test.go Outdated

Update reconciler/reconcile_common_test.go

5c62918

Co-authored-by: Victor Agababov <vagababov@gmail.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 7, 2020

Update reconciler/reconcile_common_test.go

fb5242f

Co-authored-by: Victor Agababov <vagababov@gmail.com>

mattmoor reviewed May 7, 2020

View reviewed changes

Comment thread reconciler/reconcile_common_test.go Outdated

Update reconciler/reconcile_common_test.go

f9b76da

Co-authored-by: Matt Moore <mattmoor@vmware.com>

knative-prow-robot assigned vagababov May 7, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2020

grantr approved these changes May 7, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

knative-prow-robot merged commit c88f43e into knative:master May 7, 2020

whaught deleted the and1 branch May 7, 2020 00:17

Conversation

whaught commented May 4, 2020

Uh oh!

whaught commented May 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whaught commented May 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoineco commented May 5, 2020

Uh oh!

Uh oh!

Uh oh!

antoineco May 6, 2020

Choose a reason for hiding this comment

Uh oh!

whaught May 6, 2020

Choose a reason for hiding this comment

Uh oh!

antoineco May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoineco May 6, 2020

Choose a reason for hiding this comment

Uh oh!

whaught May 6, 2020

Choose a reason for hiding this comment

Uh oh!

antoineco May 6, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whaught May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoineco commented May 6, 2020

Uh oh!

n3wscott commented May 6, 2020

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knative-metrics-robot commented May 7, 2020

Uh oh!

vagababov commented May 7, 2020

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

whaught May 6, 2020 •

edited

Loading