OCPBUGS-17418: Handle cache.DeletedFinalStateUnknown #952

ncdc · 2023-08-07T14:13:18Z

No description provided.

openshift-ci-robot · 2023-08-07T14:13:26Z

@ncdc: This pull request references Jira Issue OCPBUGS-17418, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

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.

petr-muller · 2023-08-07T14:25:53Z

/jira refresh

openshift-ci-robot · 2023-08-07T14:26:01Z

@petr-muller: This pull request references Jira Issue OCPBUGS-17418, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jiajliu

Details

In response to this:

/jira refresh

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.

petr-muller

/hold

LGTM, holding because Lala also said he's gonna take a look

petr-muller · 2023-08-07T14:40:46Z

pkg/cvo/upgradeable.go

+	if !ok {
+		klog.Errorf("Unexpected type %T", obj)
+		return
+	}


This looks good to me.

I have looked into our other DeleteFunc handlers and it seems like none of them need a similar treatment because they do not use the object from the informer but just queue something else for processing.

cluster-version-operator/pkg/autoupdate/autoupdate.go

Line 119 in 9c0a4ac

DeleteFunc: func(obj interface{}) { ctrl.queue.Add(key) },

cluster-version-operator/pkg/cvo/cvo.go

Lines 504 to 506 in 9c0a4ac

DeleteFunc: func(obj interface{}) {

optr.queue.Add(workQueueKey)

},

cluster-version-operator/pkg/featurechangestopper/featurechangestopper.go

Lines 51 to 53 in 9c0a4ac

DeleteFunc: func(obj interface{}) {

c.queue.Add("cluster")

},

And just added 2763543 to make that more obvious.

The exposure has been around since 4.8 added admin acks [1], and we're not sure if watch-interruption has increased recently, relevant-ConfigMap deletion has increased, and/or our panic-monitoring has improved. [1]: https://github.com/openshift/cluster-version-operator/pull/647/files#diff-45df4901f88b6867d7fbf7e50690f376812864a0b85ec80dda5f77e6e19097b9R396-R402 Signed-off-by: Andy Goldstein <[email protected]>

wking · 2023-08-07T15:38:17Z

Run the suite that panicked, to ensure we excercise the adjusted ConfigMap delete handler (even if we probably miss the well-timed watch-interruption that triggered this panic):

/payload-job periodic-ci-openshift-release-master-ci-4.14-e2e-aws-sdn-serial

openshift-ci · 2023-08-07T15:38:19Z

@wking: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.14-e2e-aws-sdn-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6e37dea0-3538-11ee-9524-c74145fe9934-0

LalatenduMohanty

New commit message suggestion from @wking which looks good to me.

This is to hint to any future developers that there is no existing operand handling in the functions, and they need to add all of that themselves, including (hopefully they realize) DeletedFinalStateUnknown handling.  That's a fair bit of hope to get DeletedFinalStateUnknown in place, but calling that specific aspect out in a comment or something would risk them reading the comment and assuming we were giving them an exhaustive list of guards to consider, when they may need additional guards to match distant-future informer changes (they hopefully read [1]).  And the amount of churn expected for our other delete handlers is low, so investing even more time in trying to protect those future devs seems uneccessary.

[1]: https://pkg.go.dev/k8s.io/client-go/tools/cache#ResourceEventHandler

This is to hint to any future developers that there is no existing event object handling in the functions, and they need to add all of that themselves, including (hopefully they realize) DeletedFinalStateUnknown handling. That's a fair bit of hope to get DeletedFinalStateUnknown in place, but calling that specific aspect out in a comment or something would risk them reading the comment and assuming we were giving them an exhaustive list of guards to consider, when they may need additional guards to match distant-future informer changes (they hopefully read [1]). And the amount of churn expected for our other delete handlers is low, so investing even more time in trying to protect those future devs seems uneccessary. [1]: https://pkg.go.dev/k8s.io/client-go/tools/cache#ResourceEventHandler Signed-off-by: Andy Goldstein <[email protected]>

LalatenduMohanty

/lgtm

openshift-ci · 2023-08-07T15:52:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, ncdc, petr-muller

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:

~~OWNERS~~ [LalatenduMohanty,petr-muller]

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

wking · 2023-08-07T18:07:43Z

Serial rehearsal hiccuped while reporting Telemetry, but that's unrelated to this change, which suggests that the new code doesn't break under common ConfigMap-deletion conditions. We don't know for sure whether there was a watch hiccup overlapping with a deletion, but 🤷, we can keep an eye out for that in post-merge periodics.

/hold cancel

openshift-ci-robot · 2023-08-07T18:25:42Z

@ncdc: Jira Issue OCPBUGS-17418: All pull requests linked via external trackers have merged:

openshift/cluster-version-operator#952

Jira Issue OCPBUGS-17418 has been moved to the MODIFIED state.

Details

In response to this:

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.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 7, 2023

openshift-ci bot requested review from petr-muller and wking August 7, 2023 14:16

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 7, 2023

openshift-ci bot requested a review from jiajliu August 7, 2023 14:26

petr-muller approved these changes Aug 7, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023

openshift-ci bot assigned petr-muller Aug 7, 2023

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 7, 2023

ncdc force-pushed the tombstone branch from 2763543 to cedc387 Compare August 7, 2023 15:06

LalatenduMohanty suggested changes Aug 7, 2023

View reviewed changes

openshift-ci bot assigned LalatenduMohanty Aug 7, 2023

ncdc force-pushed the tombstone branch from cedc387 to f66389b Compare August 7, 2023 15:49

LalatenduMohanty approved these changes Aug 7, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023

openshift-merge-robot merged commit 86e24d6 into openshift:master Aug 7, 2023

openshift-ci-robot mentioned this pull request Aug 15, 2023

OCPBUGS-17418: Really handle DeletedFinalStateUnknown correctly #954

Merged

	DeleteFunc: func(obj interface{}) {
	optr.queue.Add(workQueueKey)
	},

OCPBUGS-17418: Handle cache.DeletedFinalStateUnknown #952

OCPBUGS-17418: Handle cache.DeletedFinalStateUnknown #952

Uh oh!

Conversation

ncdc commented Aug 7, 2023

Uh oh!

openshift-ci-robot commented Aug 7, 2023

Uh oh!

petr-muller commented Aug 7, 2023

Uh oh!

openshift-ci-robot commented Aug 7, 2023

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

petr-muller Aug 7, 2023 • 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!

wking Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 7, 2023

Uh oh!

openshift-ci bot commented Aug 7, 2023

Uh oh!

LalatenduMohanty left a comment • 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!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 7, 2023

Uh oh!

wking commented Aug 7, 2023

Uh oh!

openshift-ci-robot commented Aug 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

petr-muller Aug 7, 2023 •

edited

Loading

LalatenduMohanty left a comment •

edited

Loading