Bug 1687940: deployment: fix scope of pod anti-affinity #167

ironcladlou · 2019-03-13T22:04:55Z

Before, the anti-affinity rule for ingress controller deployment pods was such
that no controller pods could colocate on a node, even if they were for
different ingress controllers. Now, pods for different ingress controllers can
be colocated on a node.

Fixes bug https://bugzilla.redhat.com/show_bug.cgi?id=1687940.

ironcladlou · 2019-03-14T13:41:02Z

/retest

ironcladlou · 2019-03-14T14:45:30Z

Not seeing anything suspicious about the router in the logs, the flakes are concerning...

/retest

ironcladlou · 2019-03-14T15:59:26Z

/retest

danehans · 2019-03-14T16:46:12Z

/test e2e-aws-operator
/test e2e-aws

ironcladlou · 2019-03-14T20:33:36Z

/retest

ironcladlou · 2019-03-15T13:24:45Z

/retest

Miciah · 2019-03-15T16:36:11Z

Need to fix the label selectors in RouterServiceInternal and desiredLoadBalancerService.

ironcladlou · 2019-03-15T17:18:04Z

Good catch — there might be a gap here in how we construct the selector if it must be shared

ironcladlou · 2019-03-15T19:32:38Z

Well, turns out that the YAML/manifestfactory business was hiding an entanglement of the deployment pod selector (i.e. in addition to the deployment itself, the internal service also needs to match the deployment pods). So I gave the internal service the usual refactoring treatment and extracted the naming and deployment pod selector stuff into names.go.

A rotten cluster has stymied my testing, so might have to continue this one on Monday (unless it happens that the CI tests pass and there are no issues during code review).

ironcladlou · 2019-03-15T19:35:20Z

pkg/operator/controller/controller_internal_service.go

+	}
+
+	s.Spec.Selector = map[string]string{
+		s.Spec.Selector[controllerDeploymentLabel]: IngressControllerDeploymentLabel(ic),


I think this is already an improvement, but another possibility I see is to use the selector from the deployment itself here (perhaps by extracting a function into names.go which returns the deployment selector.

I went ahead and did this refactor too

ironcladlou · 2019-03-15T19:59:24Z

So here's my post-hoc justification for increasing the complexity of the PR:

The affinity setup in the manifest YAML is already pretty complex, and at the time I introduced it I thought the value would be static. However, as the bug shows, some of the affinity definition is actually dynamic as a function of the ingress controller. One option would have been to leave the static parts in YAML and mutate just the dynamic piece at runtime, and there's precedent within the codebase for that. However, based on previous conversations, I thought it would be easier to comprehend if we kept the whole affinity piece defined in code closer to the mutation. I also cleaned up the deployment selector names as part of that, which I thought would be trivial.

But, during the course of the refactor, it became clear that the deployment pod selector is used in many contexts, and the as a result I ended up having to touch the related pieces.

I'm hoping the improved clarity of coupling between these parts will justify the expanded scope of the changes. Thoughts?

Miciah

Should delete ServingCertSecretAnnotation in manifests.go.

Miciah · 2019-03-15T20:52:05Z

pkg/operator/controller/controller_internal_service.go

+)
+
+// ensureInternalRouterServiceForIngress ensures that an internal service exists
+// for a given ClusterIngress.


"ClusterIngress"→"IngressController"

Miciah · 2019-03-15T20:55:50Z

assets/router/service-internal.yaml

@@ -5,12 +5,8 @@ apiVersion: v1
 metadata:
  # Name and Annotations are set at runtime.
  namespace: openshift-ingress


Specifying namespace in the asset is redundant now that desiredInternalIngressControllerService sets it.

Miciah · 2019-03-15T21:19:06Z

pkg/operator/controller/controller_router_deployment.go

-	}
+	deployment.Spec.Selector = IngressControllerDeploymentPodSelector(ci)
+
+	deployment.Spec.Template.Labels = IngressControllerDeploymentPodSelector(ci).MatchLabels


Looks like you could even simplify this to deployment.Spec.Template.Labels = deployment.Spec.Selector.MatchLabels.

Miciah · 2019-03-15T21:25:11Z

test/e2e/operator_test.go is full of references to the old router-... names.

ironcladlou · 2019-03-18T14:03:07Z

Updated test/e2e/operator_test.go to use naming functions for the most part (and removed a redundant test).

