Copilot commented on code in PR #13345: URL: https://github.com/apache/cloudstack/pull/13345#discussion_r4398978404
########## agent/src/main/java/com/cloud/agent/HostConnectProcess.java: ########## @@ -0,0 +1,347 @@ +// 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.agent; + +import com.cloud.agent.api.AgentConnectStatusAnswer; +import com.cloud.agent.api.AgentConnectStatusCommand; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.Command; +import com.cloud.agent.api.StartupAnswer; +import com.cloud.agent.api.StartupCommand; +import com.cloud.agent.properties.AgentProperties; +import com.cloud.agent.properties.AgentPropertiesFileHandler; +import com.cloud.agent.transport.Request; +import com.cloud.exception.CloudException; +import com.cloud.exception.OperationTimedoutException; +import com.cloud.host.Status; +import com.cloud.resource.ResourceStatusUpdater; +import com.cloud.resource.ServerResource; +import com.cloud.utils.concurrency.NamedThreadFactory; +import com.cloud.utils.nio.Link; +import org.apache.cloudstack.threadcontext.ThreadContextCommandUtil; +import org.apache.cloudstack.threadcontext.ThreadContextUtil; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.LogManager; + +import java.io.IOException; +import java.nio.channels.ClosedChannelException; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Predicate; + +public class HostConnectProcess { + private static final Logger logger = LogManager.getLogger(HostConnectProcess.class); + + public static final int DEFAULT_ASYNC_COMMAND_TIMEOUT_SEC = + AgentPropertiesFileHandler.getPropertyValue(AgentProperties.ASYNC_COMMAND_TIMEOUT_SEC); + + public static final int DEFAULT_ASYNC_STARTUP_COMMAND_TIMEOUT_SEC = + AgentPropertiesFileHandler.getPropertyValue(AgentProperties.ASYNC_STARTUP_COMMAND_TIMEOUT_SEC); + + static final long HOST_STATUS_CHECK_INITIAL_DELAY_SEC = 10; + private long hostStatusCheckDelaySec = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.AGENT_HOST_STATUS_CHECK_DELAY_SEC); + private final AtomicReference<ScheduledFuture<?>> hostStatusFutureRef = new AtomicReference<>(); + private final Agent agent; + private ScheduledExecutorService hostStatusExecutor; + + public HostConnectProcess(Agent agent) { + this.agent = agent; + initExecutors(); + } + + private void initExecutors() { + stop(); + var threadFactory = new NamedThreadFactory("Agent-" + HostStatusTask.class.getSimpleName()); + hostStatusExecutor = Executors.newScheduledThreadPool(1, threadFactory); Review Comment: This class creates a non-daemon scheduled executor, but `Agent.stop()` does not stop the `HostConnectProcess`. If the agent is shut down while a connection is pending, the status-check thread and its scheduled task remain alive and can continue issuing requests/reconnects after the agent's request handler and connection have been torn down. Add an explicit lifecycle call to stop this process during agent shutdown (and ensure the executor is closed). This issue also appears on line 193 of the same file. ########## utils/src/main/java/com/cloud/utils/backoff/BackoffAlgorithm.java: ########## @@ -35,4 +37,5 @@ public interface BackoffAlgorithm extends Adapter { */ void reset(); + Map<String, String> getConfiguration(); Review Comment: Adding this abstract method makes the public `BackoffAlgorithm` extension point source- and binary-incompatible for custom backoff implementations (including classes selected through `backoff.implementation`). Such an implementation can still be instantiated by `BackoffFactory`, but `AgentShell.setBackoffAlgorithm()` will fail when it invokes the newly required method. Preserve compatibility with a default configuration method or otherwise handle implementations compiled against the previous interface. ########## agent/src/main/java/com/cloud/agent/SynchronousListener.java: ########## @@ -0,0 +1,102 @@ +// 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.agent; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.Command; +import com.cloud.utils.Profiler; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +/** + * Blocks the thread and waits for the answer. + * + * @author mprokopchuk + */ +public class SynchronousListener implements ServerListener { + private static final Logger logger = LogManager.getLogger(SynchronousListener.class); + + protected Answer[] _answers; + protected boolean _disconnected; + private int _timeout; + + public synchronized Answer[] getAnswers() { + return _answers; + } + + @Override + public boolean isRecurring() { + return false; + } + + public synchronized boolean isDisconnected() { + return _disconnected; + } + + @Override + public synchronized boolean processAnswers(long seq, Answer[] resp) { + _answers = resp; + notifyAll(); + return true; + } + + @Override + public boolean processCommands(long seq, Command[] commands) { + return false; + } + + @Override + public synchronized boolean processDisconnect() { + logger.trace("Server disconnected. Will notify waiters"); + _disconnected = true; + notifyAll(); + return true; + } + + public synchronized Answer[] waitFor(int s) throws InterruptedException { + if (_disconnected) { + return null; + } + + if (_answers != null) { + return _answers; + } + + Profiler profiler = new Profiler(); + profiler.start(); + if (s <= 0) { + wait(); + } else { + int ms = s * 1000; + wait(ms); + } + profiler.stop(); + + logger.trace("Synchronized command - sending completed, time: {}, answer: {}", + profiler.getDurationInMillis(), _answers != null ? _answers[0].toString() : "null"); Review Comment: `ServerAttache.send` explicitly accepts and checks empty answer arrays, but this log expression dereferences element zero whenever `_answers` is non-null. An empty response (or a response with a null first entry) therefore throws `ArrayIndexOutOfBoundsException`/`NullPointerException` before the caller can handle the empty response. Guard both the array length and the element before calling `toString()`. -- 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]
