CheckOnHostCommand: add missing timeout setting by rp- · Pull Request #9677 · apache/cloudstack

rp- · 2024-09-13T12:23:54Z

Description

The new CheckOnHostCommand constructor was missing a reasonable timeout value, which meant it would fallback to the wait (1800s) timeout. On a Linstor cluster this resulted in over 15 minutes wait time until a host was recognized as down.
With timeout of 20s (as the other constructor) it takes 4-5 mins for a host to become recognized as down.

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

Screenshots (if appropriate):

How Has This Been Tested?

Failover tests (force shutdown of a host) in a Linstor cluster.

How did you try to break this feature and the system with this change?

codecov · 2024-09-13T12:32:00Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 15.11%. Comparing base (a0932b0) to head (eca66f8).
Report is 95 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...n/java/com/cloud/agent/api/CheckOnHostCommand.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #9677      +/-   ##
============================================
+ Coverage     15.08%   15.11%   +0.02%     
+ Complexity    11192    11190       -2     
============================================
  Files          5406     5406              
  Lines        473215   473214       -1     
  Branches      61680    58585    -3095     
============================================
+ Hits          71386    71521     +135     
- Misses       393880   393883       +3     
+ Partials       7949     7810     -139

Flag	Coverage Δ
uitests	`4.76% <ø> (+0.46%)`	⬆️
unittests	`15.80% <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.

core/src/main/java/com/cloud/agent/api/CheckOnHostCommand.java

The new CheckOnHostCommand constructor was missing a reasonable timeout value, which meant it would fallback to the wait (1800s) timeout. On a Linstor cluster this resulted in over 15 minutes wait time until a host was recognized as down. With timeout of 20s (as the other constructor) it takes 4-5 mins for a host to become recognized as down.

weizhouapache · 2024-09-18T08:45:03Z

@blueorangutan package

weizhouapache

code lgtm

sureshanaparti · 2024-09-20T12:17:20Z

@blueorangutan package

blueorangutan · 2024-09-20T12:18:03Z

@sureshanaparti 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 · 2024-09-20T13:22:53Z

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

slavkap

code LGTM but I haven't tested it

yadvr · 2024-10-17T09:22:50Z

@blueorangutan package

blueorangutan · 2024-10-17T09:24:03Z

@rohityadavcloud 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 · 2024-10-17T10:29:43Z

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

DaanHoogland

clgtm

DaanHoogland · 2024-10-28T14:10:02Z

@blueorangutan test

blueorangutan · 2024-10-28T14:12:03Z

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

blueorangutan · 2024-10-29T02:51:39Z

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

Test	Result	Time (s)	Test File

rp- · 2025-01-08T12:03:13Z

@DaanHoogland how to continue with this?

DaanHoogland · 2025-01-08T12:35:39Z

@DaanHoogland how to continue with this?

:D I think we can merge. Unless we need more testing for this online change. Personally I think smoke tests must have hit this change multiple times, ...

rp- · 2025-01-08T12:48:22Z

For me this is a regression fix. See also this discussion here: #10097

It can't be on purpose for CloudStack to take 15+ mins to detect a down host?

* 4.20: systemvm: fix keystore is reset when patch a systemvm (#9900) no retrieval of null hosts (#10175) upgrade: consider multiple hypervisors and secondary storages (#10046) CheckOnHostCommand: add missing timeout setting (#9677)

rp- requested review from DaanHoogland and slavkap September 13, 2024 12:23

boring-cyborg bot added the component:api label Sep 13, 2024

DaanHoogland reviewed Sep 16, 2024

View reviewed changes

core/src/main/java/com/cloud/agent/api/CheckOnHostCommand.java Outdated Show resolved Hide resolved

rp- force-pushed the checkonhostcommand-missing-timeout branch from 5ce9077 to eca66f8 Compare September 16, 2024 07:58

weizhouapache approved these changes Sep 18, 2024

View reviewed changes

sureshanaparti approved these changes Sep 20, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label Sep 20, 2024

slavkap approved these changes Sep 30, 2024

View reviewed changes

yadvr added this to the 4.19.2.0 milestone Oct 17, 2024

DaanHoogland approved these changes Oct 28, 2024

View reviewed changes

DaanHoogland assigned vladimirpetrov Nov 12, 2024

vladimirpetrov removed their assignment Nov 13, 2024

rp- modified the milestones: 4.19.2, 4.20.0 Nov 19, 2024

DaanHoogland removed the status:needs-testing label Jan 8, 2025

DaanHoogland merged commit afc95f1 into apache:4.19 Jan 8, 2025

rp- deleted the checkonhostcommand-missing-timeout branch January 8, 2025 13:05

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jan 16, 2025

CheckOnHostCommand: add missing timeout setting (apache#9677)

a70dcd6

Conversation

rp- commented Sep 13, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

weizhouapache commented Sep 18, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Sep 20, 2024

Uh oh!

blueorangutan commented Sep 20, 2024

Uh oh!

blueorangutan commented Sep 20, 2024

Uh oh!

slavkap left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 17, 2024

Uh oh!

blueorangutan commented Oct 17, 2024

Uh oh!

blueorangutan commented Oct 17, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 28, 2024

Uh oh!

blueorangutan commented Oct 28, 2024

Uh oh!

blueorangutan commented Oct 29, 2024

Uh oh!

rp- commented Jan 8, 2025

Uh oh!

DaanHoogland commented Jan 8, 2025

Uh oh!

rp- commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Sep 13, 2024 •

edited

Loading