WIP: status: Report Upgradeable status condition #508

Miciah · 2020-12-09T00:41:21Z

Add an "Upgradeable" status condition for ingresscontrollers and for the ingress clusteroperator. Compute the clusteroperator's status condition from the individual ingresscontrollers'. Compute each ingresscontroller's "Upgradeable" status by checking its load balance service (if the ingresscontroller has one). If someone or something other than the operator has modified the service, mark the ingresscontroller as not upgradeable. This ensures that the administrator cannot modify the service and then upgrade to a version of the operator that reverts the administrator's modification.

pkg/operator/controller/ingress/controller.go (Reconcile): Compute the platform status from the infrastructure config using GetPlatformStatus. Pass the platform status to ensureIngressController.
(ensureIngressController): Add a parameter for the platform status. Pass the platform status instead of the infrastructure config to ensureLoadBalancerService. Pass the deployment reference and platform status to syncIngressControllerStatus.
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): Delete the parameter for the infrastructure config. Add a parameter for the platform status. Delete redundant computation of platform status (since it is now a parameter). Delete computation of the PROXY protocol flag (desiredLoadBalancerService can compute it).
(desiredLoadBalancerService): Delete PROXY protocol parameter. Instead, compute the value using IsProxyProtocolNeeded.
(loadBalancerServiceScopeChanged): New function. Check if the load balancer scope's changed. This function is only added for consistency with the main branch.
(loadBalancerServiceChanged): New function. Check if the given load balancer services differ.
(loadBalancerServiceIsUpgradeable): New function. Check if the given load balancer service differs from the expected service for the given ingresscontroller, deployment, and platform status.
pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Refactor to be more table-driven. Change test cases to specify which annotations are expected to be present, and which are expected to be absent, in order to simplify the test logic.
(checkServiceHasAnnotation): Fix logic for when expectValue is false.
(TestLoadBalancerServiceChanged): New test.
(TestLoadBalancerServiceChangedScopeNeedsRecreate): New test.
(TestLoadBalancerServiceIsUpgradeable): New test.
pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add parameters for the deployment reference and platform status. Use these parameters and the new computeIngressUpgradeableCondition function to compute the ingresscontroller's "Upgradeable" status condition.
(computeIngressUpgradeableCondition): New function. Compute the ingresscontroller's "Upgradeable" status condition.
pkg/operator/controller/status/controller.go (Reconcile): Use the new computeOperatorUpgradeableCondition function to compute the clusteroperator's "Upgradeable" status condition.
(computeOperatorUpgradeableCondition): New function. Compute the clusteroperator's "Upgradeable" status condition from each ingresscontrollers' "Upgradeable" status condition.

Related to #507.

openshift-ci-robot · 2020-12-09T00:41:22Z

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

WIP: status: Report Upgradeable status condition

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-12-09T01:57:37Z

One of the control plane nodes failed to become ready (see nodes.json).
/test e2e-aws-operator

openshift-merge-robot · 2020-12-09T03:01:35Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`598553c`	link	`/test e2e-aws`
ci/prow/e2e-aws-operator	`598553c`	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.

sgreene570 · 2020-12-10T20:24:06Z

This depends on #507, right?

Miciah · 2020-12-10T21:24:30Z

This depends on #507, right?

Yeah. Also, I need to change the logic not to set Upgradeable=False when annotations have been modified. But before all that, I need to make a PR to add similar logic on the master branch. My plan is that 4.7 will set Upgradeable=False if the operator detects any external modifications to the service, and 4.6 will set Upgradeable=False if the operator detects any external modifications to the service other than to its annotations.

openshift-ci-robot · 2020-12-22T09:26:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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]

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

Miciah · 2020-12-22T18:49:35Z

Looks like tests passed but must-gather failed.
/test e2e-aws-operator

Miciah · 2020-12-22T20:05:35Z

must-gather failed again.
/test e2e-aws-operator

openshift-bot · 2021-03-22T23:36:32Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

* pkg/operator/controller/ingress/controller.go (Reconcile): Compute the platform status from the infrastructure config using GetPlatformStatus. Pass the platform status to ensureIngressController. (ensureIngressController): Add a parameter for the platform status. Pass the platform status instead of the infrastructure config to ensureLoadBalancerService. * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService): Delete the parameter for the infrastructure config. Add a parameter for the platform status. Delete redundant computation of platform status (since it is now a parameter). Delete computation of the PROXY protocol flag (desiredLoadBalancerService can compute it). (desiredLoadBalancerService): Delete PROXY protocol parameter. Instead, compute the value using IsProxyProtocolNeeded. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Refactor to be more table-driven. Change test cases to specify which annotations are expected to be present, and which are expected to be absent, in order to simplify the test logic. (checkServiceHasAnnotation): Fix logic for when expectValue is false.

