Bug 1891625: Support changing ingresscontroller load balancer scope #472

Miciah · 2020-10-11T07:17:31Z

Add support for changing ingresscontroller load balancer scope. On AWS and IBM cloud, this requires deleting the existing load balancer service and recreating it with the desired scope. On Azure and GCP, it suffices to update the existing service's annotations.

This change also adds logic to remove the service finalizer, which is no longer needed to clean up DNS records since the DNSRecord CRD was added.

pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Update scope if needed.
pkg/operator/controller/ingress/load_balancer_service.go (externalLBAnnotations): New variable. Map platform type to the annotation for that platform that makes the load balancer external, if the platform requires an explicit annotation.
(ensureLoadBalancerService): Add delete and update logic. Use the new deleteLoadBalancerServiceFinalizer method to delete any finalizer on any existing service. Use the new createLoadBalancerService, deleteLoadBalancerService, and updateLoadBalancerService methods to create, update, or delete the service as needed.
(desiredLoadBalancerService): Delete logic to add a finalizer to the service. Use new externalLBAnnotations variable to simplify logic.
(finalizeLoadBalancerService): Refactor to use the new deleteLoadBalancerServiceFinalizer method.
(createLoadBalancerService, deleteLoadBalancerService): New methods.
(updateLoadBalancerService): New method. Update the LoadBalancer service, using the new loadBalancerServiceChanged function.
(loadBalancerServiceScopeChanged): New function. Check if the load balancer's scope changed.
(loadBalancerServiceChanged): New function. Check if the current service needs to be updated, and if so, whether it needs to be modified or deleted and recreated.
(deleteLoadBalancerServiceFinalizer): New method. Delete any finalizer from the service.
pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceChanged): New test.
(TestLoadBalancerServiceChangedScopeNeedsRecreate): New test.
test/e2e/operator_test.go (isServiceInternal): New function. Return a Boolean value indicating whether the provided service is annotated to request an internal load-balancer.
(TestScopeChange): New test. Verify that mutating scope from the default external scope to internal scope and from internal back to external succeeds on AWS, Azure, GCP, and IBM Cloud.

Based on #394.

Miciah · 2020-10-11T21:27:58Z

/test e2e-azure

pkg/operator/controller/ingress/load_balancer_service.go

Miciah · 2020-10-13T18:39:42Z

Latest push adds new deleteLoadBalancerService and updateLoadBalancerService methods, improves logging when it is necessary to delete and recreate the service, and fixes an error in the update logic that would cause unnecessary recreation of the service for changes unrelated to scope.

ironcladlou · 2020-10-13T18:56:19Z

pkg/operator/controller/ingress/load_balancer_service.go

+		if currentAnnotations[name] != expectedAnnotations[name] {
+			scopeChanged = true
+		}
+	}


At this point, it seems that if the needsRecreate determination were moved here and if scopeChanged && needsRecreate you could return immediately, bypassing all the irrelevant diffing logic for the case when an update is possible.

Hmm, that makes sense. It would incur a second update if the user modified the scope and another field, but scope is really the only thing we expect users to modify at this time.

Latest push implements something along these lines.

Hmm, that makes sense. It would incur a second update if the user modified the scope and another field, but scope is really the only thing we expect users to modify at this time.

This is something I hadn't considered. Given that scenario I would expect the scope change to be batched with all other possible changes. I retract my suggestion

I don't feel too strongly about it... but since you retracted the suggestion, I've reverted the change. * grin *.

