Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Miciah · 2022-03-03T18:29:30Z

OVN-Kubernetes in OpenShift 4.9 and earlier does not support externalTrafficPolicy: Local, and specifying it is reported to result in imbalanced traffic for some users. This change checks the cluster network type and sets externalTrafficPolicy: Cluster on the LoadBalancer-type service that the operator creates for ingress if the network type is "OVN-Kubernetes".

pkg/operator/controller/ingress/controller.go (ensureIngressController): Pass the network config to ensureLoadBalancerService and syncIngressControllerStatus.
pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService, loadBalancerServiceIsUpgradeable): Add a parameter for the network config. Pass the network type from the network config to desiredLoadBalancerService.
(desiredLoadBalancerService): Add a parameter for the network type. Set externalTrafficPolicy: Cluster if the network type is "OVN-Kubernetes".
pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Add a test case for OVN-Kubernetes.
pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add a parameter for the network config. Pass the config to computeIngressUpgradeableCondition.
(computeIngressUpgradeableCondition): Add a parameter for the network config. Pass the network config to loadBalancerServiceIsUpgradeable.
pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Pass the network type to desiredLoadBalancerService and the network config to computeIngressUpgradeableCondition.

This PR includes commits from #711 because we want #711 to merge first, and it includes conflicting changes. The first two commits in this PR can be ignored by reviewers.

/hold

Surya, can you verify that this change is correct? In particular, I want to be sure we're checking the right API field for the right value and that the logic behind this PR is sound at a high level.

/assign @tssurya

openshift-ci · 2022-03-03T18:29:43Z

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

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1903408 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1903408 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

Bug 2060542: Use externalTrafficPolicy: Cluster with OVN

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.

tssurya · 2022-03-03T19:19:38Z

pkg/operator/controller/ingress/load_balancer_service.go

