Bug 1995330: Cherry-pick of per-service loadbalancers by squeed · Pull Request #666 · openshift/ovn-kubernetes

squeed · 2021-08-18T20:11:39Z

This is a cherry-pick of ovn-kubernetes/ovn-kubernetes#2294

This removes all of the complexity around managing vip membership.
Instead, it creates one or more load balancers per service.
Additionally, it tags those load balancers with the GroupKind and
NamespacedName of the "owning" Kubernetes object, so we can easily
find stale objects and reconcile.

It includes an idempotent load-balance reconciliation engine, so we can
generate just the ideal state in the ServiceController and let the
reconciler take care of the details. This includes a read-through cache,
to minimize unnecessary reads from the database.

A much-simpler repair engine handles cleaning up any stale Load_Balancer
objects on startup.

This also implements session affinity, which is just an option on the
Load_Balancer object.

This removes all of the complexity around managing vip membership. Instead, it creates one or more load balancers per service. Additionally, it tags those load balancers with the GroupKind and NamespacedName of the "owning" Kubernetes object, so we can easily find stale objects and reconcile. It includes an idempotent load-balance reconciliation engine, so we can generate just the ideal state in the ServiceController and let the reconciler take care of the details. This includes a read-through cache, to minimize unnecessary reads from the database. A much-simpler repair engine handles cleaning up any stale Load_Balancer objects on startup. This also implements session affinity, which is just an option on the Load_Balancer object. Signed-off-by: Casey Callendrello <cdc@redhat.com>

openshift-ci · 2021-08-18T20:22:32Z

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

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (anusaxen@redhat.com), skipping review request.

Details

In response to this:

Bug 1995330: Cherry-pick of per-service loadbalancers

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.

go-controller/pkg/ovn/loadbalancer/loadbalancer.go

trozet · 2021-08-18T21:59:05Z

go-controller/pkg/ovn/master.go

-		if err != nil {
-			return fmt.Errorf("failed to init local gateway with no OVS bridge: %v", err)
-		}
+		// do nothing, now that we have the services controller.


i think you can just remove the entire if statement now right? This was only for 4.5->4.6 and i think SDN->OVN migration. It's not applicable anymore as SDN->OVN migration uses a different method and doesnt rely on using "none" for the bridge name.

@pliurh can correct me if I'm wrong.

Oh interesting - I wonder if I can just revert #281 entirely?

If so I'll file a separate PR first.

Yeah, the SDN migration doesn't rely on #281 anymore.

It's no longer needed; we do the right thing regardless now.

It turns out it's broken. We can revert this when rhbz 1995326 is fixed and picked up.

trozet · 2021-08-20T20:09:16Z

/lgtm

trozet · 2021-08-20T20:09:54Z

/retest-required

trozet · 2021-08-20T20:10:23Z

dualstack is a known failure

/override ci/prow/e2e-metal-ipi-ovn-dualstack

openshift-ci · 2021-08-20T20:11:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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-08-20T20:11:45Z

@trozet: Overrode contexts on behalf of trozet: ci/prow/e2e-metal-ipi-ovn-dualstack

Details

In response to this:

dualstack is a known failure

/override ci/prow/e2e-metal-ipi-ovn-dualstack

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.

trozet · 2021-08-20T22:00:01Z

/retest-required

trozet · 2021-08-20T23:37:58Z

/override ci/prow/e2e-metal-ipi-ovn-dualstack

trozet · 2021-08-20T23:38:11Z

/retest-required

openshift-ci · 2021-08-20T23:38:17Z

@trozet: Overrode contexts on behalf of trozet: ci/prow/e2e-metal-ipi-ovn-dualstack

Details

In response to this:

/override ci/prow/e2e-metal-ipi-ovn-dualstack

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.

trozet · 2021-08-21T01:27:46Z

Latest local gw job failures have nothing to do with this pr

/override ci/prow/e2e-aws-ovn-local-gateway

openshift-ci · 2021-08-21T01:28:21Z

@trozet: Overrode contexts on behalf of trozet: ci/prow/e2e-aws-ovn-local-gateway

Details

In response to this:

Latest local gw job failures have nothing to do with this pr

/override ci/prow/e2e-aws-ovn-local-gateway

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.

trozet · 2021-08-21T01:35:54Z

/retest-required

dcbw · 2021-08-21T02:05:27Z

/test okd-images

dcbw · 2021-08-21T02:06:23Z

/test okd-images

dcbw · 2021-08-21T02:07:46Z

