diff --git a/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt b/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt index a93facc858..06980ebf2a 100644 --- a/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt +++ b/jicofo-common/src/main/kotlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt @@ -42,6 +42,17 @@ class IceRestartMetrics { "Number of in-place ICE restarts for which the bridge's rotated transport was signaled to the participant." ) + /** + * The bridge answered with the transport it already had, because it did not need to restart (its own + * transport has not connected yet). Nothing was relayed and the participant keeps the session it has, so + * this is neither a completed restart nor a failed one. + */ + @JvmField + val notNeeded = metricsContainer.registerCounter( + "ice_restarts_not_needed", + "Number of in-place ICE restarts for which the bridge kept its existing transport." + ) + /** * An ICE restart did not complete: the request was rejected (disabled, rate-limited, stale bridge-session ID) * or the bridge's response could not be relayed (no transport in the response, stale generation, participant diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt index 4246602ea7..74cf358537 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt @@ -216,11 +216,16 @@ class Colibri2Session( sendRequest(request.build(), "restartIce") { response -> val bridgeTransport = response.endpoints.find { it.id == participant.id }?.transport?.iceUdpTransport if (bridgeTransport == null) { - // The bridge declined the restart (disabled in its configuration, transport not established yet, - // or shutting down). It says so by omitting the transport rather than by returning an error. + // The bridge can not restart ICE at all: restarts are disabled or misconfigured, the transport + // is stopped (the endpoint is going away), or it failed to create the new Agent. It says so by + // omitting the transport rather than by returning an error, so that one endpoint's request does + // not fail the whole conference-modify. logger.warn("No transport in the response to an ICE restart request for ${participant.id}") colibriSessionManager.endpointIceRestartFailed(participant.id) } else { + // Either the transport of the new Agent, tagged with its ice-generation, or -- when the bridge + // did not need to restart, because its transport is not established yet -- the transport of the + // Agent it already has, with no generation. See endpointIceRestarted. colibriSessionManager.endpointIceRestarted(participant.id, bridgeTransport) } } diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt index bbf9d11eab..aa1faee9fe 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt @@ -793,6 +793,11 @@ class ColibriV2SessionManager @JvmOverloads constructor( * if the participant restarts twice in quick succession the two responses can arrive in either order. The * `ice-generation` the bridge echoes tells us which round each belongs to, and anything older than what we * have already relayed is discarded. + * + * A transport with no `ice-generation` at all means something different: the bridge did not restart and + * answered with the credentials of the Agent it already has. It does this when its transport has not + * connected yet, where there is nothing to preserve and nothing for the participant to do. There is nothing + * to relay in that case, and no restart to record. */ internal fun endpointIceRestarted(endpointId: String, transport: IceUdpTransportPacketExtension) { val generation = transport.iceGeneration @@ -802,8 +807,12 @@ class ColibriV2SessionManager @JvmOverloads constructor( IceRestartMetrics.failed.inc() return } - if (generation != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && - participantInfo.lastRelayedIceGeneration != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && + if (generation == IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED) { + logger.info("ICE restart: the bridge kept its existing transport for $endpointId, not relaying.") + IceRestartMetrics.notNeeded.inc() + return + } + if (participantInfo.lastRelayedIceGeneration != IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED && generation <= participantInfo.lastRelayedIceGeneration ) { logger.info( diff --git a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt index e0cd9f1a98..88c0bfea0e 100644 --- a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt +++ b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManagerTest.kt @@ -45,7 +45,6 @@ import org.jitsi.utils.time.FakeClock import org.jitsi.xmpp.extensions.colibri2.ConferenceModifyIQ import org.jitsi.xmpp.extensions.jingle.DtlsFingerprintPacketExtension import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension -import org.jitsi.xmpp.extensions.jingle.IceUdpTransportPacketExtension.GENERATION_UNSPECIFIED import org.jivesoftware.smack.packet.IQ import org.jxmpp.jid.Jid import org.jxmpp.jid.impl.JidCreate @@ -359,15 +358,18 @@ class ColibriV2SessionManagerTest : ShouldSpec() { } } - context("With a bridge that does not tag generations") { - sessionManager.endpointIceRestarted("p1", transportWithGeneration(null)).also { drain() } + context("With a transport that carries no generation") { + // The bridge kept the Agent it already had instead of restarting, so there is nothing to relay. + sessionManager.endpointIceRestarted("p1", transportWithGeneration(2)).also { drain() } sessionManager.endpointIceRestarted("p1", transportWithGeneration(null)).also { drain() } - should("relay everything (the guard can not order untagged transports)") { - iceRestartedTransports.size shouldBe 2 - iceRestartedTransports.map { - it.second.iceGeneration - } shouldBe listOf(GENERATION_UNSPECIFIED, GENERATION_UNSPECIFIED) + should("not relay it") { + iceRestartedTransports.map { it.second.iceGeneration } shouldBe listOf(2) + } + should("not lose the generation of the last transport that was relayed") { + sessionManager.endpointIceRestarted("p1", transportWithGeneration(1)).also { drain() } + + iceRestartedTransports.map { it.second.iceGeneration } shouldBe listOf(2) } } }