Copilot commented on code in PR #13345: URL: https://github.com/apache/cloudstack/pull/13345#discussion_r4368124661
########## 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(); Review Comment: The backoff instance is shared by the agent's reconnect workers, but `attemptNumber` and `increasing` are read and updated without synchronization. Concurrent `waitBeforeRetry()` calls can lose updates or calculate delays from inconsistent state, defeating the exponential backoff that is intended to prevent connection storms. Make the state transition atomic or keep attempt state per connection/retry process. ########## framework/cluster/src/main/java/com/cloud/cluster/ManagementServerAddressUtil.java: ########## @@ -0,0 +1,216 @@ +// 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.cluster; + +import org.apache.cloudstack.config.ApiServiceConfiguration; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import com.cloud.utils.net.NetUtils; + +import java.net.InetAddress; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; + +/** + * Utility class for management server address configuration detection. + */ +public class ManagementServerAddressUtil { + private static final Logger logger = LogManager.getLogger(ManagementServerAddressUtil.class); + + private static final long RESOLUTION_CACHE_TTL_MS = TimeUnit.MINUTES.toMillis(5); + private static final Map<String, ResolvedAddress> resolutionCache = new ConcurrentHashMap<>(); + + private static final class ResolvedAddress { + private final String ip; + private final long resolvedAt; + + private ResolvedAddress(String ip, long resolvedAt) { + this.ip = ip; + this.resolvedAt = resolvedAt; + } + } + + // RFC 1123 compliant hostname pattern + private static final Pattern HOSTNAME_PATTERN = Pattern.compile( + "^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])(\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9]))*$" + ); + + /** + * Validates if a string is a valid hostname according to RFC 1123. + * + * @param hostname the string to validate + * @return true if the string is a valid hostname format, false otherwise + */ + private static boolean isValidHostname(String hostname) { + if (StringUtils.isEmpty(hostname) || hostname.length() > 253) { + return false; + } + + // Check each label doesn't exceed 63 characters + String[] labels = hostname.split("\\."); + for (String label : labels) { + if (label.length() > 63) { + return false; + } + } + + return HOSTNAME_PATTERN.matcher(hostname).matches(); + } + + /** + * Detects if the management server address list configuration uses hostnames or IP addresses. + * Checks all entries in the 'host' configuration (ApiServiceConfiguration.ManagementServerAddresses) + * to determine the format. + * + * @return true if ALL addresses are valid hostnames, false otherwise (including if any are IPs or invalid) + */ + public static boolean isManagementServerAddressListUsingHostnames() { + final String msServerAddresses = ApiServiceConfiguration.ManagementServerAddresses.value(); + if (StringUtils.isEmpty(msServerAddresses)) { + return false; // Default to IP format for safety + } + + final String[] addresses = msServerAddresses.replace(" ", "").split(","); + + boolean hasHostname = false; + for (String address : addresses) { + if (StringUtils.isEmpty(address)) { + continue; + } + + // If it's an IP address, return false + if (NetUtils.isValidIp4(address) || NetUtils.isValidIp6(address)) { + return false; + } + + // If it's not a valid hostname format, return false + if (!isValidHostname(address)) { + return false; + } + + hasHostname = true; + } + + // Only treat as hostname format if at least one valid hostname was found + return hasHostname; + } + + /** + * Returns the entries of the 'host' configuration (ApiServiceConfiguration.ManagementServerAddresses) as a list, + * in the configured order. + */ + public static List<String> getConfiguredAddressList() { + final String msServerAddresses = ApiServiceConfiguration.ManagementServerAddresses.value(); + if (StringUtils.isEmpty(msServerAddresses)) { + return Collections.emptyList(); + } + final List<String> addresses = new ArrayList<>(); + for (String address : msServerAddresses.replace(" ", "").split(",")) { + if (StringUtils.isNotEmpty(address)) { + addresses.add(address); + } + } + return addresses; + } + + /** + * Convenience overload of {@link #getConfiguredAddresses(Collection, Collection)} for a single management server. + */ + public static List<String> getConfiguredAddresses(String msHostname, String msIp) { + return getConfiguredAddresses(msHostname != null ? List.of(msHostname) : List.of(), + msIp != null ? List.of(msIp) : List.of()); + } + + /** + * Returns the entries of the 'host' configuration that refer to any of the given management servers. + * Agents only know the configured entries, so lists sent to them (avoid list, maintenance list) must use + * this namespace rather than the hostnames and IPs persisted in the mshost table. + * A configured entry matches when it equals one of the persisted IPs, equals one of the persisted hostnames + * (case-insensitive) or, for a hostname alias, resolves to one of the persisted IPs. Resolutions are cached + * for {@value #RESOLUTION_CACHE_TTL_MS} ms so this can be called on hot paths such as ping answers. + * + * @param msHostnames hostnames of the management servers as persisted in the mshost table + * @param msIps service IPs of the management servers as persisted in the mshost table + * @return the matching configured entries, in the configured order; never null + */ + public static List<String> getConfiguredAddresses(Collection<String> msHostnames, Collection<String> msIps) { + final Set<String> names = new HashSet<>(); + if (msHostnames != null) { + for (String name : msHostnames) { + if (StringUtils.isNotBlank(name)) { + names.add(name.toLowerCase()); + } + } + } + final Set<String> ips = new HashSet<>(); + if (msIps != null) { + for (String ip : msIps) { + if (StringUtils.isNotBlank(ip)) { + ips.add(ip); + } + } + } + final List<String> matches = new ArrayList<>(); + if (names.isEmpty() && ips.isEmpty()) { + return matches; + } + for (String address : getConfiguredAddressList()) { + if (ips.contains(address) || names.contains(address.toLowerCase())) { + matches.add(address); + continue; + } + if (ips.isEmpty() || NetUtils.isValidIp4(address) || NetUtils.isValidIp6(address)) { + continue; + } + final String resolvedIp = resolve(address); + if (resolvedIp != null && ips.contains(resolvedIp)) { + matches.add(address); + } + } + return matches; + } + + private static String resolve(String hostname) { + final long now = System.currentTimeMillis(); + final ResolvedAddress cached = resolutionCache.get(hostname); + if (cached != null && now - cached.resolvedAt < RESOLUTION_CACHE_TTL_MS) { + return cached.ip; + } + String ip = null; + try { + ip = InetAddress.getByName(hostname).getHostAddress(); + } catch (UnknownHostException e) { + logger.debug("Unable to resolve configured management server address {}", hostname); + } + resolutionCache.put(hostname, new ResolvedAddress(ip, now)); Review Comment: When a configured alias cannot be resolved, `ip` remains null and this `ConcurrentHashMap.put` throws a NullPointerException (which is not caught). This makes `getConfiguredAddresses` fail instead of skipping the unresolvable alias, and the added `testConfiguredAddressesSkipUnresolvableAliases` exercises exactly that path. Only cache successful resolutions (or use a null-capable representation). ########## utils/src/main/java/com/cloud/utils/nio/NioConnection.java: ########## @@ -229,7 +240,18 @@ protected boolean rejectConnectionIfBlocked(final SocketChannel socketChannel) t if (!blockNewConnections) { return false; } - logger.warn("Rejecting new connection as the server is blocked from accepting new connections"); + + try { + socketChannel.shutdownOutput(); + } catch (IOException ignore) { + // ignore exception + } + try { + socketChannel.shutdownInput(); + } catch (IOException ignore) { + // ignore exception + } + logger.warn("Rejecting new connection from {} as the server is blocked from accepting new connections", socketChannel.getRemoteAddress()); Review Comment: `SocketChannel.getRemoteAddress()` is allowed to throw `IOException`, and logger argument evaluation happens before `warn` is called. If the newly accepted channel is closed during this rejection path, that exception escapes `accept()` and the selector loop catches it as fatal, stopping the NIO server before the channel is closed. Use the socket's non-throwing remote socket address or catch the lookup failure so a rejected connection cannot terminate the server. This issue also appears on line 275 of the same file. ########## 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; + } Review Comment: When the decreasing phase reaches zero, `attemptNumber` is not updated because the assignment is inside `if (!increasing)`. For any cycle that reaches an attempt above zero, the next cycle therefore starts at attempt 1 instead of the configured base delay, contradicting the documented return-to-base behavior and producing an incorrect backoff sequence. -- 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]
