Bug 1914932: Put correct resource name in relatedObjects #945

alexanderConstantinescu · 2021-01-12T17:50:11Z

PR: #873 was missing a correct resource name for networks.operator.openshift.io to be exported properly to a must-gather. PR: #877 tried fixing this, but was missing a piece to be able to pass the CI upgrade jobs, which this PR now adds.

Essentially upgrade jobs upgrade from HEAD to HEAD + patch. So with #873: HEAD now has a resource name which is incorrect, if we now want to fix this we will incorrectly delete networks.operator.openshift.io on such upgrades. This is not yet a problem for any OpenShift release, since #873 is only on master. The only fix I could think of was to explicitly skip the deletion of the CRD for now. Once this PR has merged we can then delete that explicit skip, since master will have the correct resource name and we won't expect that to change thereafter. I will file a PR for that once this integrates.

/assign @squeed

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

alexanderConstantinescu · 2021-01-12T20:34:16Z

Hooray, upgrades pass! Let's review this and get it in!

@squeed : could you review this tomorrow?

/retitle Bug 1914932: Put correct resource name in relatedObjects

openshift-ci-robot · 2021-01-12T20:34:37Z

@alexanderConstantinescu: This pull request references Bugzilla bug 1914932, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1914932: Put correct resource name in relatedObjects

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.

squeed · 2021-01-13T12:20:04Z

Suggestion: just always add the object in SetRelatedObjects. Then you don't have to worry about skipping it on delete.

alexanderConstantinescu · 2021-01-13T12:36:27Z

Suggestion: just always add the object in SetRelatedObjects. Then you don't have to worry about skipping it on delete.

That's what's happening here:

cluster-network-operator/pkg/controller/operconfig/operconfig_controller.go

Line 250 in b418646

