Skip to content

Fix host stuck in connecting state #8502

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.

Sign up for GitHub

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

Merged
shwstppr merged 2 commits into from
Jan 15, 2024
Merged

Fix host stuck in connecting state #8502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8bf874
Fix host stuck in connecting state
vishesh92 Jan 12, 2024
a7d108c
Add wait for ModifyStoragePoolCommand cmd
vishesh92 Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ protected AgentAttache notifyMonitorsOfConnection(final AgentAttache attache, fi

final Long dcId = host.getDataCenterId();
final ReadyCommand ready = new ReadyCommand(dcId, host.getId(), NumbersUtil.enableHumanReadableSizes);
ready.setWait(60);
Copy link
Contributor

@GutoVeronezi GutoVeronezi Jan 12, 2024

Choose a reason for hiding this comment

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

These wait/timeouts should be externalized.

Copy link
Member

@weizhouapache weizhouapache Jan 12, 2024

Choose a reason for hiding this comment

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

I remember somebody complained there are too much global settings.
what's the benefit of adding another setting ? @GutoVeronezi

normally ReadyCommand is executed in less than 1 second.
60 seconds is very safe value.

Copy link
Contributor

@GutoVeronezi GutoVeronezi Jan 12, 2024

Choose a reason for hiding this comment

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

In general, hard coded "magic" numbers are not interesting; we bind the system to what we think is proper and do not allow the operators to shape the system according to their use case.

For the timeout/wait settings, we could think on a flexible mecanism to allow operators overriding the timeout/wait values in a granular way, e.g. defining a timeout/wait for each command, and fallbacking to the global setting wait in the absence of the specifc setting.

Copy link
Member

@weizhouapache weizhouapache Jan 12, 2024

Choose a reason for hiding this comment

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

@GutoVeronezi
the problem is actually caused by the falling back to the default wait which is 30 mins by default.

Copy link
Contributor

@DaanHoogland DaanHoogland Jan 15, 2024

Choose a reason for hiding this comment

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

I agree with @GutoVeronezi , but in this case if it fixes we can add a new ticket for externalizing it and merge this.

Copy link
Member

@weizhouapache weizhouapache Jan 15, 2024

Choose a reason for hiding this comment

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

@DaanHoogland @GutoVeronezi
I understand there are some operations which can take long time (migration, snapshots, etc), or some intervals (for background tasks, etc) which operators want to change.
However, for these two commands which should be processed in less than 1 second, what's the benefit of adding a new setting ?

Copy link
Member Author

@vishesh92 vishesh92 Jan 15, 2024

Choose a reason for hiding this comment

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

I am with @weizhouapache on this. We already have a lot of global settings which makes it confusing for the end user as well.

Copy link
Contributor

@shwstppr shwstppr Jan 15, 2024

Choose a reason for hiding this comment

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

@vishesh92 @DaanHoogland @GutoVeronezi @weizhouapache I'm inclined to include this fix in 4.19.0. I'm creating an improvement ticket for follow-up on externalizing the wait. Maybe we can look into some generic solution while addressing it.

Copy link
Contributor

@shwstppr shwstppr Jan 15, 2024

Choose a reason for hiding this comment

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

#8506

Copy link
Contributor

@DaanHoogland DaanHoogland Jan 15, 2024

Choose a reason for hiding this comment

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

the number of global setting we already have should not be an argument. Nobody know them by head and anybody will have to use search facilities to manage those anyway. We have not the value 60 in three separate extra places for a value that has a default of 1800 (?). Both tuning and code maintenance are served by creating an externalisation for those.
@weizhouapache I understand your argument against the need for tuning, however we are now setting 60 for a value that you say should be one. That sounds like we will want to tune it to 10 or 5 or maybe to 7 afterwards.

Anyway, this is merged. Let's discuss on #8506.

final Answer answer = easySend(hostId, ready);
if (answer == null || !answer.getResult()) {
// this is tricky part for secondary storage
Expand Down
1 change: 1 addition & 0 deletions ...e/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ private NicTO createNicTOFromNetworkAndOffering(NetworkVO networkVO, NetworkOffe
public boolean hostConnect(long hostId, long poolId) throws StorageConflictException {
StoragePool pool = (StoragePool) this.dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, pool);
cmd.setWait(60);
final Answer answer = agentMgr.easySend(hostId, cmd);

if (answer == null) {
Expand Down
2 changes: 1 addition & 1 deletion ...m/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ private boolean hostSupportsUefi(boolean isUbuntuHost) {
cmd = "dpkg -l ovmf";
}
s_logger.debug("Running command : " + cmd);
int result = Script.runSimpleBashScriptForExitValue(cmd);
int result = Script.runSimpleBashScriptForExitValue(cmd, 60, false);
s_logger.debug("Got result : " + result);
return result == 0;
}
Expand Down