Skip to content
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

ExternalBridgeSelectionStrategy #889

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package org.jitsi.jicofo.bridge

import org.jitsi.utils.logging2.Logger
import org.jitsi.utils.logging2.LoggerImpl
import org.json.simple.JSONObject
import org.json.simple.JSONValue
import java.net.URI
import java.net.http.HttpClient
import java.net.http.HttpRequest
import java.net.http.HttpResponse

@Suppress("unused")
class ExternalBridgeSelectionStrategy() : BridgeSelectionStrategy() {
private val httpClient: HttpClient = HttpClient
.newBuilder()
.connectTimeout(ExternalBridgeSelectionStrategyConfig.config.timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can import ExternalBridgeSelectionStrategyConfig.config to make these calls shorter

.build()

private val logger: Logger = LoggerImpl(ExternalBridgeSelectionStrategy::class.simpleName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use createLogger() here


private val fallbackStrategy: BridgeSelectionStrategy? by lazy {
val fallbackStrategyName = ExternalBridgeSelectionStrategyConfig.config.fallbackStrategy ?: return@lazy null
try {
val clazz = Class.forName("${javaClass.getPackage().name}.$fallbackStrategyName")
clazz.getConstructor().newInstance() as BridgeSelectionStrategy
} catch (e: Exception) {
val clazz = Class.forName(fallbackStrategyName)
clazz.getConstructor().newInstance() as BridgeSelectionStrategy
}
}

private fun fallback(
bridges: MutableList<Bridge>?,
conferenceBridges: MutableMap<Bridge, Int>?,
participantRegion: String?
): Bridge {
if (fallbackStrategy == null) {
throw Exception("External bridge selection failed and no fallbackStrategy was provided.")
}
return fallbackStrategy!!.doSelect(bridges, conferenceBridges, participantRegion)
}

override fun doSelect(
bridges: MutableList<Bridge>?,
conferenceBridges: MutableMap<Bridge, Int>?,
participantRegion: String?
): Bridge {
val url = ExternalBridgeSelectionStrategyConfig.config.url
?: throw Exception("ExternalBridgeSelectionStrategy requires url to be provided")

val requestBody = JSONObject()
requestBody["bridges"] = bridges?.map {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that you are providing the full list of bridges here. Can you make that optional (behind a config flag) please? We intend to use it with a service which already has the list of available bridges.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary as long as the index is used in the response to identify the bridge. But if the JID is used as suggested, this can even be removed entirely rather than made optional IMO (our service also knows the list of available bridges).

But I was concerned how Jicofo would react if told to use a bridge it doesn't already know about. Even if not deliberate, I can imagine when bridges start up or shut down there could be brief periods where the external selection service has a different idea of available bridges than Jicofo.

mapOf(
"jid" to it.jid.toString(),
"version" to it.version,
"colibri2" to it.supportsColibri2(),
"relay_id" to it.relayId,
"region" to it.region,
"stress" to it.stress,
"operational" to it.isOperational,
"graceful_shutdown" to it.isInGracefulShutdown,
"draining" to it.isDraining,
)
}
requestBody["conference_bridges"] = conferenceBridges?.mapKeys { it.key.jid.toString() }
requestBody["participant_region"] = participantRegion
requestBody["fallback_strategy"] = ExternalBridgeSelectionStrategyConfig.config.fallbackStrategy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you need this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were using it to signal which mode the external service should use (region based or intra region), matched to the fallback strategy Jicofo would use if it couldn't contact the external service for any reason. We're actually now passing the selected mode in the URL, so this is no longer needed. I'll remove it, absent any use case for it.


val request = HttpRequest
.newBuilder()
.POST(HttpRequest.BodyPublishers.ofString(requestBody.toJSONString()))
.uri(URI.create(url))
.headers("Content-Type", "application/json")
.timeout(ExternalBridgeSelectionStrategyConfig.config.timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work as expected when the timeout is null? You could either only call .timeout if the config has a value, or make the config value required (by config instead of by optionalconfig) and add a reasonable default value in reference.conf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, .timeout(null) will throw. I think making it required with a default in reference.conf is sensible; there is no rational reason to want to block forever...

.build()

val response: HttpResponse<String>
try {
response = httpClient.send(request, HttpResponse.BodyHandlers.ofString())
} catch (exc: Exception) {
logger.error("ExternalBridgeSelectionStrategy: HTTP request failed with ${exc}, using fallback strategy")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "ExternalBridgeSelectionStrategy" from the message, it's already in the logger context.

return fallback(bridges, conferenceBridges, participantRegion)
}

val statusCode = response.statusCode()
if (statusCode !in 200..299) {
logger.error("ExternalBridgeSelectionStrategy: HTTP request failed with ${statusCode}, using fallback strategy")
return fallback(bridges, conferenceBridges, participantRegion)
}

val responseBody: JSONObject
try {
responseBody = JSONValue.parseWithException(response.body()) as JSONObject
} catch (exc: Exception) {
logger.error("ExternalBridgeSelectionStrategy: HTTP response parsing failed with ${exc}, using fallback strategy")
return fallback(bridges, conferenceBridges, participantRegion)
}

val selectedBridgeIndex = responseBody["selected_bridge_index"] as? Number

if (selectedBridgeIndex == null) {
logger.error("ExternalBridgeSelectionStrategy: HTTP response selected_bridge_index missing or invalid, using fallback strategy")
return fallback(bridges, conferenceBridges, participantRegion)
}

val bridge = bridges!![selectedBridgeIndex.toInt()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fail early (before sending the request) if bridges is null or empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about having the external service return an ID (JID) of the selected bridge instead of an index? We want to have the service return a bridge that jicofo doesn't yet know about (this will require more changes, but it will be easier to have the HTTP API ready to support it and not have to change).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this seems sensible. Interesting use case to use a bridge it doesn't know about yet. Launching bridges on-demand?

logger.info("ExternalBridgeSelectionStrategy: participantRegion=${participantRegion}, bridge=${bridge}")
return bridge
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.jitsi.jicofo.bridge

import org.jitsi.config.JitsiConfig
import org.jitsi.metaconfig.optionalconfig
import java.time.Duration

class ExternalBridgeSelectionStrategyConfig private constructor() {
val url: String? by optionalconfig {
"${BASE}.url".from(JitsiConfig.newConfig)
}
fun url() = url

val timeout: Duration? by optionalconfig {
"${BASE}.timeout".from(JitsiConfig.newConfig)
}
fun timeout() = timeout

val fallbackStrategy: String? by optionalconfig {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's better to make this required (by config) and provide a good default value?

"${BASE}.fallback-strategy".from(JitsiConfig.newConfig)
}
fun fallbackStrategy() = fallbackStrategy

companion object {
const val BASE = "jicofo.bridge.external-selection-strategy"

@JvmField
val config = ExternalBridgeSelectionStrategyConfig()
}
}