Ready for review again.

ironcladlou · 2019-03-18T14:37:48Z

/retest

ironcladlou · 2019-03-18T16:59:51Z

/retest while I look at the failures...

ironcladlou · 2019-03-18T17:20:53Z

/retest

ironcladlou · 2019-03-18T17:49:19Z

Looks like CI account issues.

/retest

danehans · 2019-03-18T17:55:25Z

test/e2e/operator_test.go

@@ -175,69 +175,6 @@ func TestClusterIngressControllerCreateDelete(t *testing.T) {
 	}
 }

-func TestRouterServiceInternalEndpoints(t *testing.T) {


Curious to know why this test is being removed.

IMO the test isn't buying us much other than asserting the service exists, which is covered implicitly elsewhere. It did check the endpoints, but that's such a specific test you could cover with a unit (e.g. do the selectors match) and there's origin e2e coverage for metrics which actually uses the service (currently broken).

Spoke offline and decided to do a followup which introduces a new test variant which exercises the metrics endpoint through the service (which will both implicitly test the endpoints and also the actual functional area around the internal service, which is enabling metrics integration)

ironcladlou · 2019-03-19T13:29:15Z

/retest

ironcladlou · 2019-03-19T15:23:50Z

Still trying to get a cluster to try and reproduce any issues. Operator logs and router logs look fine...

/retest

ironcladlou · 2019-03-20T14:27:51Z

/retest

ironcladlou · 2019-03-20T15:43:57Z

Okay, finally reproduced some of these in a local cluster.

ironcladlou · 2019-03-20T15:47:24Z

https://github.com/openshift/origin/blob/master/test/extended/router/router.go#L112 ☹️

Before, the anti-affinity rule for ingress controller deployment pods was such that no controller pods could colocate on a node, even if they were for different ingress controllers. Now, pods for different ingress controllers can be colocated on a node. Fixes bug https://bugzilla.redhat.com/show_bug.cgi?id=1687940.

pravisankar

Minor nit, otherwise LGTM

pravisankar · 2019-03-20T19:24:10Z

assets/router/service-internal.yaml

 spec:
  type: ClusterIP
-  selector:
-    app: router


Recent changes are hinting us that we don't have much value in creating assets anymore.
Probably we should ditch assets and create objects internally in the code.

If we keep the assets, maybe we could at least use more abstract names, such as ClusterIPService, similar to how we have LoadBalancerService. I wouldn't object to removing the assets entirely though.

pravisankar · 2019-03-20T19:27:31Z

pkg/operator/controller/controller_internal_service.go

+// ensureInternalRouterServiceForIngress ensures that an internal service exists
+// for a given IngressController.
+func (r *reconciler) ensureInternalIngressControllerService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (*corev1.Service, error) {
+	desired := desiredInternalIngressControllerService(ic, deploymentRef)


This line can be moved to line 33 where we use this desired object.

Miciah

manifests.go still has an unused definition for ServingCertSecretAnnotation.

Miciah · 2019-03-20T19:34:25Z

pkg/operator/controller/controller_router_deployment.go

-		deployment.Labels = map[string]string{}
+	deployment.Labels = map[string]string{
+		// associate the deployment with the ingresscontroller
+		manifests.OwningClusterIngressLabel: ci.Name,


Should this be using IngressControllerDeploymentLabel?

Not sure, this is the same as before, and deploymentLabel is for ingress.operator.openshift.io/ingress-controller-deployment — we might need to audit our labels generallly

Meant for this comment to apply to controller_internal_service.go

And no, this was to associate the deployment with the ingresscontroller, which is the intent of the label... IngressControllerDeploymentLabel was meant to be a way to associate other stuff with the deployment itself. The naming is probably poor

What is the intended use of IngressControllerDeploymentLabel? I was assuming it was, "I have an ingresscontroller and need the label I would use to identify it."

Sorry, just saw your last comment (GitHub didn't show it when I submitted mine last comment). I see now, IngressControllerDeploymentLabel identifies the deployment, and we need a label that identifies the ingresscontroller (for which we do not yet have a function). Please disregard my preceding comments.

Miciah · 2019-03-20T19:34:56Z

pkg/operator/controller/controller_internal_service.go

+	s.Name = name.Name
+
+	s.Labels = map[string]string{
+		manifests.OwningClusterIngressLabel: ic.Name,


Should this be using IngressControllerDeploymentLabel?

I don't know if it matters since there's an owner ref to the deployment already

ironcladlou · 2019-03-20T20:18:15Z

/retest

Miciah · 2019-03-20T21:02:54Z

/lgtm

pravisankar

/lgtm

openshift-ci-robot · 2019-03-20T21:18:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah, pravisankar

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,ironcladlou,pravisankar]

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

Miciah · 2019-03-20T22:44:18Z

level=error msg="4 errors occurred:"
level=error msg="\t* module.vpc.aws_route_table_association.private_routing[2]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.private_routing.2: timeout while waiting for state to become 'success' (timeout: 5m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route_table_association.route_net[0]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.route_net.0: timeout while waiting for state to become 'success' (timeout: 5m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route_table_association.route_net[2]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.route_net.2: timeout while waiting for state to become 'success' (timeout: 5m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route_table_association.route_net[3]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.route_net.3: timeout while waiting for state to become 'success' (timeout: 5m0s)"

/test e2e-aws-operator

Miciah · 2019-03-20T23:55:18Z

level=info msg="Waiting up to 30m0s for the cluster at https://api.ci-op-qdt49y5z-43abb.origin-ci-int-aws.dev.rhcloud.com:6443 to initialize..."
level=fatal msg="failed to initialize the cluster: timed out waiting for the condition"

/test e2e-aws-operator

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 Mar 13, 2019

openshift-ci-robot requested review from knobunc and Miciah March 13, 2019 22:05

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2019

ironcladlou force-pushed the affinity-fix branch from caa6a43 to 6fce585 Compare March 13, 2019 22:10

ironcladlou changed the title ~~WIP: Bug 1687940: deployment: fix scope of pod anti-affinity~~ Bug 1687940: deployment: fix scope of pod anti-affinity Mar 14, 2019

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 Mar 14, 2019

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2019

ironcladlou commented Mar 15, 2019

View reviewed changes

Miciah reviewed Mar 15, 2019

View reviewed changes

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

ironcladlou force-pushed the affinity-fix branch from 515600a to 268eb2c Compare March 18, 2019 14:02

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

danehans reviewed Mar 18, 2019

View reviewed changes

ironcladlou force-pushed the affinity-fix branch from 268eb2c to 52875c9 Compare March 20, 2019 18:51

ironcladlou force-pushed the affinity-fix branch from 52875c9 to 8130d69 Compare March 20, 2019 19:09

pravisankar reviewed Mar 20, 2019

View reviewed changes

Miciah reviewed Mar 20, 2019

View reviewed changes

openshift-ci-robot assigned Miciah Mar 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2019

openshift-ci-robot assigned pravisankar Mar 20, 2019

pravisankar approved these changes Mar 20, 2019

View reviewed changes

openshift-merge-robot merged commit 3f228cf into openshift:master Mar 21, 2019

Bug 1687940: deployment: fix scope of pod anti-affinity #167

Bug 1687940: deployment: fix scope of pod anti-affinity #167

Conversation

ironcladlou commented Mar 13, 2019

ironcladlou commented Mar 14, 2019

ironcladlou commented Mar 14, 2019

ironcladlou commented Mar 14, 2019

danehans commented Mar 14, 2019

ironcladlou commented Mar 14, 2019

ironcladlou commented Mar 15, 2019

Miciah commented Mar 15, 2019

ironcladlou commented Mar 15, 2019

ironcladlou commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Mar 15, 2019

Miciah left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Mar 15, 2019

ironcladlou commented Mar 18, 2019

ironcladlou commented Mar 18, 2019

ironcladlou commented Mar 18, 2019

ironcladlou commented Mar 18, 2019

ironcladlou commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Mar 19, 2019

ironcladlou commented Mar 19, 2019

ironcladlou commented Mar 20, 2019

ironcladlou commented Mar 20, 2019

ironcladlou commented Mar 20, 2019 • edited

pravisankar left a comment

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

Miciah left a comment

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

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

Choose a reason for hiding this comment

ironcladlou commented Mar 20, 2019

Miciah commented Mar 20, 2019

pravisankar left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 20, 2019

Miciah commented Mar 20, 2019

Miciah commented Mar 20, 2019

ironcladlou commented Mar 20, 2019 •

edited