Refactor: Replace sleep() with wait() #10504

sroopsai · 2025-03-05T11:41:00Z

Description

This PR fixes #10486 .

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

boring-cyborg · 2025-03-05T11:41:05Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected] (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

DaanHoogland · 2025-03-06T10:40:36Z

thanks @sroopsai , let's test this.

DaanHoogland · 2025-03-06T10:41:33Z

@blueorangutan package

blueorangutan · 2025-03-06T10:42:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov · 2025-03-06T10:48:28Z

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 16.60%. Comparing base (28e2411) to head (2658f94).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...tils/src/main/java/com/cloud/utils/ThreadUtil.java	0.00%	12 Missing ⚠️
...va/com/cloud/agent/manager/DirectAgentAttache.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #10504      +/-   ##
============================================
- Coverage     16.60%   16.60%   -0.01%     
  Complexity    13925    13925              
============================================
  Files          5730     5731       +1     
  Lines        508082   508236     +154     
  Branches      61770    61789      +19     
============================================
+ Hits          84386    84388       +2     
- Misses       414260   414413     +153     
+ Partials       9436     9435       -1

Flag	Coverage Δ
uitests	`3.93% <ø> (ø)`
unittests	`17.49% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2025-03-06T11:47:25Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 12680

DaanHoogland · 2025-03-06T12:19:53Z

@blueorangutan test

blueorangutan · 2025-03-06T12:22:05Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-03-07T04:50:52Z

[SF] Trillian test result (tid-12599)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 56751 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10504-t12599-kvm-ol8.zip
Smoke tests completed. 135 look OK, 1 have errors, 5 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestSharedNetworkWithConfigDrive>:setup	`Error`	1518.92	test_network.py
all_test_vpc_vpn	`Skipped`	---	test_vpc_vpn.py
all_test_webhook_delivery	`Skipped`	---	test_webhook_delivery.py
all_test_webhook_lifecycle	`Skipped`	---	test_webhook_lifecycle.py
all_test_host_maintenance	`Skipped`	---	test_host_maintenance.py
all_test_hostha_kvm	`Skipped`	---	test_hostha_kvm.py

blueorangutan · 2025-03-07T13:25:32Z

[SF] Trillian Build Failed (tid-12615)

blueorangutan · 2025-03-10T12:32:12Z

[SF] Trillian test result (tid-12623)
Environment: kvm-ol9 (x2), Advanced Networking with Mgmt server ol9
Total time taken: 60312 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10504-t12623-kvm-ol9.zip
Smoke tests completed. 139 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestClusterDRS>:setup	`Error`	0.00	test_cluster_drs.py
ContextSuite context=TestSharedNetworkWithConfigDrive>:setup	`Error`	1520.31	test_network.py

weizhouapache · 2025-03-10T12:51:42Z

@blueorangutan test ol8 vmware-70u3

blueorangutan · 2025-03-10T12:54:03Z

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests

blueorangutan · 2025-03-11T07:26:44Z

[SF] Trillian test result (tid-12635)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 64123 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10504-t12635-vmware-70u3.zip
Smoke tests completed. 134 look OK, 7 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_events_resource	`Error`	319.55	test_events_resource.py
test_01_events_resource	`Error`	319.56	test_events_resource.py
test_deploy_more_vms_than_limit_allows	`Error`	153.47	test_deploy_vms_in_parallel.py
test_01_prepare_and_cancel_maintenance	`Error`	0.09	test_ms_maintenance_and_safe_shutdown.py
test_04_deploy_vm_for_other_user_and_test_vm_operations	`Error`	129.84	test_network_permissions.py
ContextSuite context=TestSharedNetworkWithConfigDrive>:setup	`Error`	1524.67	test_network.py
test_02_restore_vm_with_disk_offering	`Error`	53.05	test_restore_vm.py
test_03_restore_vm_with_disk_offering_custom_size	`Error`	57.28	test_restore_vm.py
test_02_restore_vm_strict_tags_failure	`Error`	57.52	test_vm_strict_host_tags.py

sureshanaparti

clgtm

sureshanaparti · 2025-06-05T08:16:02Z

@blueorangutan package

Copilot

Pull Request Overview

This PR refactors the retry loop in runInContext to use wait() instead of Thread.sleep(), allowing the lock on the current instance to be released during each retry interval.

Replaced Thread.sleep(...) with wait(...) inside the synchronized retry loop.
Kept the retry logic and timeout calculation identical.

Comments suppressed due to low confidence (1)

engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java:168

Switching from Thread.sleep() to wait() releases the monitor lock, altering concurrency semantics. Verify that other threads should be allowed to enter synchronized methods on this instance during the wait.

wait(1000 * _HostPingRetryTimer.value());

Copilot · 2025-06-05T08:17:45Z

engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java

@@ -165,7 +165,7 @@ protected synchronized void runInContext() {
                    PingCommand cmd = resource.getCurrentStatus(_id);
                    int retried = 0;
                    while (cmd == null && ++retried <= _HostPingRetryCount.value()) {
-                        Thread.sleep(1000*_HostPingRetryTimer.value());
+                        wait(1000 * _HostPingRetryTimer.value());


Missing InterruptedException handling: wait(...) throws InterruptedException. Surround this call with a try/catch or declare the exception to prevent compilation errors.

Suggested change

wait(1000 * _HostPingRetryTimer.value());

try {

wait(1000 * _HostPingRetryTimer.value());

} catch (InterruptedException e) {

logger.warn("PingTask interrupted while waiting to retry ping [id: {}, uuid: {}, name: {}]", _id, _uuid, _name, e);

Thread.currentThread().interrupt(); // Restore the interrupted status

break;

}

engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java

DaanHoogland

your code looks good @sroopsai , two remarks

the co-pilot’s suggestion seem sensible (even when looking excessive)
I think we can de with a utility that embeds the try/rethrow to use in other locations that use sleep() now as well.

note this is not a -1, just a question/suggestion to improve.

sroopsai · 2025-06-11T15:00:54Z

Thank You for the suggestions @DaanHoogland

The other locations where sleep() is being called is in non-synchronised blocks, I think it is not good idea to replace those sleep() calls with wait(). But in this case sleep() is inside a synchronised block, there is no problem here. If you want I can write a utility method wrapping wait() call inside a try/catch block and use this method here and maybe use the method in future for sleep() calls (if written in synchronised blocks).

DaanHoogland · 2025-06-12T06:33:51Z

Thank You for the suggestions @DaanHoogland

The other locations where sleep() is being called is in non-synchronised blocks, I think it is not good idea to replace those sleep() calls with wait(). But in this case sleep() is inside a synchronised block, there is no problem here. If you want I can write a utility method wrapping wait() call inside a try/catch block and use this method here and maybe use the method in future for sleep() calls (if written in synchronised blocks).

that makes sense @sroopsai , A utility method that takes a timeout value and a set of log parameters would be great. Either in this PR or in a aseparate one.

DaanHoogland · 2025-06-12T06:34:29Z

@blueorangutan package

blueorangutan · 2025-06-12T06:36:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-06-12T08:13:41Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 13734

blueorangutan · 2025-06-12T13:42:37Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13753

github-actions · 2025-06-13T13:35:16Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

…ait calls

DaanHoogland · 2025-06-13T18:16:11Z

@blueorangutan package

blueorangutan · 2025-06-13T18:18:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-06-13T19:37:33Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 13769

engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java

…ait calls

DaanHoogland · 2025-06-14T09:26:31Z

@blueorangutan package

blueorangutan · 2025-06-14T09:28:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-06-14T10:45:28Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13779

DaanHoogland · 2025-06-16T07:03:33Z

@blueorangutan package

blueorangutan · 2025-06-16T07:04:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2025-06-16T08:28:24Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13787

DaanHoogland · 2025-06-16T12:49:40Z

@blueorangutan test

blueorangutan · 2025-06-16T12:52:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-06-17T06:01:48Z

[SF] Trillian test result (tid-13538)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 58247 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10504-t13538-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2025-06-17T07:10:41Z

@blueorangutan test ol8 vmware-70u3

blueorangutan · 2025-06-17T07:12:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests

blueorangutan · 2025-06-18T01:23:38Z

[SF] Trillian test result (tid-13539)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 62198 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10504-t13539-vmware-70u3.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_prepare_and_cancel_maintenance	`Error`	0.21	test_ms_maintenance_and_safe_shutdown.py

Refactor: Replace sleep() with wait()

339b327

boring-cyborg bot added component:agent component:orchestration labels Mar 5, 2025

DaanHoogland added the type:improvement label Mar 6, 2025

DaanHoogland added this to the 4.21.0 milestone Mar 6, 2025

sureshanaparti added this to Apache CloudStack 4.21.0 Jun 5, 2025

sureshanaparti approved these changes Jun 5, 2025

View reviewed changes

sureshanaparti requested review from Copilot and DaanHoogland June 5, 2025 08:15

Copilot AI reviewed Jun 5, 2025

View reviewed changes

sureshanaparti moved this to In Progress in Apache CloudStack 4.21.0 Jun 5, 2025

DaanHoogland reviewed Jun 8, 2025

View reviewed changes

Add utility method to handle wait calls

37a3727

github-actions bot added the status:has-conflicts label Jun 13, 2025

Refactor: Replace sleep() with wait() and add an utility method for w…

e6e92e8

…ait calls

github-actions bot removed the status:has-conflicts label Jun 13, 2025

DaanHoogland reviewed Jun 14, 2025

View reviewed changes

engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java Show resolved Hide resolved

Refactor: Replace sleep() with wait() and add an utility method for w…

03e4eb8

…ait calls

Add License to ThreadUtil

2658f94

DaanHoogland approved these changes Jun 16, 2025

View reviewed changes

-                        wait(1000 * _HostPingRetryTimer.value());
+                        try {
+                            wait(1000 * _HostPingRetryTimer.value());
+                        } catch (InterruptedException e) {
+                            logger.warn("PingTask interrupted while waiting to retry ping [id: {}, uuid: {}, name: {}]", _id, _uuid, _name, e);
+                            Thread.currentThread().interrupt(); // Restore the interrupted status
+                            break;
+                        }

Refactor: Replace sleep() with wait() #10504

Are you sure you want to change the base?

Refactor: Replace sleep() with wait() #10504

Uh oh!

Conversation

sroopsai commented Mar 5, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Uh oh!

boring-cyborg bot commented Mar 5, 2025

Uh oh!

DaanHoogland commented Mar 6, 2025

Uh oh!

DaanHoogland commented Mar 6, 2025

Uh oh!

blueorangutan commented Mar 6, 2025

Uh oh!

codecov bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Mar 6, 2025

Uh oh!

DaanHoogland commented Mar 6, 2025

Uh oh!

blueorangutan commented Mar 6, 2025

Uh oh!

blueorangutan commented Mar 7, 2025

Uh oh!

blueorangutan commented Mar 7, 2025

Uh oh!

blueorangutan commented Mar 10, 2025

Uh oh!

weizhouapache commented Mar 10, 2025

Uh oh!

blueorangutan commented Mar 10, 2025

Uh oh!

blueorangutan commented Mar 11, 2025

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Jun 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanHoogland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroopsai commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Jun 12, 2025

Uh oh!

DaanHoogland commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

blueorangutan commented Jun 12, 2025

Uh oh!

github-actions bot commented Jun 13, 2025

Uh oh!

DaanHoogland commented Jun 13, 2025

Uh oh!

blueorangutan commented Jun 13, 2025

Uh oh!

blueorangutan commented Jun 13, 2025

Uh oh!

codecov bot commented Mar 6, 2025 •

edited

Loading

DaanHoogland left a comment •

edited

Loading

sroopsai commented Jun 11, 2025 •

edited

Loading