Copilot commented on code in PR #13345:
URL: https://github.com/apache/cloudstack/pull/13345#discussion_r4144525123
##########
framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletAdapter.java:
##########
@@ -84,18 +85,23 @@ protected String getServiceEndpointName(String strPeer) {
if (mshost == null)
return null;
- return composeEndpointName(mshost.getServiceIP(),
mshost.getServicePort());
+ return composeEndpointName(mshost, mshost.getServicePort());
}
@Override
public int getServicePort() {
return _clusterServicePort;
}
- private String composeEndpointName(String nodeIP, int port) {
- StringBuffer sb = new StringBuffer();
-
sb.append("https://";).append(nodeIP).append(":").append(port).append("/clusterservice");
- return sb.toString();
+ private String composeEndpointName(ManagementServerHostVO msHost, int
port) {
+ final String hostName = msHost.getName();
+ final String identifier = hostName != null ? hostName :
msHost.getServiceIP();
Review Comment:
A blank hostname is treated as usable because only `null` is checked. Legacy
or partially populated `mshost` rows can therefore produce
`https://:port/clusterservice` even when `serviceIP` is present; use a
non-blank check before falling back to the IP.
##########
utils/src/main/java/com/cloud/utils/backoff/impl/ExponentialWithJitterBackoff.java:
##########
@@ -0,0 +1,168 @@
+// 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 com.cloud.utils.backoff.impl;
+
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.NumbersUtil;
+import com.cloud.utils.backoff.BackoffAlgorithm;
+import com.cloud.utils.backoff.BackoffFactory;
+import com.cloud.utils.component.AdapterBase;
+
+import java.security.SecureRandom;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Exponential backoff with up/down cycling.
+ * Delay grows exponentially until a maximum, then decreases back to base,
then repeats.
+ *
+ * @author mprokopchuk
+ */
+public class ExponentialWithJitterBackoff extends AdapterBase implements
BackoffAlgorithm,
+ ExponentialWithJitterBackoffMBean {
+
+ /**
+ * Property name for the minimal delay to be used either by {@code
agent.properties} file or by configuration key.
+ */
+ public static final String MIN_DELAY_MS_CONFIG_KEY =
"backoff.min_delay_ms";
+
+ /**
+ * Property name for the maximal delay to be used either by {@code
agent.properties} file or by configuration key.
+ */
+ public static final String MAX_DELAY_MS_CONFIG_KEY =
"backoff.max_delay_ms";
+
+ /**
+ * Default value for minimal delay for the property {@link
ExponentialWithJitterBackoff#MIN_DELAY_MS_DEFAULT}.
+ */
+ public static final int MIN_DELAY_MS_DEFAULT = 5_000;
+
+ /**
+ * Default value for maximal delay for the property {@link
ExponentialWithJitterBackoff#MAX_DELAY_MS_DEFAULT}.
+ */
+ public static final int MAX_DELAY_MS_DEFAULT = 15_000;
+
+ private final Map<String, Thread> asleep = new ConcurrentHashMap<>();
+ private final Random random = new SecureRandom();
+
+ private int minDelayMs;
+ private int maxDelayMs;
+ private int maxAttempts;
+ private int attemptNumber;
+ private boolean increasing;
+
+ @Override
+ public void waitBeforeRetry() {
+ boolean interrupted = false;
+ long waitMs = getTimeToWait();
+ Thread current = Thread.currentThread();
+ try {
+ asleep.put(current.getName(), current);
+ logger.debug("Going to sleep for {}",
DateUtil.formatMillis(waitMs));
+ Thread.sleep(waitMs);
+ logger.debug("Sleep done for {}", DateUtil.formatMillis(waitMs));
+ } catch (InterruptedException e) {
+ interrupted = true;
+ logger.info("Thread {} interrupted while waiting for retry",
current.getName(), e);
+ } finally {
+ asleep.remove(current.getName());
+ calculateNextAttempt();
+ if (interrupted) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ }
+
+ /**
+ * Calculates next attempt and direction.
+ */
+ private void calculateNextAttempt() {
+ if (increasing) {
+ int nextAttemptNumber = attemptNumber + 1;
+ increasing = getNextDelay() <= maxDelayMs && nextAttemptNumber <=
maxAttempts;
+ if (increasing) {
+ attemptNumber = nextAttemptNumber;
+ }
+ } else {
+ int nextAttemptNumber = Math.max(attemptNumber - 1, 0);
+ increasing = nextAttemptNumber == 0;
+ if (!increasing) {
+ attemptNumber = nextAttemptNumber;
+ }
+ }
+ }
+
+ @Override
+ public void reset() {
+ attemptNumber = 0;
+ }
+
+ @Override
+ public Map<String, String> getConfiguration() {
+ Map<String, String> configuration = new HashMap<>();
+ configuration.put(MIN_DELAY_MS_CONFIG_KEY, String.valueOf(minDelayMs));
+ configuration.put(MAX_DELAY_MS_CONFIG_KEY, String.valueOf(maxDelayMs));
+ configuration.put(BackoffFactory.BACKOFF_IMPLEMENTATION_KEY,
this.getClass().getName());
+ return configuration;
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) {
+ minDelayMs = NumbersUtil.parseInt((String)
params.get(MIN_DELAY_MS_CONFIG_KEY), MIN_DELAY_MS_DEFAULT);
+ maxDelayMs = NumbersUtil.parseInt((String)
params.get(MAX_DELAY_MS_CONFIG_KEY), MAX_DELAY_MS_DEFAULT);
+ maxAttempts = (int) Math.round(Math.log((double) maxDelayMs /
minDelayMs) / Math.log(2));
+
+ attemptNumber = random.nextInt(maxAttempts + 1);
+ increasing = random.nextBoolean();
+ // do nothing
+ return true;
+ }
+
+ @Override
+ public Collection<String> getWaiters() {
+ return asleep.keySet();
+ }
+
+ @Override
+ public boolean wakeup(String threadName) {
+ Thread th = asleep.get(threadName);
+ if (th != null) {
+ th.interrupt();
+ return true;
+ }
+ return false;
+ }
+
+ private long getNextDelay() {
+ return (long) Math.min(minDelayMs * Math.pow(2, attemptNumber),
maxDelayMs);
+ }
+
+ @Override
+ public long getTimeToWait() {
+ long delay = Math.max(1L, getNextDelay());
+ int jitterBound = (int) Math.max(1L, delay / 2);
+ int jitter = random.nextInt(jitterBound);
+ return delay + jitter;
+ }
+
+ @Override
+ public void setTimeToWait(long seconds) {
+ // ignore
+ }
Review Comment:
The MBean exposes `setTimeToWait`, but this implementation silently ignores
every value. An operator can successfully invoke the JMX setter and believe the
retry delay changed while `getTimeToWait()` remains controlled by the
exponential configuration. Either implement the setter's documented management
behavior or remove it from this MBean contract.
##########
utils/src/main/java/org/apache/cloudstack/threadcontext/ThreadContextUtil.java:
##########
@@ -0,0 +1,116 @@
+// 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.cloudstack.threadcontext;
+
+import com.cloud.utils.StringUtils;
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.ThreadContext;
+
+import java.lang.reflect.Type;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Utility class, helps to propagate {@link ThreadContext} values from parent
to child threads.
+ *
+ * @author mprokopchuk
+ */
+public class ThreadContextUtil {
+ private static final Logger logger =
LogManager.getLogger(ThreadContextUtil.class);
+
+ public static final String MDC_UUID_KEY = "uuid";
+ public static final String MDC_LOG_CONTEXT_ID_KEY = "logcontextid";
+ public static final String CONTEXT_UUID_KEY = "uuid";
+ public static final String CONTEXT_LOG_ID_KEY = "logid";
+
+ /**
+ * Wrap {@link Runnable} to propagate {@link ThreadContext} values.
+ *
+ * @param delegate
+ * @return
+ */
+ public static Runnable wrapThreadContext(Runnable delegate) {
+ @SuppressWarnings("unchecked")
+ Map<String, String> context = ThreadContext.getContext() != null ?
+ new HashMap<>(ThreadContext.getContext()) : null;
+
+ return () -> {
+ @SuppressWarnings("unchecked")
+ Map<String, String> oldContext = ThreadContext.getContext() !=
null ?
+ new HashMap<>(ThreadContext.getContext()) : null;
+ try {
+ ThreadContext.clearMap();
+ if (context != null) {
+ context.forEach(ThreadContext::put);
+ }
+ delegate.run();
+ } finally {
+ ThreadContext.clearMap();
+ if (oldContext != null) {
+ oldContext.forEach(ThreadContext::put);
+ }
+ }
+ };
+ }
+
+ /**
+ * Set UUID in ThreadContext.
+ *
+ * @param uuid the UUID value to set
+ */
+ public static void setUuid(String uuid) {
+ if (StringUtils.isNotEmpty(uuid)) {
+ ThreadContext.put(MDC_UUID_KEY, uuid);
+ }
+ }
+
+ /**
+ * Set log context ID in ThreadContext.
+ *
+ * @param logContextId the log context ID value to set
+ */
+ public static void setLogContextId(String logContextId) {
+ if (StringUtils.isNotEmpty(logContextId)) {
+ ThreadContext.put(MDC_LOG_CONTEXT_ID_KEY, logContextId);
+ }
+ }
+
+ /**
+ * Extract UUID from JSON cmdInfo string and set it in MDC if UUID is not
already present.
+ * This is specifically used for async job processing.
+ *
+ * @param cmdInfo the JSON string containing command info
+ */
+ public static void extractAndSetUuidFromCmdInfo(String cmdInfo) {
+ if (StringUtils.isBlank((String) ThreadContext.get(MDC_UUID_KEY)) &&
StringUtils.isNotBlank(cmdInfo)) {
+ try {
+ Type mapType = new TypeToken<Map<String, String>>()
{}.getType();
+ Gson gson = new Gson();
+ Map<String, String> params = gson.fromJson(cmdInfo, mapType);
+ String entityUuid = params.get(CONTEXT_UUID_KEY);
Review Comment:
Async VM work jobs do not store `cmdInfo` as a JSON map: `VmWorkSerializer`
writes a `JobSerializerHelper` object-serialized string. Parsing every job with
Gson therefore fails for those jobs and leaves the UUID out of MDC (while
logging a warning), so the claimed UUID propagation does not cover VM async
operations. Extract the UUID through the job/VM-work serialization path or use
the persisted instance identity instead of assuming JSON.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