/override ci/prow/okd-images
The images exist for OCP since prow/images worked; apparently OKD mirrors are borked.

openshift-ci · 2021-08-21T02:08:23Z

@dcbw: Overrode contexts on behalf of dcbw: ci/prow/okd-images

Details

In response to this:

/override ci/prow/okd-images
The images exist for OCP since prow/images worked; apparently OKD mirrors are borked.

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.

trozet · 2021-08-21T04:09:52Z

/retest-required

openshift-ci · 2021-08-21T06:27:25Z

@squeed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/okd-e2e-gcp-ovn	`f303076`	link	`/test okd-e2e-gcp-ovn`
ci/prow/e2e-openstack-ovn	`f303076`	link	`/test e2e-openstack-ovn`
ci/prow/e2e-ovn-hybrid-step-registry	`f303076`	link	`/test e2e-ovn-hybrid-step-registry`
ci/prow/e2e-azure-ovn	`f303076`	link	`/test e2e-azure-ovn`
ci/prow/e2e-gcp-ovn-upgrade	`f303076`	link	`/test e2e-gcp-ovn-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.

dcbw · 2021-08-21T12:31:07Z

/retest-requires

dcbw · 2021-08-21T12:44:27Z

/retest-required

openshift-ci · 2021-08-21T14:01:41Z

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

openshift/ovn-kubernetes#666

Bugzilla bug 1995330 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1995330: Cherry-pick of per-service loadbalancers

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-08-23T07:31:45Z

Many many thanks, @trozet @dcbw for pushing this over the line ❤️

This commit replace updstream commits a6cd666 7e78874 7e78874 Since we merged openshift#666 Downstream before the commits from ovn-kubernetes/ovn-kubernetes#2136 were merged. Signed-off-by: astoycos <astoycos@redhat.com>

This commit replaces upstream commits a6cd666 7e78874 7e78874 Since we merged openshift#666 Downstream before the commits from ovn-kubernetes/ovn-kubernetes#2136 were merged. Signed-off-by: astoycos <astoycos@redhat.com>

openshift-ci bot requested review from JacobTanenbaum and danwinship August 18, 2021 20:13

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

squeed changed the title ~~Cherry-pick of per-service loadbalancers~~ Bug 1995330: Cherry-pick of per-service loadbalancers Aug 18, 2021

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 18, 2021

squeed requested review from astoycos and trozet and removed request for JacobTanenbaum and danwinship August 18, 2021 20:24

dcbw reviewed Aug 18, 2021

View reviewed changes

go-controller/pkg/ovn/loadbalancer/loadbalancer.go Show resolved Hide resolved

trozet reviewed Aug 18, 2021

View reviewed changes

squeed added 2 commits August 19, 2021 21:58

Remove local-to-shared load-balancer OCP hack.

e32886e

It's no longer needed; we do the right thing regardless now.

HACK: disable skip_snat for load balancers.

f303076

It turns out it's broken. We can revert this when rhbz 1995326 is fixed and picked up.

ardaguclu mentioned this pull request Aug 20, 2021

Bug 1996097: Skip renamed dualstack tests in OVNKubernetes openshift/origin#26414

Merged

openshift-ci bot assigned trozet Aug 20, 2021

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

openshift-merge-robot merged commit 8dc564d into openshift:master Aug 21, 2021

Comments

Conversation

squeed commented Aug 18, 2021

Uh oh!

openshift-ci bot commented Aug 18, 2021

Uh oh!

Uh oh!

trozet Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

squeed Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

squeed Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

pliurh Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

openshift-ci bot commented Aug 20, 2021

Uh oh!

openshift-ci bot commented Aug 20, 2021

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

trozet commented Aug 20, 2021

Uh oh!

openshift-ci bot commented Aug 20, 2021

Uh oh!

trozet commented Aug 21, 2021

Uh oh!

openshift-ci bot commented Aug 21, 2021

Uh oh!

trozet commented Aug 21, 2021

Uh oh!

dcbw commented Aug 21, 2021

Uh oh!

dcbw commented Aug 21, 2021

Uh oh!

dcbw commented Aug 21, 2021

Uh oh!

openshift-ci bot commented Aug 21, 2021

Uh oh!

trozet commented Aug 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

dcbw commented Aug 21, 2021

Uh oh!

dcbw commented Aug 21, 2021

Uh oh!

openshift-ci bot commented Aug 21, 2021

Uh oh!

squeed commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

openshift-ci bot commented Aug 21, 2021 •

edited

Loading