OCPBUGS-36465: Delete and recreate canary route to clear spec.host by Miciah · Pull Request #1095 · openshift/cluster-ingress-operator

Miciah · 2024-06-27T03:56:14Z

Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored (see openshift/origin@54c072c). Therefore it is necessary to delete and recreate the route.

Before this change, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect.

After this change, the operator should clear spec.host, and spec.subdomain should be in effect.

Follow-up to #1047.

openshift-ci-robot · 2024-06-27T03:56:21Z

@Miciah: This pull request references Jira Issue OCPBUGS-32887, which is invalid:

expected the bug to target either version "4.17." or "openshift-4.17.", but it targets "4.16.0" instead

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:

Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored (see openshift/origin@54c072c). Therefore it is necessary to delete and recreate the route.

Before this change, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect.

After this change, the operator should clear spec.host, and spec.subdomain should be in effect.

Follow-up to #1047.

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 openshift-eng/jira-lifecycle-plugin repository.

Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored[1]. Therefore it is necessary to delete and recreate the route. 1. openshift/origin@54c072c Before this commit, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect. After this commit, the operator should clear spec.host, and spec.subdomain should be in effect. Follow-up to commit 77c61ba. This commit is related to OCPBUGS-36465. https://issues.redhat.com/browse/OCPBUGS-36465 * pkg/operator/controller/canary/route.go (updateCanaryRoute): Delete and recreate the route in order to clear spec.host. (deleteCanaryRoute): Add an options parameter for updateCanaryRoute to use. * test/e2e/canary_test.go (TestCanaryRouteClearsSpecHost): New test. Verify that the operator clears spec.host if it is set on the canary route. * test/e2e/all_test.go (TestAll): Add TestCanaryRouteClearsSpecHost as a serial test.

openshift-ci-robot · 2024-07-02T23:48:27Z

@Miciah: This pull request references Jira Issue OCPBUGS-36465, 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.17.0) matches configured target version for branch (4.17.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @melvinjoseph86

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

Details

In response to this:

Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored (see openshift/origin@54c072c). Therefore it is necessary to delete and recreate the route.

Before this change, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect.

After this change, the operator should clear spec.host, and spec.subdomain should be in effect.

Follow-up to #1047.

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 openshift-eng/jira-lifecycle-plugin repository.

candita · 2024-07-03T15:32:16Z

/assign
/assign @rfredette

rfredette · 2024-07-09T19:37:29Z

/lgtm

candita · 2024-07-10T17:28:25Z

pkg/operator/controller/canary/route.go

+		// Attempts to clear spec.host may be ignored.  Thus, to clear
+		// spec.host, it is necessary to delete and recreate the route.
+		log.Info("deleting and recreating the canary route to clear spec.host", "namespace", current.Namespace, "name", current.Name, "old spec.host", current.Spec.Host)
+		foreground := metav1.DeletePropagationForeground


DeletePropagationForeground keeps the key-value in the store until all the dependencies are also deleted (making sure there are no orphans). It's not perfectly clear to me why this is needed, but I don't expect it to cause any harm.

The godoc for the DeletePropagationForeground value is getting into the implementation details, but the godoc for the PropagationPolicy field is more to the point:

// Whether and how garbage collection will be performed. // Either this field or OrphanDependents may be set, but not both. // The default policy is decided by the existing finalizer set in the // metadata.finalizers and the resource-specific default policy. // Acceptable values are: 'Orphan' - orphan the dependents; 'Background' - // allow the garbage collector to delete the dependents in the background; // 'Foreground' - a cascading policy that deletes all dependents in the // foreground.

The important thing is that deletion happens in the foreground so that the operator can immediately recreate the route.

candita · 2024-07-10T17:42:53Z

test/e2e/canary_test.go

+	t.Log("Setting a bogus spec.host...")
+	if err := kclient.Update(context.TODO(), &canaryRoute); err != nil {
+		t.Fatal(err)
+	}


I think we need to specifically call the function updateCanaryRoute to test this. In unit test. Can be completed in a followup.

This is an end-to-end test, not a unit test. All it needs to do is the following:

Get the canary route.

Update its spec.host field.

Verify that the operator clears the field.

The fact that clearing the field requires deleting and recreating the route is irrelevant for this test. If the operator succeeds in clearing spec.host, it's good. Is there some case that this test fails to cover?

candita · 2024-07-10T17:43:50Z

pkg/operator/controller/canary/route.go

 		return false, nil
 	}