+		// externalTrafficPolicy: Local is unsupported by OVN in
+		// OpenShift 4.9 and earlier; see
+		// <https://bugzilla.redhat.com/show_bug.cgi?id=2060542>.
+		if networkType == "OVN-Kubernetes" {


its OVNKubernetes, we should probably hardcode tha value as a constant to avoid the typo?
reference: https://github.com/openshift/api/blob/fb6f933bb8d5ce8454a8777c0c4782c193ef5674/operator/v1/types_network.go#L536

Thanks! I didn't think to check for named constants because the Spec.NetworkType field of the Network type in config/v1/types_network.go is a string type. I wonder whether the field could be changed to use the NetworkType type; I suppose if the same values are allowed, it may be all right to change the type, although it would require changes in any Go code using the Go type definitions.

tssurya · 2022-03-03T19:20:53Z

pkg/operator/controller/ingress/load_balancer_service.go

@@ -524,8 +530,8 @@ func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, *
 // return value is nil.  Otherwise, if something or someone else has modified
 // the service, then the return value is a non-nil error indicating that the
 // modification must be reverted before upgrading is allowed.
-func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus) error {
-	want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform)
+func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, networkConfig *configv1.Network) error {


@danwinship or @squeed : I have always been confused as to when to use the operv1 versus configv1 api for things. Could you please configm if using configv1 ^ is ok here? Seems like for config we support sdn and ovn: https://github.com/openshift/api/blob/d5252bac47154049f80d49eff7fec5bb642be9af/config/v1/types_network.go#L44 and for operator we support the other ones like kuryr etc..sorry for the dumb question, I probably should be knowing these things by now.

It is confusing that operator/v1/types_network.go defines these named constants, but config/v1/types_network.go just uses a string type.

@tssurya good question! In this case, it doesn't really matter, because configv1 NetworkType is inferred from configv1. Since configv1 is guaranteed to be set from cluster creation, it is slightly better to use. But it really, really, really doesn't matter.

It is confusing that operator/v1/types_network.go defines these named constants [but configv1 doesn't]

That's because you can run a cluster with a third-party network operator - so we need to support arbitrary network types in configv1. But operv1 defines what the openshift cluster network operator supports.

because configv1 NetworkType is inferred from configv1.

~~you meant from operv1?~~
(later)... I see you meant configv1 that users set! got it :)

Since configv1 is guaranteed to be set from cluster creation, it is slightly better to use. But it really, really, really doesn't matter.

ack thanks casey! makes sense. @Miciah so I think we are good here.

tssurya · 2022-03-03T19:23:58Z

thanks @Miciah for the quick fix, appreciate it!

Miciah · 2022-03-04T19:18:50Z

Bootstrap failures.
/retest

tssurya · 2022-03-07T08:53:20Z

pkg/operator/controller/ingress/load_balancer_service.go

+		// OpenShift 4.9 and earlier; see
+		// <https://bugzilla.redhat.com/show_bug.cgi?id=2060542>.
+		if networkType == string(operatorv1.NetworkTypeOVNKubernetes) {
+			service.Spec.ExternalTrafficPolicy = "Cluster"


nit: we could use the https://github.com/kubernetes/kubernetes/blob/e6c093d87ea4cbb530a7b2ae91e54c0842d8308a/pkg/apis/core/types.go#L3672 constant from upstream

Fixed. Thanks!

Miciah · 2022-03-08T21:41:19Z

Failed to launch the cluster:

level=error msg=Error: error listing tags for LB Target Group (arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-fjqjgw7q-265e5-b5vk5-sint/84bf5875e5bc1488): TargetGroupNotFound: Target groups 'arn:aws:elasticloadbalancing:us-east-1:460538899914:targetgroup/ci-op-fjqjgw7q-265e5-b5vk5-sint/84bf5875e5bc1488' not found
level=error msg=	status code: 400, request id: d43be720-0c69-49e4-9094-98e9ad332ddb
level=error
level=error msg=  on ../tmp/openshift-install-cluster-733658679/vpc/master-elb.tf line 99, in resource "aws_lb_target_group" "services":
level=error msg=  99: resource "aws_lb_target_group" "services" {
level=error
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change

/test e2e-aws-operator

Miciah · 2022-03-17T20:11:13Z

#711 merged, and I've rebased this PR; ignoring the changes from #711, the diff for this PR is exactly the same but for some line numbers (compare https://github.com/openshift/cluster-ingress-operator/commit/15202ad247d0dedaadec7c2564b60db9800f50ff.patch and https://github.com/openshift/cluster-ingress-operator/commit/7b3de002a1bf5373504d2b5dde2985cec012aecf.patch).
/hold cancel

Miciah · 2022-04-01T21:51:41Z

/hold cancel

OVN-Kubernetes in OpenShift 4.9 and earlier does not support externalTrafficPolicy: Local, and specifying it is reported to result in imbalanced traffic for some users. This commit checks the cluster network type and sets externalTrafficPolicy: Cluster on the LoadBalancer-type service that the operator creates for ingress if the network type is "OVN-Kubernetes". This commit fixes bug 2079517. https://bugzilla.redhat.com/show_bug.cgi?id=2079517 * pkg/operator/controller/ingress/controller.go (ensureIngressController): Pass the network config to ensureLoadBalancerService and syncIngressControllerStatus. * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService, loadBalancerServiceIsUpgradeable): Add a parameter for the network config. Pass the network type from the network config to desiredLoadBalancerService. (desiredLoadBalancerService): Add a parameter for the network type. Set externalTrafficPolicy: Cluster if the network type is "OVN-Kubernetes". * pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Add a test case for OVN-Kubernetes. * pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add a parameter for the network config. Pass the config to computeIngressUpgradeableCondition. (computeIngressUpgradeableCondition): Add a parameter for the network config. Pass the network config to loadBalancerServiceIsUpgradeable. * pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Pass the network type to desiredLoadBalancerService and the network config to computeIngressUpgradeableCondition.

openshift-ci · 2022-04-27T16:33:34Z

@Miciah: An error was encountered querying GitHub for users with public email (hongli@redhat.com) for bug 2079517 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#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. 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 2079517: Use externalTrafficPolicy: Cluster with OVN

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.

tssurya · 2022-04-28T10:25:40Z

/retest-required

candita · 2022-04-28T20:54:32Z

pkg/operator/controller/ingress/status_test.go

+				Spec: configv1.NetworkSpec{
+					NetworkType: "OpenShiftSDN",
+				},
+			}


Normally we would add a new variable to the test case itself, and add a couple of test cases to test the variant values.

candita · 2022-04-28T21:04:30Z

@Miciah was there any update for e2e tests for this?

candita · 2022-04-28T21:10:57Z

Error: Failed to download metadata for repo 'localdev-rhel-8-server-ose-rpms': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried
...
No match for argument: skopeo
Error: Unable to find a match: skopeo

/test e2e-gcp-serial

candita · 2022-04-28T23:46:18Z

fail [github.com/openshift/origin/test/extended/oauth/requestheaders.go:204]: unexpected error response status (401) while trying to reach '/oauth/authorize?client_id=openshift-challenging-client&response_type=token' endpoint: HTTP/1.1 401 Unauthorized
...
Referrer-Policy: strict-origin-when-cross-origin
Warning: 199 Origin "A non-empty X-CSRF-Token header is required to receive basic-auth challenges"
...
X-Frame-Options: DENY
X-Xss-Protection: 1; mode=block
A non-empty X-CSRF-Token header is required to receive basic-auth challenges
failed: (2m23s) 2022-04-28T22:33:08 "[Serial] [sig-auth][Feature:OAuthServer] [RequestHeaders] [IdP] test RequestHeaders IdP [Suite:openshift/conformance/serial]"

candita · 2022-04-28T23:47:54Z

/retest

candita · 2022-04-29T19:18:42Z

/lgtm

openshift-ci · 2022-04-29T19:21:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,candita]

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

lihongan · 2022-06-06T03:46:55Z

verified with pre-merge test process, see https://bugzilla.redhat.com/show_bug.cgi?id=2079517#c1
/label qe-approved

lihongan · 2022-06-06T03:47:15Z

/label cherry-pick-approved

openshift-ci · 2022-06-06T06:01:00Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

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-ci · 2022-06-06T06:02:07Z

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

openshift/cluster-ingress-operator#713

Bugzilla bug 2079517 has been moved to the MODIFIED state.

In response to this:

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN

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 assigned tssurya Mar 3, 2022

openshift-ci bot requested review from anuragthehatter, ironcladlou and lmzuccarelli March 3, 2022 18:29

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2022

tssurya suggested changes Mar 3, 2022

View reviewed changes

Miciah force-pushed the BZ2060542-use-externalTrafficPolicy-Cluster-with-OVN branch from a777e70 to fddf13c Compare March 3, 2022 19:51

tssurya reviewed Mar 7, 2022

View reviewed changes

Miciah force-pushed the BZ2060542-use-externalTrafficPolicy-Cluster-with-OVN branch from fddf13c to 15202ad Compare March 8, 2022 17:46

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

Miciah force-pushed the BZ2060542-use-externalTrafficPolicy-Cluster-with-OVN branch from 15202ad to 7b3de00 Compare March 17, 2022 20:05

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2022

Miciah force-pushed the BZ2060542-use-externalTrafficPolicy-Cluster-with-OVN branch from 7b3de00 to e4aeaf7 Compare April 27, 2022 16:32

Miciah changed the title ~~Bug 2060542: Use externalTrafficPolicy: Cluster with OVN~~ Bug 2079517: Use externalTrafficPolicy: Cluster with OVN Apr 27, 2022

candita reviewed Apr 28, 2022

View reviewed changes

Miciah added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Apr 29, 2022

openshift-ci bot assigned lihongan, melvinjoseph86, quarterpin, ShudiLi and candita Apr 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 6, 2022

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 6, 2022

openshift-merge-robot merged commit cb65025 into openshift:release-4.9 Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Miciah commented Mar 3, 2022

openshift-ci bot commented Mar 3, 2022

tssurya Mar 3, 2022

Miciah Mar 3, 2022

tssurya Mar 3, 2022 •

edited

Miciah Mar 3, 2022

squeed Mar 7, 2022

squeed Mar 7, 2022

tssurya Mar 7, 2022 •

edited

tssurya commented Mar 3, 2022

Miciah commented Mar 4, 2022

tssurya Mar 7, 2022

Miciah Mar 8, 2022

Miciah commented Mar 8, 2022

Miciah commented Mar 17, 2022 •

edited

Miciah commented Apr 1, 2022

openshift-ci bot commented Apr 27, 2022

tssurya commented Apr 28, 2022

candita Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 29, 2022

openshift-ci bot commented Apr 29, 2022

lihongan commented Jun 6, 2022

lihongan commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Bug 2079517: Use externalTrafficPolicy: Cluster with OVN #713

Conversation

Miciah commented Mar 3, 2022

openshift-ci bot commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssurya Mar 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssurya Mar 7, 2022 • edited

Choose a reason for hiding this comment

tssurya commented Mar 3, 2022

Miciah commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Mar 8, 2022

Miciah commented Mar 17, 2022 • edited

Miciah commented Apr 1, 2022

openshift-ci bot commented Apr 27, 2022

tssurya commented Apr 28, 2022

Choose a reason for hiding this comment

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 28, 2022

candita commented Apr 29, 2022

openshift-ci bot commented Apr 29, 2022

lihongan commented Jun 6, 2022

lihongan commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

tssurya Mar 3, 2022 •

edited

tssurya Mar 7, 2022 •

edited

Miciah commented Mar 17, 2022 •

edited