-
Notifications
You must be signed in to change notification settings - Fork 637
feat(server): add a storage-aware GET /readiness endpoint #3221
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
Open
SebastianGruza
wants to merge
4
commits into
apache:master
Choose a base branch
from
SebastianGruza:feat/server-readiness
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d0f4a25
feat(server): add a storage-aware GET /readiness endpoint
SebastianGruza c9e70b1
feat(server): keep addresses out of the readiness body, single-flight…
SebastianGruza 213fd9c
feat(server): simplify the readiness probe (BasicThreadFactory, no un…
SebastianGruza 4a4aa1c
feat(server): exempt /readiness from load shedding, keep channels on …
SebastianGruza File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
67 changes: 67 additions & 0 deletions
67
...aph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/ReadinessAPI.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * 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: 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). 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)); | ||
| 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(); | ||
| } | ||
| } |
130 changes: 130 additions & 0 deletions
130
...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/StorageReadiness.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| /* | ||
| * 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.ArrayList; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.apache.hugegraph.HugeGraph; | ||
| import org.apache.hugegraph.auth.HugeGraphAuthProxy; | ||
| import org.apache.hugegraph.core.GraphManager; | ||
| import org.apache.hugegraph.util.Log; | ||
| import org.slf4j.Logger; | ||
|
|
||
| /** | ||
| * Whether this server can serve graph traffic, for a readiness probe. | ||
| * Graphs on an embedded backend are ready as soon as the REST layer answers. | ||
| * Graphs on hstore are probed through the backend's "storage_readiness" | ||
| * metadata: at least one known Store answers a cheap direct call; PD is only | ||
| * needed until the first Store list is known. The storage is shared by every | ||
| * hstore graph of the process, so one graph is probed and the result is | ||
| * reused for a short TTL to keep repeated probes cheap. The body never | ||
| * carries raw exception text, since the endpoint is unauthenticated. | ||
| */ | ||
| public final class StorageReadiness { | ||
|
|
||
| public static final String STORAGE_READINESS_META = "storage_readiness"; | ||
| public static final String BACKEND_HSTORE = "hstore"; | ||
|
|
||
| private static final Logger LOG = Log.logger(StorageReadiness.class); | ||
|
|
||
| private static volatile Map<String, Object> lastResult; | ||
| private static volatile long lastCheckedAt; | ||
|
|
||
| private StorageReadiness() { | ||
| } | ||
|
|
||
| /** One storage probe with a time budget in ms. */ | ||
| public interface Probe { | ||
|
|
||
| Map<String, Object> probe(long timeoutMs) throws Exception; | ||
| } | ||
|
|
||
| public static Map<String, Object> check(GraphManager manager, | ||
| long timeoutMs, long cacheTtlMs) { | ||
| // The graphs are auth proxies and the probe request carries no user, | ||
| // so look the graph up and probe it as the internal admin, the way | ||
| // other internal paths do; the result carries no data or addresses | ||
| List<Map<String, Object>> holder = new ArrayList<>(1); | ||
| HugeGraphAuthProxy.runAsAdmin(() -> { | ||
| HugeGraph graph = firstHstoreGraph(manager); | ||
| if (graph == null) { | ||
| Map<String, Object> body = new LinkedHashMap<>(); | ||
| body.put("ready", true); | ||
| body.put("storage", "embedded"); | ||
| body.put("reason", "no graph on a remote storage"); | ||
| holder.add(body); | ||
| return; | ||
| } | ||
| holder.add(check(t -> graph.metadata(null, STORAGE_READINESS_META, t), | ||
| timeoutMs, cacheTtlMs)); | ||
| }); | ||
| return holder.get(0); | ||
| } | ||
|
|
||
| public static synchronized Map<String, Object> check(Probe probe, long timeoutMs, | ||
| long cacheTtlMs) { | ||
| long now = System.currentTimeMillis(); | ||
| Map<String, Object> cached = lastResult; | ||
| if (cached != null && now - lastCheckedAt < cacheTtlMs) { | ||
| Map<String, Object> body = new LinkedHashMap<>(cached); | ||
| body.put("cached", true); | ||
| return body; | ||
| } | ||
| Map<String, Object> body = new LinkedHashMap<>(); | ||
| body.put("ready", false); | ||
| body.put("storage", BACKEND_HSTORE); | ||
| try { | ||
| Map<String, Object> result = probe.probe(timeoutMs); | ||
| body.putAll(result); | ||
| } catch (Throwable e) { | ||
| LOG.warn("Storage readiness probe failed", e); | ||
| body.put("ready", false); | ||
| body.put("reason", "probe failed: " + e.getClass().getSimpleName()); | ||
| } | ||
| body.put("cached", false); | ||
| lastResult = body; | ||
| lastCheckedAt = System.currentTimeMillis(); | ||
| return new LinkedHashMap<>(body); | ||
| } | ||
|
|
||
| public static boolean isReady(Map<String, Object> body) { | ||
| return Boolean.TRUE.equals(body.get("ready")); | ||
| } | ||
|
|
||
| public static synchronized void resetCache() { | ||
| lastResult = null; | ||
| lastCheckedAt = 0L; | ||
| } | ||
|
|
||
| private static HugeGraph firstHstoreGraph(GraphManager manager) { | ||
| for (String name : manager.graphs()) { | ||
| try { | ||
| HugeGraph graph = manager.graph(name); | ||
| if (graph != null && BACKEND_HSTORE.equals(graph.backend())) { | ||
| return graph; | ||
| } | ||
| } catch (Throwable e) { | ||
| LOG.debug("Skip graph {} while looking for a remote storage", name, e); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| } |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readinessis added to the auth and path whitelists but not toLoadDetectFilter.WHITE_API_LIST("",apis,metrics,versions). So/readinessgoes through the worker-load and free-memory checks, before the result cache is reached, and gets a 503 oncemax_worker_threads - 1other requests are in flight.restserver.max_worker_threadsdefaults to2 * 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
/versionsto/readinesscan lose every endpoint during a spike while storage is fine.Could
readinessgo intoWHITE_API_LISTnext toversions(LoadReleaseFilterreads the same list, so the counter stays balanced), with a case liketestFilter_WhiteListPathIgnored? If shedding load through readiness is intended, please say so in theReadinessAPIJavadoc.There was a problem hiding this comment.
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.
readinessis inLoadDetectFilter.WHITE_API_LISTnext toversions;LoadReleaseFilterreads the same list, so theworkLoadcounter stays balanced.testFilter_ReadinessIgnoredLikeVersionsinLoadDetectFilterTest: with a 2-thread limit and one request in flight the filter lets/readinessthrough without touching the counter and without a log entry, in the same shape astestFilter_WhiteListPathIgnored. Readiness is not meant to shed load; theReadinessAPIJavadoc says it answers from the storage state.