+	if len(updated.Spec.Host) == 0 && len(current.Spec.Host) != 0 {


It looks like we also need to fix the function canaryRouteChanged to return true if the Spec.Host changed, otherwise we never get here.

/hold

Update: I was mistaken on this.
/unhold

https://github.com/openshift/cluster-ingress-operator/pull/965/files hasn't been fully backported yet and I was looking in the wrong branch.

candita · 2024-07-10T18:39:08Z

/approve

openshift-ci · 2024-07-10T18:40:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

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

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

openshift-ci · 2024-07-10T22:14:57Z

@Miciah: 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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2024-07-10T22:19:31Z

@Miciah: Jira Issue OCPBUGS-36465: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#1095

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

Details

In response to this:

Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored (see openshift/origin@54c072c). Therefore it is necessary to delete and recreate the route.

Before this change, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect.

After this change, the operator should clear spec.host, and spec.subdomain should be in effect.

Follow-up to #1047.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-07-11T00:18:16Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-ingress-operator-container-v4.17.0-202407102341.p0.gf072291.assembly.stream.el9 for distgit ose-cluster-ingress-operator.
All builds following this will include this PR.

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2024

openshift-ci bot requested review from gcs278 and knobunc June 27, 2024 03:57

Miciah force-pushed the OCPBUGS-32887-delete-and-recreate-the-canary-route-to-clear-spec.host branch from 3239d99 to e3c06ab Compare June 27, 2024 03:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2024

Miciah force-pushed the OCPBUGS-32887-delete-and-recreate-the-canary-route-to-clear-spec.host branch from e3c06ab to 1ae7e8c Compare July 2, 2024 23:48

Miciah changed the title ~~OCPBUGS-32887: Delete and recreate canary route to clear spec.host~~ OCPBUGS-36465: Delete and recreate canary route to clear spec.host Jul 2, 2024

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 Jul 2, 2024

openshift-ci bot requested a review from melvinjoseph86 July 2, 2024 23:48

This was referenced Jul 2, 2024

[release-4.16] OCPBUGS-32887: Delete and recreate canary route to clear spec.host #1099

Merged

[release-4.14] OCPBUGS-36467: Allow operator to update Route spec.subdomain #1101

Merged

openshift-ci bot assigned candita and rfredette Jul 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2024

candita reviewed Jul 10, 2024

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 10, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2024

openshift-merge-bot bot merged commit f072291 into openshift:master Jul 10, 2024

gcs278 mentioned this pull request Jul 25, 2024

NE-1531: AWS Subnet Selection #1046

Merged

Conversation

Miciah commented Jun 27, 2024

Uh oh!

openshift-ci-robot commented Jun 27, 2024

Uh oh!

openshift-ci-robot commented Jul 2, 2024

Uh oh!

candita commented Jul 3, 2024

Uh oh!

rfredette commented Jul 9, 2024

Uh oh!

candita Jul 10, 2024 • 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!

Miciah Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

candita Jul 10, 2024 • 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!

Miciah Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

candita Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

candita Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

candita Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

candita commented Jul 10, 2024

Uh oh!

openshift-ci bot commented Jul 10, 2024

Uh oh!

openshift-ci bot commented Jul 10, 2024

Uh oh!

openshift-ci-robot commented Jul 10, 2024

Uh oh!

openshift-bot commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

candita Jul 10, 2024 •

edited

Loading

candita Jul 10, 2024 •

edited

Loading