Don't rely on in-reconciler state to determine last-in-cluster. #33

markusthoemmes · 2020-04-27T12:24:33Z

Proposed Changes

Trying to determine the last resource in the cluster via state kept in the reconciler is racy and error-prone. Ask the API instead.

Deployments should also be removed via the ownerRef. An explicit deletion shouldn't be necessary.

Release Note

NONE

/assign @houshengbo @jcrossley3 @aliok

knative-prow-robot

@markusthoemmes: 0 warnings.

In response to this:

Proposed Changes

Trying to determine the last resource in the cluster via state kept in the reconciler is racy and error-prone. Ask the API instead.

Deployments should also be removed via the ownerRef. An explicit deletion shouldn't be necessary.

Release Note
NONE
/assign @houshengbo @jcrossley3 @aliok

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-04-27T12:24:49Z

Hi @markusthoemmes. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jcrossley3

Looks good. Makes sense. Thanks! Just a couple questions.

jcrossley3 · 2020-04-27T13:18:36Z

pkg/reconciler/knativeeventing/knativeeventing.go

+		return fmt.Errorf("failed to list all KnativeEventings: %w", err)
+	}
+
+	if len(kes.Items) == 1 {


Curious whether there's also a race condition here. Specifically, if the API server tells us there are 2 while both are being deleted, is it safe to rely on the other's reconcile loop to delete the cluster-scoped resources? I assume so, but just checking.

Hm, you are right. I'll make this a little more bulletproof by checking if all items have a DeletionTimestamp set, i.e.: All resources are being deleted.

jcrossley3 · 2020-04-27T13:20:46Z

pkg/reconciler/knativeserving/knativeserving.go

+		return fmt.Errorf("failed to list all KnativeServings: %w", err)
+	}
+
+	if len(kes.Items) == 1 {


Same question above applies, but also makes me wonder if we can factor this "delete the RBAC last" logic into a shared lib.

We probably can, yes. This is part of a larger refactor which I wanted to get through first before consolidating too much.

houshengbo · 2020-04-27T14:07:52Z

pkg/reconciler/knativeserving/knativeserving.go

+	if allBeingDeleted {
+		// If the list has only one element, we're currently reconciling the last one and thus
+		// can go ahead and delete cluster-scoped resources.
+		manifest, err := r.transform(ctx, instance)


Along the line 263 before, you use r.transform(instance), but add ctx as another param here. Which one should be the correct one?

I see. they are two different functions

Both will eventually have a ctx once I land a followup of this that will use genreconciler for KnativeEventing as well.

It boils down to logger usage: We should always use the logger in the context, because it has all the... well... context already set to it.

markusthoemmes · 2020-04-27T14:17:09Z

/ok-to-test

houshengbo

/lgtm

houshengbo · 2020-04-28T14:28:41Z

/approve

knative-prow-robot · 2020-04-28T14:28:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, markusthoemmes

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:

~~OWNERS~~ [houshengbo]

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

houshengbo · 2020-04-28T14:38:46Z

/test pull-knative-sandbox-operator-integration-tests

markusthoemmes · 2020-04-28T14:41:00Z

hmpf, rebase mistake.

houshengbo · 2020-04-28T15:50:14Z

/lgtm

knative-prow-robot · 2020-04-28T15:59:11Z

@markusthoemmes: PR needs rebase.

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.

Signed-off-by: Matthias Wessendorf <[email protected]>

knative-prow-robot assigned aliok, houshengbo and jcrossley3 Apr 27, 2020

knative-prow-robot requested review from aliok and jcrossley3 April 27, 2020 12:24

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

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

knative-prow-robot reviewed Apr 27, 2020

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 27, 2020

jcrossley3 reviewed Apr 27, 2020

View reviewed changes

houshengbo reviewed Apr 27, 2020

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 27, 2020

markusthoemmes force-pushed the delete-respective branch from 316bcfd to 96c722c Compare April 28, 2020 06:43

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 28, 2020

markusthoemmes force-pushed the delete-respective branch from 20ab838 to fc24d1b Compare April 28, 2020 09:05

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2020

markusthoemmes force-pushed the delete-respective branch from fc24d1b to db52b0d Compare April 28, 2020 14:09

houshengbo reviewed Apr 28, 2020

View reviewed changes

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

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

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

Decouple cluster-scoped resource deletion from tracking.

0fcb8de

markusthoemmes force-pushed the delete-respective branch from 4e63b35 to 0fcb8de Compare April 28, 2020 15:34

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

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

houshengbo removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2020

knative-prow-robot merged commit e1d9ea9 into knative:master Apr 28, 2020

matzew added a commit to matzew/knative-operator that referenced this pull request Sep 10, 2020

💄 adding ttlSecondsAfterFinished to the job (knative#33)

ad4bbdf

Signed-off-by: Matthias Wessendorf <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't rely on in-reconciler state to determine last-in-cluster. #33

Don't rely on in-reconciler state to determine last-in-cluster. #33

markusthoemmes commented Apr 27, 2020

knative-prow-robot left a comment

knative-prow-robot commented Apr 27, 2020

jcrossley3 left a comment

jcrossley3 Apr 27, 2020

markusthoemmes Apr 27, 2020

jcrossley3 Apr 27, 2020

markusthoemmes Apr 27, 2020

houshengbo Apr 27, 2020

houshengbo Apr 27, 2020

markusthoemmes Apr 27, 2020

markusthoemmes commented Apr 27, 2020

houshengbo left a comment

houshengbo commented Apr 28, 2020

knative-prow-robot commented Apr 28, 2020

houshengbo commented Apr 28, 2020

markusthoemmes commented Apr 28, 2020

houshengbo commented Apr 28, 2020

knative-prow-robot commented Apr 28, 2020

Don't rely on in-reconciler state to determine last-in-cluster. #33

Don't rely on in-reconciler state to determine last-in-cluster. #33

Conversation

markusthoemmes commented Apr 27, 2020

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Apr 27, 2020

jcrossley3 left a comment

Choose a reason for hiding this comment

jcrossley3 Apr 27, 2020

Choose a reason for hiding this comment

markusthoemmes Apr 27, 2020

Choose a reason for hiding this comment

jcrossley3 Apr 27, 2020

Choose a reason for hiding this comment

markusthoemmes Apr 27, 2020

Choose a reason for hiding this comment

houshengbo Apr 27, 2020

Choose a reason for hiding this comment

houshengbo Apr 27, 2020

Choose a reason for hiding this comment

markusthoemmes Apr 27, 2020

Choose a reason for hiding this comment

markusthoemmes commented Apr 27, 2020

houshengbo left a comment

Choose a reason for hiding this comment

houshengbo commented Apr 28, 2020

knative-prow-robot commented Apr 28, 2020

houshengbo commented Apr 28, 2020

markusthoemmes commented Apr 28, 2020

houshengbo commented Apr 28, 2020

knative-prow-robot commented Apr 28, 2020