Skip to content
Open
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public class AuthenticationFilter implements ContainerRequestFilter, ContainerRe
private static final AntPathMatcher MATCHER = new AntPathMatcher();
private static final Set<String> FIXED_WHITE_API_SET = ImmutableSet.of(
"versions",
"readiness",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ readiness is added to the auth and path whitelists but not to LoadDetectFilter.WHITE_API_LIST ("", apis, metrics, versions). So /readiness goes through the worker-load and free-memory checks, before the result cache is reached, and gets a 503 once max_worker_threads - 1 other requests are in flight. restserver.max_worker_threads defaults to 2 * CPUS, so on a 2-CPU pod the probe fails while 3 other requests run.

Under sustained load Kubernetes then drops busy Servers from the Service and shifts their traffic to the rest, so a deployment that moves its readiness probe from /versions to /readiness can lose every endpoint during a spike while storage is fine.

Could readiness go into WHITE_API_LIST next to versions (LoadReleaseFilter reads the same list, so the counter stays balanced), with a case like testFilter_WhiteListPathIgnored? If shedding load through readiness is intended, please say so in the ReadinessAPI Javadoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 4a4aa1c, thanks, that would have been a nasty interaction: a readiness probe shed under load would pull exactly the busiest Servers out of the Service while the storage is healthy. readiness is in LoadDetectFilter.WHITE_API_LIST next to versions; LoadReleaseFilter reads the same list, so the workLoad counter stays balanced. testFilter_ReadinessIgnoredLikeVersions in LoadDetectFilterTest: with a 2-thread limit and one request in flight the filter lets /readiness through without touching the counter and without a log entry, in the same shape as testFilter_WhiteListPathIgnored. Readiness is not meant to shed load; the ReadinessAPI Javadoc says it answers from the storage state.

"openapi.json"
);
/** Remove auth/login API from whitelist */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ public class LoadDetectFilter implements ContainerRequestFilter {
"",
"apis",
"metrics",
"versions"
"versions",
"readiness"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ This whitelist entry makes LoadDetectFilter.filter() return before it increments WorkLoad or enforces the worker-load and low-memory limits. /readiness is unauthenticated, and StorageReadiness.check() makes concurrent callers wait synchronously on one in-flight probe for up to readiness.timeout (60 seconds); during slow storage, a burst can occupy the REST worker pool and starve graph requests. Please cap concurrent readiness waiters and reject excess calls quickly while preserving one probe under load. Evidence: exact-head static trace through LoadDetectFilter.filter() and StorageReadiness.check().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. New option readiness.max_waiters (default 16, range 1..10000): at most that many callers wait at once for the probe in flight, each still bounded by its own readiness.timeout; the rest get an immediate 503 with the reason too many readiness callers waiting for the probe (N) and hold no worker. One probe under load stays. Test testExcessWaitersAreRejectedAtOnce (cap 1: the owner, one waiter, a third caller rejected in under 500 ms, the slot free again afterwards).

);

// Call gc every 30+ seconds if memory is low and request frequently
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public class PathFilter implements ContainerRequestFilter {
"apis",
"metrics",
"versions",
"readiness",
"health",
"gremlin",
"graphs/auth",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with this
* work for additional information regarding copyright ownership. The ASF
* licenses this file to You under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance with the
* License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package org.apache.hugegraph.api.profile;

import java.util.Map;

import org.apache.hugegraph.api.API;
import org.apache.hugegraph.config.HugeConfig;
import org.apache.hugegraph.config.ServerOptions;
import org.apache.hugegraph.core.GraphManager;
import org.apache.hugegraph.util.JsonUtil;

import com.codahale.metrics.annotation.Timed;

import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.annotation.security.PermitAll;
import jakarta.inject.Singleton;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.Response;

/**
* Storage-aware readiness for Kubernetes and load balancers: on hstore 200
* while at least one known Store answers this server, 503 while none does
* (or, before any Store list is known, while PD does not answer); on hbase
* 200 while the cluster answers an admin call within the budget. Unauthenticated, like
* /versions, so that an httpGet probe needs no credential; the body carries
* no addresses and no raw exception text.
*/
@Path("readiness")
@Singleton
@Tag(name = "ReadinessAPI")
public class ReadinessAPI extends API {

@GET
@Timed
@Produces(APPLICATION_JSON_WITH_CHARSET)
@PermitAll
public Response get(@Context GraphManager manager, @Context HugeConfig conf) {
Map<String, Object> body = StorageReadiness.check(
manager, conf.get(ServerOptions.READINESS_TIMEOUT),
conf.get(ServerOptions.READINESS_CACHE_TTL),
conf.get(ServerOptions.READINESS_MAX_WAITERS));
Response.Status status = StorageReadiness.isReady(body) ?
Response.Status.OK :
Response.Status.SERVICE_UNAVAILABLE;
return Response.status(status)
.type(APPLICATION_JSON_WITH_CHARSET)
.entity(JsonUtil.toJson(body))
.build();
}
}
Loading
Loading