-
Notifications
You must be signed in to change notification settings - Fork 222
[WIP] OCPBUGS-1689: Clear route admission status on route and namespace labels change #849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] OCPBUGS-1689: Clear route admission status on route and namespace labels change #849
Conversation
|
@gcs278: This pull request references Jira Issue OCPBUGS-1689, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
|
Skipping CI for Draft Pull Request. |
57baeef to
db6b9ab
Compare
c51a3e3 to
07baa05
Compare
|
@gcs278: This pull request references Jira Issue OCPBUGS-1689, which is invalid:
Comment DetailsIn response to this:
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. |
|
/refresh jira |
|
/jira refresh |
|
@gcs278: This pull request references Jira Issue OCPBUGS-1689, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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. |
ac63a2b to
57bdd89
Compare
… pkg/operator/controller/route-status/route_status.go
57bdd89 to
9b79503
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
797d7bb to
161f283
Compare
gcs278
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the very detailed review.
| namespacesSet.Insert(namespaceList.Items[i].Name) | ||
| namespacesInShard, err := routestatus.GetNamespacesSelectedByIngressController(r.cache, ingressController) | ||
| if err != nil { | ||
| return reconcile.Result{}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I agree, fixed via custom error type.
| // New creates the route status controller. This is the controller that handles reconciling route status and keeping | ||
| // it in sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return nil, err | ||
| } | ||
| // Add watch for changes in Route | ||
| if err := c.Watch(source.NewKindWithCache(&routev1.Route{}, cache), &handler.EnqueueRequestForObject{}); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea so we actually talked something very similar in slack:
Grant Spence
[5:51 PM]
Hey Miciah, when it comes to adding a control loop to clear route status if a route label changed, is it worth adding route.status.labels to the API like we did with IngressControllers? So that we can detect only when labels change on a route? Otherwise, I reconcile any route object updates and check if they are still admitted to their status-claimed ICs
Miciah
[6:01 PM]
You mean change the route API?
Grant Spence
[6:01 PM]
yep, as we did with the ingress controller api
Miciah
[6:02 PM]
It seems heavy-handed. That's a lot of extra status (and status updates).
Grant Spence
[6:03 PM]
Yea I don't disagree
I'm assuming this filtering would require keeping state via route status. It's an admirable thing to do, but is going to require more churn with APIs.
Do you have an idea on how to do this without updating the API? I mean, we could keep state in memory, with a map of routes-to-route-labels and namespaces-to-namespace-labels, but I'm a bit ambivalent on whether it's worth it.
I feel like namespaces don't change often. And for routes, though they may get updated more often than namespaces (with status changes), we are reconciling that individual route (that involves a GET for each IC it's admitted to, and if that IC as a namespace selector, a GET for a namespace, I've since updated it use a cache). It would add efficiency, but not sure if it's worth the maintenance burden.
What are your thoughts?
|
|
||
| routeList := routev1.RouteList{} | ||
| if err := r.cache.List(context.Background(), &routeList, client.InNamespace(ns.Name)); err != nil { | ||
| log.Error(err, "failed to list routes for namespace", "related", obj.GetSelfLink()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
|
|
||
| for _, route := range routeList.Items { | ||
| log.Info("queueing route", "name", route.Name, "related", obj.GetSelfLink()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| routes: routev1.RouteList{ | ||
| Items: []routev1.Route{ | ||
| *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), | ||
| *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great catch. I fixed this to exclusively test namespaceSelectors, and exposed a bug. https://github.com/openshift/cluster-ingress-operator/pull/849/files/d0e9ac4aac19304711f167a15390119f7673f404..d62a15ffe9788b63891f65e5d2ad2bc5bf407a1b#diff-f45bfd10ac3726fa96fa3ebc79b5d614b2d0b00943f33a1132fb963885021c15R119 wasn't working when route selectors were Nil, it was just clearing everything, since nothing ever matched a Nil Route Selector (should default to true if Nil).
I know this came up before, Arkadeep exposed it when doing the route-metrics controller and I think you and I specifically talked about this. So, it looks like this is currently a minor bug in the code. It only happens when you change a namespace or route selector, and the routeSelector is Nil, it will clear ALL routes admitted to that IC, once. I think it's a minor bug because the routes will just get re-admitted, but is unneeded churn.
| routes: routev1.RouteList{ | ||
| Items: []routev1.Route{ | ||
| *newRouteWithLabelWithAdmittedStatuses("foo-route", "ns", "shard-label", "foo-ic", "bar-ic"), | ||
| *newRouteWithLabelWithAdmittedStatuses("bar-route", "ns2", "", "baz-ic", "foo-ic", "bar-ic"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this wasn't what I wanted actually, I don't want any route labels or route selectors, so removed this.
| if err := client.List(context.Background(), &actualRoutes); err == nil { | ||
| for _, expectedRoute := range tc.expectedRoutes.Items { | ||
| // Find the actual route that we should compare status with this expected route | ||
| var actualRoute routev1.Route | ||
| for _, route := range actualRoutes.Items { | ||
| if route.Name == expectedRoute.Name { | ||
| actualRoute = route | ||
| } | ||
| } | ||
| assert.Equal(t, expectedRoute.Status, actualRoute.Status, "route name", expectedRoute.Name) | ||
| } | ||
| } else { | ||
| t.Errorf("error retrieving all from client: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| // newIngressControllerWithSelectors returns a new ingresscontroller with the specified name, | ||
| // routeSelectors, and namespaceSelectors based on the parameters. | ||
| func newIngressControllerWithSelectors(name, routeMatchLabel, routeMatchExpression, namespaceMatchLabel, namespaceMatchExpression string) *operatorv1.IngressController { | ||
| ingresscontroller := operatorv1.IngressController{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: operatorcontroller.DefaultOperatorNamespace, | ||
| }, | ||
| } | ||
| if len(routeMatchLabel) != 0 { | ||
| ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{ | ||
| "type": routeMatchLabel, | ||
| }, | ||
| } | ||
| } | ||
| if len(routeMatchExpression) != 0 { | ||
| ingresscontroller.Spec.RouteSelector = &metav1.LabelSelector{ | ||
| MatchExpressions: []metav1.LabelSelectorRequirement{ | ||
| { | ||
| Key: "type", | ||
| Operator: metav1.LabelSelectorOpIn, | ||
| Values: []string{routeMatchExpression}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| if len(namespaceMatchLabel) != 0 { | ||
| ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{ | ||
| "type": namespaceMatchLabel, | ||
| }, | ||
| } | ||
| } | ||
| if len(namespaceMatchExpression) != 0 { | ||
| ingresscontroller.Spec.NamespaceSelector = &metav1.LabelSelector{ | ||
| MatchExpressions: []metav1.LabelSelectorRequirement{ | ||
| { | ||
| Key: "type", | ||
| Operator: metav1.LabelSelectorOpIn, | ||
| Values: []string{namespaceMatchExpression}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| return &ingresscontroller | ||
| } | ||
|
|
||
| // newNamespace returns a new namespace with the specified name | ||
| // and if label exists, with that label | ||
| func newNamespace(name, label string) *corev1.Namespace { | ||
| namespace := corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
| if len(label) != 0 { | ||
| namespace.ObjectMeta.Labels = map[string]string{"type": label} | ||
| } | ||
| return &namespace | ||
| } | ||
|
|
||
| // newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. | ||
| func newRouteWithLabelWithAdmittedStatuses(name, namespace, label string, icAdmitted ...string) *routev1.Route { | ||
| route := newRouteWithAdmittedStatuses(name, icAdmitted...) | ||
| route.ObjectMeta.Namespace = namespace | ||
| if len(label) != 0 { | ||
| route.Labels = map[string]string{ | ||
| "type": label, | ||
| } | ||
| } | ||
| return route | ||
| } | ||
|
|
||
| // newRouteWithAdmittedStatuses returns a new route that is admitted by ingress controllers. | ||
| func newRouteWithAdmittedStatuses(name string, icAdmitted ...string) *routev1.Route { | ||
| route := routev1.Route{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
| if len(icAdmitted) != 0 { | ||
| for _, ic := range icAdmitted { | ||
| route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{ | ||
| RouterName: ic, | ||
| Conditions: []routev1.RouteIngressCondition{ | ||
| { | ||
| Type: routev1.RouteAdmitted, | ||
| Status: "True", | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
| return &route | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, after doing a lot of unit tests, I just started using pointers in all of my test cases, because some functions we are testing take pointers, and we can test Nil parameters. These functions don't need to return pointers, just was a force of habit, and felt like it didn't hurt since a function in the future might need pointers.
Oh yes, I do really like that pattern. Let me see if I can replicate.
| t.Errorf("error ingress controller route: %v", err) | ||
| } | ||
| } | ||
| for _, route := range tc.routes.Items { | ||
| if err := client.Create(context.Background(), &route); err != nil { | ||
| t.Errorf("error creating route: %v", err) | ||
| } | ||
| } | ||
| for _, ns := range tc.namespace.Items { | ||
| if err := client.Create(context.Background(), &ns); err != nil { | ||
| t.Errorf("error creating ns: %v", err) | ||
| } | ||
| } | ||
|
|
||
| if errs := ClearRoutesNotAdmittedByIngress(client, tc.ingressController); tc.expectedErr && len(errs) == 0 { | ||
| t.Errorf("expected errors, got no errors") | ||
| } else if !tc.expectedErr && len(errs) != 0 { | ||
| t.Errorf("did not expected errors: %v", errs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
/hold
|
|
@gcs278: This pull request references Jira Issue OCPBUGS-1689, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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. |
|
My commits are pretty messy I think better to squash: |
|
From QE side, after rebel the namespace, the route was not admitted anymore. % oc get route router-loadbalancer-test -oyaml
% oc label namespace test type=opt-out --overwrite % oc get route router-loadbalancer-test -oyaml
/label qe-approved |
|
/unassign I withdraw myself from the review because the PR has enough of quality reviews already made. |
|
PR needs rebase. DetailsInstructions 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. |
|
Still holding. Going to push through #869, which is a simpler PR that introduces the builder pattern that these unit tests need. |
|
@gcs278: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Marking back to WIP. This went stale. I think we need to rethink status and status clearing. |
|
/payload ? |
|
Closing in favor of a router-specific implementation which is much cleaner (in my humble opinion): openshift/router#514 |
|
@gcs278: This pull request references Jira Issue OCPBUGS-1689. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
OCPBUGS-1689: Add the route-status control loop for clearing route admitted status when labels change on routes and namespaces. Refactored route_status.go and functions to be organized under the route-status directory. Added unit testing for route_status.go with the addition of the fake_client.go to simplify future creation of fake clients for unit testing.
pkg/operator/controller/ingress/controller.go: Moving functions from/to route_status.go.pkg/operator/controller/route-metrics/controller.go: De-duplicate logic between route-status and route-metric.pkg/operator/controller/route-status/controller.go: New control loop that reconciles route status if a route or namespace label changes.pkg/operator/controller/route-status/controller_test.go: Unit testing for the new control loop using fake client.pkg/operator/controller/route-status/router_status.go: Moved from pkg/operator/controller/ingress to be co-located with route status control loop.pkg/operator/controller/route-status/router_status_test.go: New route_status.go unit testing using fake clients.pkg/operator/operator.go: Start the new route_status control loop and share the cache with the route_metric control loop.test/e2e/router_status_test.go: E2E test for route status control loop.test/unit/fake_client.go: Added common fake client initialization functions.test/unit/utils.go: Added IngressController, Namespace, and Route builder functions to for reusable helper functions.