(But I kept the change where I factored loadBalancerServiceScopeChanged out of loadBalancerServiceChanged: https://github.com/openshift/cluster-ingress-operator/compare/395121e5f4daa1fb4d3bc4a8145aa0324627dba1..328e6f49ceae326c0ca8e3c7d34535787d00a36f)

openshift-ci-robot · 2020-10-13T23:26:53Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`328e6f4`	link	`/test e2e-aws`
ci/prow/e2e-aws-operator	`328e6f4`	link	`/test e2e-aws-operator`

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.

Miciah · 2020-10-14T15:28:07Z

The ingress operator logs from the last CI run appear to be incomplete— the test output indicates that the operator created resources for the ingresscontroller named "scope", but the logs do not mention the ingresscontroller. (Or did some recent change break logging?) Let's see if another CI run gets us some useful logs.
/test e2e-aws-operator

Add support for changing ingresscontroller load balancer scope. On AWS and IBM cloud, this requires deleting the existing load balancer service and recreating it with the desired scope. On Azure and GCP, it suffices to update the existing service's annotations. This commit also adds logic to remove the service finalizer, which is no longer needed to clean up DNS records since the DNSRecord CRD was added. * pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Update scope if needed. * pkg/operator/controller/ingress/load_balancer_service.go (externalLBAnnotations): New variable. Map platform type to the annotation for that platform that makes the load balancer external, if the platform requires an explicit annotation. (ensureLoadBalancerService): Add delete and update logic. Use the new deleteLoadBalancerServiceFinalizer method to delete any finalizer on any existing service. Use the new createLoadBalancerService, deleteLoadBalancerService, and updateLoadBalancerService methods to create, update, or delete the service as needed. (desiredLoadBalancerService): Delete logic to add a finalizer to the service. Use new externalLBAnnotations variable to simplify logic. (finalizeLoadBalancerService): Refactor to use the new deleteLoadBalancerServiceFinalizer method. (createLoadBalancerService, deleteLoadBalancerService): New methods. (updateLoadBalancerService): New methods. Update the LoadBalancer service, using the new loadBalancerServiceChanged function. (loadBalancerServiceScopeChanged): New function. Check if the load balancer's scope changed. (loadBalancerServiceChanged): New function. Check if the current service needs to be updated, and if so, whether it needs to be modified or deleted and recreated. (deleteLoadBalancerServiceFinalizer): New method. Delete any finalizer from the service. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceChanged): New test. (TestLoadBalancerServiceChangedScopeNeedsRecreate): New test. * test/e2e/operator_test.go (isServiceInternal): New function. Return a Boolean value indicating whether the provided service is annotated to request an internal load-balancer. (TestScopeChange): New test. Verify that mutating scope from the default external scope to internal scope and from internal back to external succeeds on AWS, Azure, GCP, and IBM Cloud. Co-authored-by: Dan Mace <ironcladlou@gmail.com>

Miciah · 2020-10-20T23:33:24Z

/retest

gregsheremeta · 2020-10-21T13:15:23Z

which Jira is this related to?

Miciah · 2020-10-21T21:42:25Z

which Jira is this related to?

https://issues.redhat.com/browse/NE-172

Miciah · 2020-10-26T21:35:28Z

Linking a BZ to facilitate backports.
/retitle Bug 1891625: Support changing ingresscontroller load balancer scope

openshift-ci-robot · 2020-10-26T21:35:50Z

@Miciah: This pull request references Bugzilla bug 1891625, 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)

Details

In response to this:

Bug 1891625: Support changing ingresscontroller load balancer scope

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 · 2020-10-26T21:45:32Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2020-10-26T21:45:33Z

@Miciah: 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.

Details

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.

sgreene570

Looks good!

/lgtm

cc @dustman9000

pkg/operator/controller/ingress/load_balancer_service.go

pkg/operator/controller/ingress/load_balancer_service_test.go

openshift-ci-robot · 2020-10-28T18:02:08Z

