Revert "pkg/cvo/updatepayload.go: timeout payload retrieval" #881

petr-muller · 2023-01-06T12:16:11Z

Reverts #846

Apparently introduced a regression that blocks some upgrades, opening to allow /payload testing of periodic-ci-openshift-release-master-nightly-4.13-e2e-metal-ipi-upgrade-ovn-ipv6. If it fixes the job we should consider merging the revert and work on the fix with clean main branch.

…val"

openshift-ci · 2023-01-06T12:16:37Z

@petr-muller: This pull request references Bugzilla bug 2090680, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @evakhoni

Details

In response to this:

Revert "Bug 2090680: pkg/cvo/updatepayload.go: timeout payload retrieval"

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-01-06T12:17:18Z

/payload-job periodic-ci-openshift-release-master-nightly-4.13-e2e-metal-ipi-upgrade-ovn-ipv6

openshift-ci · 2023-01-06T12:17:22Z

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

periodic-ci-openshift-release-master-nightly-4.13-e2e-metal-ipi-upgrade-ovn-ipv6

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/107b2f70-8dbc-11ed-85af-ae9fb3ea3df1-0

petr-muller · 2023-01-06T12:18:12Z

This /payload thing is really nifty, the people who built it must be awesome humans!

openshift-ci · 2023-01-06T12:55:09Z

@petr-muller: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

Revert "pkg/cvo/updatepayload.go: timeout payload retrieval"

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-01-06T12:56:17Z

Removing the bug reference because this is not an actual fix for the bug and the automation started to act on it in the bugzilla.

deads2k · 2023-01-06T14:03:33Z

pkg/cvo/updatepayload.go

 	}
+	verifyCtx := ctx
+
+	// if 'force' specified, ensure call to verify payload signature times out well before parent context


This comment looks exactly like what I blindly suggested to Trevor :)

deads2k · 2023-01-06T14:06:43Z

/lgtm
/hold

I suggest merging this so that the "from" level gets fixed, but I'll hold in case Trevor wants to argue it.

openshift-ci · 2023-01-06T14:08:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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~~ [petr-muller]

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

deads2k · 2023-01-06T15:22:35Z

pkg/cvo/updatepayload.go


 	// download the payload to the directory
 	var err error
-	info.Directory, err = r.targetUpdatePayloadDir(retrieveCtx, update)


This is bugged. It couples the lookup timeout to the payload retrieval timeout, causing guaranteed failures when verifies timeout. This prevents force from working without signatures. Before reintroducing, you should write a test.

deads2k · 2023-01-06T15:22:46Z

/hold cancel

openshift-ci-robot · 2023-01-06T15:33:48Z

/retest-required

Remaining retests: 0 against base HEAD cdc1b51 and 2 for PR HEAD 5aca4e0 in total

petr-muller · 2023-01-06T19:46:28Z

/override ci/prow/e2e-agnostic-upgrade-into-change

Failures unrelated to CVO:

: [sig-node] nodes should not go unready after being upgraded and go unready only once
: [sig-network-edge] ns/openshift-authentication route/oauth-openshift disruption/ingress-to-oauth-server connection/new should be available throughout the test
: [sig-network-edge] ns/openshift-console route/console disruption/ingress-to-console connection/new should be available throughout the test 
: [sig-network-edge] ns/openshift-console route/console disruption/ingress-to-console connection/reused should be available throughout the test

openshift-ci · 2023-01-06T19:46:45Z

@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-upgrade-into-change

Details

In response to this:

/override ci/prow/e2e-agnostic-upgrade-into-change

Failures unrelated to CVO:

: [sig-node] nodes should not go unready after being upgraded and go unready only once
: [sig-network-edge] ns/openshift-authentication route/oauth-openshift disruption/ingress-to-oauth-server connection/new should be available throughout the test
: [sig-network-edge] ns/openshift-console route/console disruption/ingress-to-console connection/new should be available throughout the test 
: [sig-network-edge] ns/openshift-console route/console disruption/ingress-to-console connection/reused should be available throughout the test

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 · 2023-01-06T19:46:45Z

@petr-muller: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

evakhoni · 2023-01-08T21:13:10Z

@petr-muller this is just a revert so no qa on this one right?

petr-muller · 2023-01-08T22:45:22Z

@evakhoni that's correct

Revert "Bug 2090680: pkg/cvo/updatepayload.go: timeout payload retrie…

5aca4e0

…val"

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 6, 2023

openshift-ci bot requested review from DavidHurta, evakhoni and jottofar January 6, 2023 12:16

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2023

petr-muller changed the title ~~Revert "Bug 2090680: pkg/cvo/updatepayload.go: timeout payload retrieval"~~ Revert "pkg/cvo/updatepayload.go: timeout payload retrieval" Jan 6, 2023

openshift-ci bot removed bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 6, 2023

deads2k reviewed Jan 6, 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 Jan 6, 2023

openshift-ci bot assigned deads2k Jan 6, 2023

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

deads2k reviewed Jan 6, 2023

View reviewed changes

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

openshift-merge-robot merged commit a849b95 into openshift:master Jan 6, 2023

petr-muller mentioned this pull request Feb 3, 2023

Bug 2090680: RetrievePayload: Improve timeouts and cover behavior with tests #896

Merged

Revert "pkg/cvo/updatepayload.go: timeout payload retrieval" #881

Revert "pkg/cvo/updatepayload.go: timeout payload retrieval" #881

Uh oh!

Conversation

petr-muller commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

petr-muller commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

petr-muller commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

petr-muller commented Jan 6, 2023

Uh oh!

deads2k Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

deads2k Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 6, 2023

Uh oh!

openshift-ci-robot commented Jan 6, 2023

Uh oh!

petr-muller commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

openshift-ci bot commented Jan 6, 2023

Uh oh!

evakhoni commented Jan 8, 2023

Uh oh!

petr-muller commented Jan 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants