consul: prevent re-registration churn by correctly comparing connect sidecar tags #9330
Conversation
shoenig
force-pushed
the
b-connect-registration-bad-tags
branch
from
3c411b1
to
659af8a
Compare
| return true | ||
| } | ||
|
|
||
| for i, valueA := range a { |
There was a problem hiding this comment.
this returns different if the same tags, in a different order.
there's an argument that we should consider the canonical (sorted) form, allowing users to changed the order without re-registering (which may be the case, for example, if the tags are procedurally generated in a way that doesn't guarantee consistent ordering).
there's another argument that says if they change the order of the tags on a job update, we'll reregister the service (with the tags in exactly the order they specified).
There was a problem hiding this comment.
Consul itself persists tags in the order given, and at least for now I think Nomad should should maintain what the user provides.
|
as @jazzyfresh , some of the failing tests look sidecar/tag related; don't know whether they're flaky, fragile, or protective. |
…tags
Previously, connect sidecars would be re-registered with consul every cycle
of Nomad's reconciliation loop around Consul service registrations. This is
because part of the comparison used `reflect.DeepEqual` on []string objects,
which returns false when one object is `[]string{}` and the other is `[]string{}(nil)`.
Unforunately, this was always the case, and every Connect sidecar service
would be re-registered on every iteration, which happens every 30 seconds.
shoenig
force-pushed
the
b-connect-registration-bad-tags
branch
from
659af8a
to
685caee
Compare
|
Yeah sorry the tests were broken because there was a flipped condition. Yay for comprehensive tests! |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Previously, connect sidecars would be re-registered with consul every cycle
of Nomad's reconciliation loop around Consul service registrations. This is
because part of the comparison used
reflect.DeepEqualon[]stringobjects,which returns false when one object is
[]string{}and the other is[]string{}(nil).Unfortunately, this was always the case, and every Connect sidecar service
would be re-registered on every iteration, which happens every 30 seconds.
At the very least exacerbates #9307