-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Introducing granular command timeouts global setting #9659
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.
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
Introducing granular command timeouts global setting #9659
Conversation
|
@blueorangutan package |
|
@harikrishna-patnala 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #9659 +/- ##
=========================================
Coverage 16.04% 16.05%
- Complexity 12848 12858 +10
=========================================
Files 5639 5639
Lines 493945 494040 +95
Branches 59901 59917 +16
=========================================
+ Hits 79272 79316 +44
- Misses 405854 405899 +45
- Partials 8819 8825 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 11058 |
a666303 to
f603974
Compare
|
@blueorangutan package |
|
@harikrishna-patnala 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11069 |
sureshanaparti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11439)
|
58b66f6 to
76abf5d
Compare
|
@blueorangutan package |
|
@harikrishna-patnala 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. |
8d416b6 to
1a76b72
Compare
|
@blueorangutan package |
|
@harikrishna-patnala 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11092 |
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11457)
|
|
@blueorangutan package |
|
@harikrishna-patnala 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 11107 |
|
@DaanHoogland, before merging this one, please check #8605. This PR solves the same thing that #8605, but apparently that solution was ignored. |
ok @GutoVeronezi (and @harikrishna-patnala) , please discuss whether to merge or choose one of the two? As far as I can see the main difference is whether the configuration is done in a Configitem or in a separate table, is that right? |
|
[SF] Trillian test result (tid-11833)
|
Thanks @GutoVeronezi for bringing this up. I wasn't aware of your earlier PR, so I independently worked on a solution to address this issue, which is now available here. |
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java
Show resolved
Hide resolved
…ep the backward compatibility
18890a7 to
124da2b
Compare
|
@blueorangutan package |
|
@harikrishna-patnala 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12000 |
shwstppr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
* Introducing granular command timeouts global setting * fix marvin tests * Fixed log messages * some more log message fix * Fix empty value setting * Converted the global setting to non-dynamic * set wait on command only when granular wait is defined. This is to keep the backward compatibility * Improve error logging
Description
CloudStack has a global setting "wait" which controls the wait time by the management server for all the commands which it sends. There are also some command specific timeout settings which controls specific commands like "ready.command.wait", "kvm.storage.online.migration.wait", etc.
This PR simplifies these configurations by introducing a new parent global setting which serves as a single point to configure any command timeout. The new global setting is "commands. timeout", this timeout overrides the wait global config. This holds a comma separated key value pairs containing timeout for specific commands. For example: DhcpEntryCommand=600, SavePasswordCommand=300, VmDataCommand=300.
There are also some hardcoded values defined in the code base for few commands. so the order of precedence of all these timeouts will be
"commands.timeout" > Existing global settings > Hardcoded value for the command > global "wait"
Types of changes
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?