[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

Details

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-robot · 2020-10-28T19:18:15Z

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

openshift/cluster-ingress-operator#472

Bugzilla bug 1891625 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1891625: Support changing ingresscontroller load balancer scope

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 · 2020-10-28T19:18:32Z

@Miciah: new pull request created: #482

Details

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-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2020

openshift-ci-robot requested review from ironcladlou and knobunc October 11, 2020 07:17

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2020

Miciah force-pushed the mutable-publishing-scope branch 2 times, most recently from 0483460 to 6cd8ed8 Compare October 11, 2020 07:58

sgreene570 reviewed Oct 13, 2020

View reviewed changes

pkg/operator/controller/ingress/load_balancer_service.go Outdated Show resolved Hide resolved

pkg/operator/controller/ingress/load_balancer_service.go Show resolved Hide resolved

Miciah force-pushed the mutable-publishing-scope branch from 6cd8ed8 to 395121e Compare October 13, 2020 18:39

ironcladlou reviewed Oct 13, 2020

View reviewed changes

Miciah force-pushed the mutable-publishing-scope branch 3 times, most recently from e608f60 to 328e6f4 Compare October 13, 2020 20:10

Miciah force-pushed the mutable-publishing-scope branch from 328e6f4 to 20c3d84 Compare October 14, 2020 18:21

Miciah force-pushed the mutable-publishing-scope branch from 20c3d84 to 982682f Compare October 14, 2020 20:50

Miciah changed the title ~~WIP: Support changing ingresscontroller load balancer scope~~ Support changing ingresscontroller load balancer scope Oct 20, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2020

dustman9000 mentioned this pull request Oct 20, 2020

Support Updating Existing IngressController openshift/cloud-ingress-operator#118

Closed

Miciah mentioned this pull request Oct 21, 2020

Support changing ingresscontroller load balancer scope #394

Closed

openshift-ci-robot changed the title ~~Support changing ingresscontroller load balancer scope~~ Bug 1891625: Support changing ingresscontroller load balancer scope Oct 26, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Oct 26, 2020

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

Miciah mentioned this pull request Oct 28, 2020

WIP: Set publishNotReadyAddresses to true on LB services #480

Closed

sgreene570 reviewed Oct 28, 2020

View reviewed changes

pkg/operator/controller/ingress/load_balancer_service.go Show resolved Hide resolved

pkg/operator/controller/ingress/load_balancer_service_test.go Show resolved Hide resolved

openshift-ci-robot assigned sgreene570 Oct 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2020

openshift-merge-robot merged commit 3fbc377 into openshift:master Oct 28, 2020

openshift-cherrypick-robot mentioned this pull request Oct 28, 2020

[release-4.6] Bug 1891626: Support changing ingresscontroller load balancer scope #482

Merged

Miciah mentioned this pull request Dec 4, 2020

Bug 1904582: Assume ingresscontroller is external absent status #502

Merged

Miciah mentioned this pull request Dec 15, 2020

Bug 1906560: Revert "Support changing ingresscontroller load balancer scope" #514

Merged

sgreene570 mentioned this pull request Feb 18, 2021

NE-518 GCP: Implement GCP Internal LB Global Access option #550

Merged

3 tasks

Miciah mentioned this pull request Mar 27, 2021

NE-621: Support changing ingresscontroller load balancer scope #582

Merged

Miciah mentioned this pull request May 16, 2021

Bug 1914127: Delete the ingress.openshift.io/operator finalizer #549

Merged

Bug 1891625: Support changing ingresscontroller load balancer scope #472

Bug 1891625: Support changing ingresscontroller load balancer scope #472

Uh oh!

Conversation

Miciah commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah commented Oct 11, 2020

Uh oh!

Uh oh!

Uh oh!

Miciah commented Oct 13, 2020

Uh oh!

ironcladlou Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Miciah Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Miciah Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

ironcladlou Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Miciah Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Miciah Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 13, 2020

Uh oh!

Miciah commented Oct 14, 2020

Uh oh!

Miciah commented Oct 20, 2020

Uh oh!

gregsheremeta commented Oct 21, 2020

Uh oh!

Miciah commented Oct 21, 2020

Uh oh!

Miciah commented Oct 26, 2020

Uh oh!

openshift-ci-robot commented Oct 26, 2020

Uh oh!

Miciah commented Oct 26, 2020

Uh oh!

openshift-cherrypick-robot commented Oct 26, 2020

Uh oh!

sgreene570 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 28, 2020

Uh oh!

openshift-ci-robot commented Oct 28, 2020

Uh oh!

openshift-cherrypick-robot commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Miciah commented Oct 11, 2020 •

edited

Loading