Administrative state for routing objects #523
Conversation
chrisgresty
changed the title
|
|
||
| private const val HEALTH = "health" | ||
| const val HEALTH_SUCCESS = "success" | ||
| const val HEALTH_FAIL = "fail" |
There was a problem hiding this comment.
"Object health" is an ongoing state. But success/fail imply an outcome of some action. Therefore consider renaming tag values to healthy/unhealthy, or reachable/unreachable. Etc.
There was a problem hiding this comment.
These do indicate the outcomes of actions of a kind, although I do see your point. Perhaps the "health" name is wrong. How about healthcheck=[passing|failing]:N ?
…min-state-for-routing-objects
| .filter { isTaggedWith(it, lbGroupTag(appId)) } | ||
| .filterNot { isTaggedWith(it, "$INACTIVE_TAG.*".toRegex()) } | ||
| .filter { checkTag(it, ::lbGroupTagValue, appId) } | ||
| .filter { checkTag(it, ::stateTagValue, STATE_ACTIVE, null) } |
There was a problem hiding this comment.
(Warning, this is an opinionated comment)
.filter takes a predicate. Consider renaming checkTag to reflect this predicate, for example taggedWith(...) etc.
| is ObjectActive -> "$ACTIVE_TAG:${status.failedProbes}" | ||
| is ObjectInactive -> "$INACTIVE_TAG:${status.successfulProbes}" | ||
| } | ||
| val RELEVANT_STATES = setOf(STATE_ACTIVE, STATE_UNREACHABLE) |
There was a problem hiding this comment.
Set private visibility for RELEVANT_STATES.
| @@ -104,18 +98,18 @@ internal class HealthCheckMonitoringService( | |||
| internal fun runChecks(application: String, objectStore: StyxObjectStore<RoutingObjectRecord>) { | |||
| val monitoredObjects = discoverMonitoredObjects(application, objectStore) | |||
| .map { | |||
There was a problem hiding this comment.
Consider unpacking the Pair members like so:
map { (name, record) -> ... }
| } | ||
| .filter { (_, _, objectHealth) -> objectHealth != null } | ||
|
|
||
| val pendingHealthChecks = monitoredObjects | ||
| .map { (name, record, objectHealth) -> | ||
| healthCheck(probe, record.routingObject, objectHealth) | ||
| healthCheck(probe, record.routingObject, objectHealth!!) |
There was a problem hiding this comment.
Unnecessary non-null assertion: !!.
| } | ||
| .filter { (_, _, objectHealth) -> objectHealth != null } | ||
|
|
||
| val pendingHealthChecks = monitoredObjects | ||
| .map { (name, record, objectHealth) -> | ||
| healthCheck(probe, record.routingObject, objectHealth) | ||
| healthCheck(probe, record.routingObject, objectHealth!!) | ||
| .map { newHealth -> Triple(name, objectHealth, newHealth) } | ||
| .doOnNext { (name, currentHealth, newHealth) -> |
There was a problem hiding this comment.
Please collapse the separate map and doOnNext operators to a single doOnNext.
There was a problem hiding this comment.
I think the map is required to change the type of the pendingHealthChecks mono to the triple, which is used in the following subscriber. The doOnNext alone doesn't do this.
There was a problem hiding this comment.
Yeah I noticed that retrospectively
| Triple(it.first, it.second, health) | ||
| val tags = it.second.tags | ||
| val objectHealth = objectHealthFrom(stateTagValue(tags), healthcheckTagValue(tags)) | ||
| Triple(it.first, it.second, objectHealth) | ||
| } | ||
| .filter { (_, _, objectHealth) -> objectHealth != null } |
There was a problem hiding this comment.
My editor says that objectHealth != null is always true. Therefore this filter can be removed as useless.
There was a problem hiding this comment.
(Opinionated comment warning)
I'd consider inlining discoverMonitoredObjects. It doesn't contribute much but to hide the selection criteria out of sight. :-)
(... okay some tests would be affected)
There was a problem hiding this comment.
The tests can be removed, I think. The behaviour of the filter is pretty much self evident, especially now the tag logic is elsewhere and standardised.
| // a state where the object does not exist using compute alone. But even with ifPresent, as we are open to | ||
| // the object disappearing between the ifPresent and the compute, which would again lead to the compute creating | ||
| // a new object when we don't want it to. But at least this will happen much less frequently. | ||
| db.get(name).ifPresent { |
There was a problem hiding this comment.
Good observation. Worth raising an enhancement issue?
| override fun health() = if (failedProbes > 0) Pair(HEALTHCHECK_FAILING, failedProbes) else null | ||
| } | ||
| data class ObjectUnreachable(val successfulProbes: Int) : ObjectHealth() { | ||
| override fun state() = STATE_UNREACHABLE |
There was a problem hiding this comment.
The state() is redundant for ObjectActive and ObjectUnreachable cases. The class itself represents the object state.
There was a problem hiding this comment.
It does, and that's how it is used in the healthCheckFunction. But in the retag function, it is useful to be able to convert between the object state as an ObjectHealth object, and the state tag (and health tag) that it corresponds to. This seemed like the best place for that linkage.
.plus(stateTag(newStatus.state()))
.plus(healthcheckTag(newStatus.health()))
| * This handler is configured with the base path /admin/providers/provider-name so that it can extract the remainder | ||
| * One handler for all origins | ||
| */ | ||
| internal class OriginsAdminHandler( |
There was a problem hiding this comment.
Please use UrlPatternRouter to implement this handler. It provides most of the required functionality out of the the box:
- Match on HTTP methods
- Match on URL paths
- Extract variables from URL paths
There was a problem hiding this comment.
I completely missed that class. I'll re-implement. Thanks.
|
|
||
| scenario("Ignores closed origins") { | ||
| // TODO: Do this |
There was a problem hiding this comment.
Heads up, something still needs doing ;-)
There was a problem hiding this comment.
Ah. Yes. Thanks. :)
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
| package com.hotels.styx.api; |
There was a problem hiding this comment.
This class should move away from the API module. The API module is only for definitions necessary for 3rd party extensions.
I believe styx-proxy is the correct destination.
…min-state-for-routing-objects
| private const val STATE = "state" | ||
| const val STATE_ACTIVE = "active" | ||
| const val STATE_UNREACHABLE = "unreachable" | ||
| const val STATE_CLOSED = "closed" |
There was a problem hiding this comment.
This is really inactive that is different from closed:
- inactive: the resources are preserved but object is out of load balancer
- closed: the resources are removed and object is due to be removed for good
| * One handler for all origins | ||
| */ | ||
| internal class OriginsAdminHandler( | ||
| basePath: String, |
There was a problem hiding this comment.
Please ensure the enable/disable behaviour follows the existing behaviour described here:
https://github.com/HotelsDotCom/styx/blob/master/docs/user-guide/admin-interface.md#origin-toggle
There was a problem hiding this comment.
Created a Trello ticket to cover this work: https://trello.com/c/yO2hxOQx
|
|
||
| private val router: UrlPatternRouter | ||
|
|
||
| init { |
There was a problem hiding this comment.
This init block is unnecessary.
| private fun errorResponse(status: HttpResponseStatus, message: String) = | ||
| HttpResponse.response(status) | ||
| .disableCaching() | ||
| .addHeader(HttpHeaderNames.CONTENT_TYPE, JSON_UTF_8.toString()) |
There was a problem hiding this comment.
As per coding style we would static inline constants like CONTENT_TYPE and response.
| .flatMap { | ||
| router.handle(it, context) | ||
| .map { response -> response.stream() } | ||
| } |
There was a problem hiding this comment.
FYI: We could use HttpAggregator, instead of manually aggregating.
|
|
||
| fun getResponse(handler: OriginsAdminHandler, request: LiveHttpRequest) = | ||
| Mono.from(Mono.from(handler.handle(request, HttpInterceptorContext.create())).block().aggregate(1000)).block() | ||
|
|
There was a problem hiding this comment.
Unnecessary mono & .block(). I'd do this:
fun Eventual<LiveHttpResponse>.wait(maxBytes: Int = 100*1024, debug: Boolean = false) = this.toMono()
.flatMap { it.aggregate(maxBytes).toMono() }
.doOnNext {
if (debug) {
println("${it.status()} - ${it.headers()} - ${it.bodyAs(StandardCharsets.UTF_8)}")
}
}
.block()
There was a problem hiding this comment.
I see. You've replaced the first block() with a .flatMap() to process the aggregated HttpResponse, so you only need the final block() - there's no need to wait twice.
See #518