Add an "Upgradeable" status condition for ingresscontrollers and for the ingress clusteroperator. Compute the clusteroperator's status condition from the individual ingresscontrollers'. Compute each ingresscontroller's "Upgradeable" status by checking its load balance service (if the ingresscontroller has one). If the service is missing the correct owner reference or if something or someone other than the operator has modified the service, mark the ingresscontroller as not upgradeable. This ensures that the administrator cannot modify the service and then upgrade to a version of the operator that reverts the administrator's modification. This commit is related to bug 1905490. https://bugzilla.redhat.com/show_bug.cgi?id=1905490 * assets/router/service-cloud.yaml: Add spec.sessionAffinity with the default value to simplify comparisons of desired and actual service. * pkg/manifests/bindata.go: Regenerate. * pkg/operator/controller/ingress/controller.go (ensureIngressController): Pass the deployment reference and platform status to syncIngressControllerStatus. * pkg/operator/controller/ingress/load_balancer_service.go (loadBalancerServiceExternallyModified): New function. Check if the given load balancer services differ. (loadBalancerServiceIsUpgradeable): New function. Check if the given load balancer service is missing the owner reference or differs from the expected service for the given ingresscontroller, deployment, and platform status. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceExternallyModified): New test. * pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add parameters for the deployment reference and platform status. Use these parameters and the new computeIngressUpgradeableCondition function to compute the ingresscontroller's "Upgradeable" status condition. (computeIngressUpgradeableCondition): New function. Compute the ingresscontroller's "Upgradeable" status condition. * pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): New test. * pkg/operator/controller/status/controller.go (Reconcile): Use the new computeOperatorUpgradeableCondition function to compute the clusteroperator's "Upgradeable" status condition. (computeOperatorUpgradeableCondition): New function. Compute the clusteroperator's "Upgradeable" status condition from each ingresscontrollers' "Upgradeable" status condition. * pkg/operator/controller/status/controller_test.go (TestComputeOperatorUpgradeableCondition): New test.

Ignore the "service.beta.kubernetes.io/load-balancer-source-ranges" service annotation when computing the Upgradeable status condition so that users can continue to use the annotation with OpenShift 4.8. Setting the spec.loadBalancerSourceRanges field still affects the Upgradeable status condition. This means that users who have been setting spec.loadBalancerSourceRanges will be prompted to migrate to using the "service.beta.kubernetes.io/load-balancer-source-ranges" annotation on OpenShift 4.8 before upgrading to OpenShift 4.9. In OpenShift 4.9, we will add an API field to IngressController for configuring source ranges and set Upgradeable=False if the annotation is set to prompt users to switch to the new API field, so the user will be able to migrate from the annotation to this API after upgrading to OpenShift 4.9. * pkg/operator/controller/ingress/load_balancer_service.go (loadBalancerServiceExternallyModified): Ignore the "service.beta.kubernetes.io/load-balancer-source-ranges" annotation. * pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceExternallyModified): * pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Expect the "service.beta.kubernetes.io/load-balancer-source-ranges" annotation to be ignored and not to affect the Upgradeable status condition.

sgreene570 · 2021-04-20T14:46:27Z

assets/router/service-cloud.yaml

    protocol: TCP
    port: 443
    targetPort: https
+  sessionAffinity: None


I'm curious to find out if any customers are currently setting sessionAffinity to a non-default value :)

sgreene570 · 2021-04-20T14:55:55Z

pkg/operator/controller/ingress/load_balancer_service.go

+	changed, updated := loadBalancerServiceExternallyModified(current, desired, platform)
+	if changed {
+		diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+		return fmt.Errorf("load balancer service has been modified; changes must be reverted before upgrading: %s", diff)


Great idea to ~~log~~ bubble up the diff here!

openshift-ci · 2021-04-20T16:22:20Z

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

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

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.

openshift-bot · 2021-05-20T22:47:06Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-06-20T01:34:57Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-06-20T01:35:05Z

@Miciah: PR needs rebase.

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.

openshift-ci · 2021-06-20T01:35:06Z

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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 Dec 9, 2020

openshift-ci-robot requested review from danehans and sgreene570 December 9, 2020 00:41

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

Miciah force-pushed the status-report-Upgradeable-status-condition branch from 598553c to 9d2a3cd Compare December 22, 2020 04:26

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2020

Miciah changed the base branch from release-4.6 to master December 22, 2020 04:27

Miciah force-pushed the status-report-Upgradeable-status-condition branch from 9d2a3cd to 59a8d36 Compare December 22, 2020 09:26

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2020

Miciah force-pushed the status-report-Upgradeable-status-condition branch from 59a8d36 to 385e5d6 Compare December 22, 2020 16:53

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 22, 2021

Miciah force-pushed the status-report-Upgradeable-status-condition branch from 385e5d6 to ac32762 Compare April 20, 2021 07:08

Miciah mentioned this pull request Apr 20, 2021

Bug 1950284: Annotate services of type LoadBalancer with user tags (AWS only) #578

Merged

Miciah force-pushed the status-report-Upgradeable-status-condition branch from ac32762 to 1995b03 Compare April 20, 2021 12:56

Miciah added 2 commits April 20, 2021 09:47

Miciah force-pushed the status-report-Upgradeable-status-condition branch from 1995b03 to aa24de8 Compare April 20, 2021 13:47

sgreene570 reviewed Apr 20, 2021

View reviewed changes

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 20, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2021

openshift-ci bot closed this Jun 20, 2021

WIP: status: Report Upgradeable status condition #508

WIP: status: Report Upgradeable status condition #508

Uh oh!

Conversation

Miciah commented Dec 9, 2020

Uh oh!

openshift-ci-robot commented Dec 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

openshift-merge-robot commented Dec 9, 2020

Uh oh!

sgreene570 commented Dec 10, 2020

Uh oh!

Miciah commented Dec 10, 2020

Uh oh!

openshift-ci-robot commented Dec 22, 2020

Uh oh!

Miciah commented Dec 22, 2020

Uh oh!

Miciah commented Dec 22, 2020

Uh oh!

openshift-bot commented Mar 22, 2021

Uh oh!

sgreene570 Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Apr 20, 2021 • 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!

openshift-ci bot commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented May 20, 2021

Uh oh!

openshift-bot commented Jun 20, 2021

Uh oh!

openshift-ci bot commented Jun 20, 2021

Uh oh!

openshift-ci bot commented Jun 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Miciah commented Dec 9, 2020 •

edited

Loading

sgreene570 Apr 20, 2021 •

edited

Loading

openshift-ci bot commented Apr 20, 2021 •

edited

Loading