fix(jibri): render only healthy shards, carry per-host ports, drop the legacy domain filter - #959
Open
aaronkvanmeerten wants to merge 2 commits into
Open
aaronkvanmeerten wants to merge 2 commits into
aaronkvanmeerten wants to merge 2 commits into
Conversation
Two defects in the ansible-rendered xmpp.conf, found while comparing it against the consul-template render. 1. configure-jibri.py queried /v1/catalog/service, which reports every registration regardless of health, so a shard that had failed its consul checks stayed in xmpp.conf until it was fully deregistered. It now queries /v1/health/service with passing=true. A small adapter reshapes the health response into the catalog shape fact_from_service() already consumes, since the health endpoint nests node and service separately and names the service metadata Meta rather than ServiceMeta. 2. The per-host port was computed in fact_from_service() from the prosody_client_port service meta and then thrown away: the grouping appended only the bare address, and xmpp.conf.j2 emitted one shared control-login port for the whole domain group. Shards grouped under one domain can be on different ports, because nomad shards in a pool share a node IP and are told apart only by port, so a single port could misdescribe a co-located shard. host_addresses entries are now "address:port". Jibri splits each entry and prefers the port it carries, falling back to control-login.port only when an entry has none, so the group port becomes a harmless fallback. Also removes a latent NameError: a service whose tagged addresses carried neither a lan nor a wan entry left private_ip and public_ip unbound. They are now seeded from the service address before any tagged address overrides them. No membership change where every shard is healthy. Verified against real consul on a stage jibri: the same nine hosts come back, now with explicit ports. The port fix is preventative, since no shard in that environment currently sets prosody_client_port.
The consul branch of configure-jibri.py kept only services whose domain equalled local_domain. That dates from when each environment had a domain of its own; they now nearly all share one, so the filter no longer separates environments and only drops the handful of standalone environments that a jibri in the pool is meant to serve. The consul-template render of the same file has never filtered by domain. Every domain registered under the environment is now taken. The environment filter on the consul query is unchanged, so this widens coverage only to domains already tagged with this jibri's environment. Measured on a stage jibri, against the same consul: the nine stage.8x8.vc hosts are unchanged and two standalone domains that were previously dropped come back, which matches both what consul reports as passing and what the consul-template render already covered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes to the ansible-rendered
xmpp.conf, found by comparing it against the consul-template render of the same file. The two renderers disagreed in both directions: ansible covered only one domain but every host in it, while consul-template covered every domain but only the first host in each.This PR fixes the ansible side. The consul-template side, which keeps only one host per domain, is a separate change.
1. Unhealthy shards were rendered
configure-jibri.pyqueried/v1/catalog/service, which reports every registration regardless of health. A shard that had failed its consul checks stayed inxmpp.confuntil it was fully deregistered, so jibri kept trying to connect to it.It now queries
/v1/health/servicewithpassing=true. A small adapter reshapes the health response into the catalog shapefact_from_service()already consumes, since the health endpoint nests node and service separately and names the service metadataMetarather thanServiceMeta. The environment filter moves with it, fromServiceMeta.environmenttoService.Meta.environment.2. The per-host port was computed and then thrown away
fact_from_service()readsprosody_client_portfrom the service meta, defaulting to 5222, and stores it ashost_port. But the grouping appended only the bare address, andxmpp.conf.j2emitted a singlecontrol-login.portfor the whole domain group:Shards grouped under one domain can be on different ports. Nomad shards in a pool share a node IP and are told apart only by port, so one shared port cannot describe them, and a new shard can reuse a retired shard's IP on a different port. Entries are now
address:port:This is what jibri already expects. From
XmppApi.kt, it creates one MucClient per host and prefers the port the entry carries:So
control-login.portbecomes a harmless fallback for entries with no port.This fix is preventative. No shard in stage currently sets
prosody_client_port, so nothing changes there today.3. The local-domain filter is obsolete
The consul branch kept only services whose domain equalled
local_domain. That dates from when each environment had a real domain of its own. They now nearly all sharestage.8x8.vc, so the filter no longer separates anything and only drops the standalone environments that a jibri in the pool is meant to serve. The consul-template render has never filtered by domain.Every domain registered under the environment is now taken. The environment filter on the query is unchanged, so coverage widens only to domains already tagged with this jibri's environment.
Also
Removes a latent
NameError: a service whose tagged addresses carried neither alannor awanentry leftprivate_ipandpublic_ipunbound. They are now seeded from the service address before any tagged address overrides them. The fallback also now prefers the service's own address over the node address, matching consul-template's.Addresssemantics, which only affects registrations with no tagged addresses.Testing
Twelve assertions against a stub consul: the endpoint switch,
passing=true, theService.Meta.environmentfilter path, two shards on one IP keeping distinct ports, all hosts retained per domain, tagged-address precedence, the former crash path, other domains surviving whilelocal_domainis set, and the rendered template output.Measured on a live stage jibri against the same consul, old script versus new:
The nine stage hosts are unchanged, ports are explicit, and the two standalone domains that were previously dropped come back. That matches what consul reports as passing, and the domain coverage matches what the consul-template render already had.
Not yet applied to a running jibri. Suggested rollout is one stage jibri, checking that
/etc/jitsi/jibri/xmpp.confgains the two domains and that jibri initializes a MucClient per host.Follow-ups, not in this PR
MapSetXkeyed on.ServiceMeta.domain), so it still covers a single host where this now lists all of them. That is the other half of the divergence.configure-jigasi.pyandconfigure-jvb.pyquery the same catalog endpoint and have the same health-filtering gap. Their templates differ enough to be worth separate changes.