-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix for router with dynamic config changes not reloading #20559
Fix for router with dynamic config changes not reloading #20559
Conversation
/retest |
route := createBlueprintRoute(routeapi.TLSTerminationEdge) | ||
route.Name = fmt.Sprintf("%v-1", route.Name) | ||
route.Name = fmt.Sprintf("%v-temp-%d", route.Name, time.Now().Unix()) |
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.
s/%v/%s/ ?
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.
can't we use the route UUID and resource version instead of timestamp?
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.
Sounds nice to me, what do you think @ramr?
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.
Sure that will work too - will do.
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, ok so weirdly I don't have that UUID and resource version.
Here's a log dump from the code in the router image:
W0807 04:27:03.848930 1 manager.go:665] dummy blueprint route = &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:_blueprint-edge-route-temp-1533616023 GenerateName: Namespace:_hapcm_blueprint_pool SelfLink: UID: ResourceVersion: Generation:0 CreationTimestamp:0001-01-01 00:00:00 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[] OwnerReferences:[] Initializers:nil Finalizers:[] ClusterName:} Spec:{Host: Path: To:{Kind: Name:_hapcm_blueprint_pool.svc Weight:0xc421bd4a7c} AlternateBackends:[] Port:<nil> TLS:0xc421722120 WildcardPolicy:} Status:{Ingress:[]}}
And I just realized why - this is not a real
route object in etcd per se - it is just a prototype
route created by the config manager to force the router to allocate a pre-allocated pool of routes.
So can't use that - will do the s#%v#%s#
fix though.
Edited typos
simulate an actual change otherwise router commit doesn't call reload. And address @mfojtik review comments. fixes bugz #1612019
97713f0
to
7ad74e0
Compare
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.
/lgtm
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, pravisankar, ramr 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:
Approvers can indicate their approval by writing |
/retest |
we need to simulate an actual change otherwise router commit doesn't call reload (as it detects no diffs), so use a generated name and cleanup after.
fixes bugz #1612019
/cc @openshift/sig-network-edge