relatedObjects = append(relatedObjects, configv1.ObjectReference{

, right? It's still getting deleted on the update jobs since the resource field changes between the versions.

Anyhow, the goal is to remove the skip altogether in a succeeding PR as I mentioned in the description.

squeed · 2021-01-13T13:59:41Z

That's a good point.
/lgtm
/retest

squeed · 2021-01-13T13:59:49Z

/approve

openshift-ci-robot · 2021-01-13T14:00:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderConstantinescu, squeed

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~~ [squeed]

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

alexanderConstantinescu · 2021-01-13T16:00:02Z

/test e2e-aws-sdn-multi
/test e2e-gcp
/test e2e-gcp-ovn

openshift-bot · 2021-01-13T17:08:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-13T17:21:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-13T23:21:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T02:36:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T02:49:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T04:07:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T08:27:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T08:40:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

alexanderConstantinescu · 2021-01-14T09:07:37Z

@danwinship : could you override e2e-aws-sdn-multi ? Its history looks grim https://prow.ci.openshift.org/job-history/gs/origin-ci-test/pr-logs/directory/pull-ci-openshift-cluster-network-operator-master-e2e-aws-sdn-multi and I don't want to hold this PR anymore because of it

EDIT: never mind, it started passing.

openshift-bot · 2021-01-14T10:24:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T13:26:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

juanluisvaladas · 2021-01-14T15:19:52Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-01-14T15:19:53Z

@juanluisvaladas: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.6

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.

alexanderConstantinescu · 2021-01-14T15:24:55Z

/cherry-pick release-4.6 cancel

We shouldn't cherry-pick this as-is. I need another PR towards master after this one merges, as mentioned in the description.

openshift-cherrypick-robot · 2021-01-14T15:24:57Z

@alexanderConstantinescu: once the present PR merges, I will cherry-pick it on top of release-4.6 cancel in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.6 cancel

We shouldn't cherry-pick this as-is. I need another PR towards master after this one merges, as mentioned in the description.

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.

juanluisvaladas · 2021-01-14T15:36:57Z

/retest

danwinship · 2021-01-14T16:22:01Z

pkg/controller/statusmanager/status_manager.go

@@ -97,6 +95,11 @@ func (status *StatusManager) deleteRelatedObjectsNotRendered(co *configv1.Cluste
 				log.Printf("Object Kind is Namespace, skip")
 				continue
 			}
+			// @aconstan: remove this after having the PR implementing this change, integrated.


Ugh. If we add new objects to relatedObjects to get them must-gathered, then that means we'll end up deleting those objects when you downgrade to a version of CNO where they weren't part of relatedObjects. This may be the cause of the "openshift-sdn CRDs get deleted when downgrading" bug? (Not sure where that is or who was working on it...)

We need to avoid doing deleteRelatedObjectsNotRendered on downgrade. @squeed

For now, the fix is probably to backport this hack to 4.6 and 4.5

It is the cause of it, @juanluisvaladas verified it with PR: #948. We need this back-ported.

openshift-bot · 2021-01-14T18:50:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

juanluisvaladas · 2021-01-14T20:22:55Z

/retest

openshift-bot · 2021-01-14T21:13:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T22:05:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-14T22:44:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-01-14T23:17:28Z

@alexanderConstantinescu: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-vsphere-ovn	`b418646`	link	`/test e2e-vsphere-ovn`

Full PR test history. Your PR dashboard.

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.

openshift-bot · 2021-01-14T23:36:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-15T00:51:16Z

@alexanderConstantinescu: All pull requests linked via external trackers have merged:

openshift/cluster-network-operator#945

Bugzilla bug 1914932 has been moved to the MODIFIED state.

In response to this:

Bug 1914932: Put correct resource name in relatedObjects

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-cherrypick-robot · 2021-01-15T00:51:39Z

@juanluisvaladas: #945 failed to apply on top of branch "release-4.6":

Applying: Bug 1914932: Put correct resource name in relatedObjects
Using index info to reconstruct a base tree...
M	pkg/controller/operconfig/operconfig_controller.go
M	pkg/controller/statusmanager/status_manager.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/statusmanager/status_manager.go
Auto-merging pkg/controller/operconfig/operconfig_controller.go
CONFLICT (content): Merge conflict in pkg/controller/operconfig/operconfig_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 1914932: Put correct resource name in relatedObjects
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.6

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-cherrypick-robot · 2021-01-15T00:51:43Z

@alexanderConstantinescu: cannot checkout release-4.6 cancel: error checking out release-4.6 cancel: exit status 1. output: error: pathspec 'release-4.6 cancel' did not match any file(s) known to git

In response to this:

/cherry-pick release-4.6 cancel

We shouldn't cherry-pick this as-is. I need another PR towards master after this one merges, as mentioned in the description.

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.

Bug 1914932: Put correct resource name in relatedObjects

b418646

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

openshift-ci-robot assigned squeed Jan 12, 2021

openshift-ci-robot requested review from danielmellado and danwinship January 12, 2021 17:50

alexanderConstantinescu mentioned this pull request Jan 12, 2021

Bug 1914932: Put correct resource name in relatedObjects #877

Closed

openshift-ci-robot changed the title ~~Put correct resource name in relatedObjects~~ Bug 1914932: Put correct resource name in relatedObjects Jan 12, 2021

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jan 12, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 12, 2021

openshift-ci-robot 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. labels Jan 13, 2021

juanluisvaladas mentioned this pull request Jan 14, 2021

[DNM] Debug downgrade to 4.6 #948

Closed

danwinship reviewed Jan 14, 2021

View reviewed changes

openshift-merge-robot merged commit 06a19f9 into openshift:master Jan 15, 2021

alexanderConstantinescu mentioned this pull request Jan 15, 2021

Bug 1916601: guard against the deletion of the network CRD: network.operator.openshift.io #953

Merged

Bug 1914932: Put correct resource name in relatedObjects #945

Bug 1914932: Put correct resource name in relatedObjects #945

Conversation

alexanderConstantinescu commented Jan 12, 2021

alexanderConstantinescu commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

squeed commented Jan 13, 2021

alexanderConstantinescu commented Jan 13, 2021

squeed commented Jan 13, 2021

squeed commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

alexanderConstantinescu commented Jan 13, 2021

openshift-bot commented Jan 13, 2021

openshift-bot commented Jan 13, 2021

openshift-bot commented Jan 13, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

alexanderConstantinescu commented Jan 14, 2021 • edited

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

juanluisvaladas commented Jan 14, 2021

openshift-cherrypick-robot commented Jan 14, 2021

alexanderConstantinescu commented Jan 14, 2021

openshift-cherrypick-robot commented Jan 14, 2021

juanluisvaladas commented Jan 14, 2021

danwinship Jan 14, 2021

Choose a reason for hiding this comment

alexanderConstantinescu Jan 14, 2021

Choose a reason for hiding this comment

openshift-bot commented Jan 14, 2021

juanluisvaladas commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-bot commented Jan 14, 2021

openshift-ci bot commented Jan 14, 2021 • edited

openshift-bot commented Jan 14, 2021

openshift-ci-robot commented Jan 15, 2021

openshift-cherrypick-robot commented Jan 15, 2021

openshift-cherrypick-robot commented Jan 15, 2021

alexanderConstantinescu commented Jan 14, 2021 •

edited

openshift-ci bot commented Jan 14, 2021 •

edited