Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

sgreene570 · 2021-05-11T17:39:37Z

Commit ea085e7 added logic to reconcile the Ingress namespace during
cluster upgrades. This follow-up commit fixes some mistakes introduced
by that commit.

pkg/operator/controller/ingress/namespace.go:

Fix a typo: opensift.io/cluster-monitoring.
We are, after all, in the computing business, and not the baking
business.

Check if annotation/label map values exist since comparing a
non-existent map value to the empty string will always return true
in this case.

pkg/operator/controller/ingress/namespace_test.go:

Add unit test to prove that a map value that does not exist
is equivalent to the empty string.

Rename the original namespace to desired.
Switch ordering of desired and mutated in routerNamespaceChanged
calling sites to better reflect the intent of the unit tests.

This is a follow up to #608, which merged with some mistakes that were missed during review.

openshift-ci · 2021-05-11T17:39:41Z

@sgreene570: This pull request references Bugzilla bug 1954330, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

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

In response to this:

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation

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.

sgreene570 · 2021-05-11T17:40:03Z

/bugzilla refresh

openshift-ci · 2021-05-11T17:40:10Z

@sgreene570: This pull request references Bugzilla bug 1954330, 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.8.0) matches configured target release for branch (4.8.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 @asood-rh

In response to this:

/bugzilla 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.

Miciah · 2021-05-11T17:55:11Z

Check if annotation/label map values exist since comparing a
non-existent map value to the empty string will always return true
in this case.

Are you sure? That is surprising to me. Can you add a unit test for this?

sgreene570 · 2021-05-11T18:14:01Z

Check if annotation/label map values exist since comparing a
non-existent map value to the empty string will always return true
in this case.

Are you sure? That is surprising to me. Can you add a unit test for this?

Yup. I was also surprised by this behavior.
I've added a unit test (which required some slight tweaking to the testing logic).

Commit `ea085e7` added logic to reconcile the Ingress namespace during cluster upgrades. This follow-up commit fixes some mistakes introduced by that commit. pkg/operator/controller/ingress/namespace.go: Fix a typo: `opensift.io/cluster-monitoring`. We are, after all, in the computing business, and not the baking business. Check if annotation/label map values exist since comparing a non-existent map value to the empty string will always return true in this case. pkg/operator/controller/ingress/namespace_test.go: Add unit test to prove that a map value that does not exist is equivalent to the empty string. Rename the `original` namespace to `desired`. Switch ordering of `desired` and `mutated` in `routerNamespaceChanged` calling sites to better reflect the intent of the unit tests.

openshift-ci · 2021-05-11T18:18:37Z

@sgreene570: This pull request references Bugzilla bug 1954330, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @asood-rh

In response to this:

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation

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 · 2021-05-11T18:19:07Z

@sgreene570: An error was encountered querying GitHub for users with public email (asood@redhat.com) for bug 1954330 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#abuse-rate-limits\",\n  \"message\": \"You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation

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.

sgreene570 · 2021-05-11T18:41:00Z

/retest

Miciah · 2021-05-11T19:17:59Z

pkg/operator/controller/ingress/namespace_test.go

 		tc.mutate(mutated)
-		if changed, updated := routerNamespaceChanged(original, mutated); changed != tc.expect {
+		if changed, updated := routerNamespaceChanged(mutated, desired); changed != tc.expect {


To my thinking, the names were fine before: routerNamespaceChanged has parameters current and expected, and its logic copies current and updates it using expected. In actual use, current is the object that's in the API, and expected is the new desired object, which mutates the current API object based on some update (changes to the ingresscontroller, or changes to the operator itself as in the case of these new annotations and labels). I don't think the order really matters here though.

In actual use, current is the object that's in the API, and expected is the new desired object

Right, the only caveat being in the case of the ingress namespace resource, desired is effectively manifests.RouterNamespace. I think that's where the prior naming was throwing me off.

If you don't mind, I would prefer to use the new naming and ordering in this PR, since IMO, it makes the test added in this PR simpler.

Miciah · 2021-05-12T18:38:50Z

Thanks!
/lgtm

openshift-ci · 2021-05-12T18:39:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, sgreene570

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~~ [Miciah,sgreene570]

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

openshift-ci · 2021-05-12T18:39:52Z

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

Bugzilla bug 1954330 has been moved to the MODIFIED state.

In response to this:

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation

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.

sgreene570 · 2021-05-18T16:58:00Z

/cherry-pick release-4.7

openshift-cherrypick-robot · 2021-05-18T16:58:47Z

@sgreene570: #611 failed to apply on top of branch "release-4.7":

Applying: ingress: Fix up openshift-ingress namespace reconciliation
Using index info to reconstruct a base tree...
M	pkg/operator/controller/ingress/namespace.go
A	pkg/operator/controller/ingress/namespace_test.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/operator/controller/ingress/namespace_test.go deleted in HEAD and modified in ingress: Fix up openshift-ingress namespace reconciliation. Version ingress: Fix up openshift-ingress namespace reconciliation of pkg/operator/controller/ingress/namespace_test.go left in tree.
Auto-merging pkg/operator/controller/ingress/namespace.go
CONFLICT (content): Merge conflict in pkg/operator/controller/ingress/namespace.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 ingress: Fix up openshift-ingress namespace reconciliation
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.7

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 bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label May 11, 2021

openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 11, 2021

openshift-ci bot requested review from candita and danehans May 11, 2021 17:39

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2021

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 11, 2021

openshift-ci bot requested a review from asood-rh May 11, 2021 17:40

sgreene570 force-pushed the bz-1954330 branch from e0b9ef9 to 7d469a7 Compare May 11, 2021 18:12

sgreene570 force-pushed the bz-1954330 branch from 7d469a7 to d96c028 Compare May 11, 2021 18:17

sgreene570 force-pushed the bz-1954330 branch from d96c028 to 0faf81c Compare May 11, 2021 18:18

Miciah reviewed May 11, 2021

View reviewed changes

openshift-ci bot assigned Miciah May 12, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2021

openshift-merge-robot merged commit 285df81 into openshift:master May 12, 2021

sgreene570 mentioned this pull request May 18, 2021

[release-4.7] Bug 1960776: Reconcile openshift-ingress namespace on upgrade #616

Merged

Miciah mentioned this pull request Nov 22, 2022

OCPBUGS-2793: deployment: ignore defaultmode for volumes, accept unsolicited containers openshift/external-dns-operator#176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

sgreene570 commented May 11, 2021 •

edited

openshift-ci bot commented May 11, 2021

sgreene570 commented May 11, 2021

openshift-ci bot commented May 11, 2021

Miciah commented May 11, 2021

sgreene570 commented May 11, 2021

openshift-ci bot commented May 11, 2021

openshift-ci bot commented May 11, 2021

sgreene570 commented May 11, 2021

Miciah May 11, 2021

sgreene570 May 11, 2021 •

edited

Miciah commented May 12, 2021

openshift-ci bot commented May 12, 2021

openshift-ci bot commented May 12, 2021

sgreene570 commented May 18, 2021

openshift-cherrypick-robot commented May 18, 2021

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

Bug 1954330: ingress: Fix up openshift-ingress namespace reconciliation #611

Conversation

sgreene570 commented May 11, 2021 • edited

openshift-ci bot commented May 11, 2021

sgreene570 commented May 11, 2021

openshift-ci bot commented May 11, 2021

Miciah commented May 11, 2021

sgreene570 commented May 11, 2021

openshift-ci bot commented May 11, 2021

openshift-ci bot commented May 11, 2021

sgreene570 commented May 11, 2021

Miciah May 11, 2021

Choose a reason for hiding this comment

sgreene570 May 11, 2021 • edited

Choose a reason for hiding this comment

Miciah commented May 12, 2021

openshift-ci bot commented May 12, 2021

openshift-ci bot commented May 12, 2021

sgreene570 commented May 18, 2021

openshift-cherrypick-robot commented May 18, 2021

sgreene570 commented May 11, 2021 •

edited

sgreene570 May 11, 2021 •

edited