Externalize kvm agent storage reboot configuration by GutoVeronezi · Pull Request #4586 · apache/cloudstack

GutoVeronezi · 2021-01-14T17:50:11Z

Description

On KVMHAMonitor, when there is an inconsistency on the heartbeat's file or heartbeat timeout is extrapolated several times, the host is restarted. The host restarting amid several operations might bring inconsistency problems; therefore, it is interesting to let operators choose if they want the host restart or not on the hearbeat's timeout or storage errors.

This PR intends to externalize the heartbeat reboot configuration on KVMHAMonitor. The default value is true; therefore, the current behavior is maintained.

This PR depends on PR#4585.

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 functioanlity)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

It has been tested locally on a test lab.

On agent, I changed my log4j to debug mode.
I added hearbeat.update.timeout=10 setting to agent.properties to force a timeout on hearbeat.
I added reboot.host.and.alert.management.on.hearbeat.timeout setting to agent.properties and changed it some times;
I restarted agent and watched the log.

RodrigoDLopez

Hey @GutoVeronezi nice feature ...

I was thinking and if we change the name of these files, put something more accurate or self explainable?

AgentProperty.java => AgentProperties.java
since we will work to bring all available properties to this file, right?
AgentPropertyFile.java => AgentPropertiesFileHandler.java
this class sounds like a handler or facility to me, am I right?

Bearing in mind that we are looking to standardize and normalize things here. What do you think of normalizing the KVMHAMonitor.java file, at least on the lines where some changes were needed.

String _hostIP => String hostIP

My eyes bleed when I see things like that in the code.

RodrigoDLopez · 2021-01-15T14:37:01Z

+
 private static final Logger s_logger = Logger.getLogger(KVMHAMonitor.class);
- private final Map<String, NfsStoragePool> _storagePool = new ConcurrentHashMap<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> _storagePool = new ConcurrentHashMap<>();


And if we start to normalize these variable names in this file?
Removing these underscore at the start of any variable. at least where some changes were needed

you mean
Suggested change
privatefinalMap<String, NfsStoragePool> _storagePool = newConcurrentHashMap<>();
privatefinalMap<String, NfsStoragePool> storagePool = newConcurrentHashMap<>();
@RodrigoDLopez ;)
(will need work in the rest of the file!!!)

GutoVeronezi · 2021-01-15T18:08:12Z

@RodrigoDLopez sure, the new files names will be more intuitive.
I'll do it.

About the normalizations of the names, I understand what you mean, I'll do it too.

DaanHoogland · 2021-01-19T08:24:56Z

good work @GutoVeronezi looking forward to see this merged, but I am going to put my foot down for very extensive testing. 👍

RodrigoDLopez · 2021-02-01T17:04:15Z

nice job @GutoVeronezi
Code LGTM.

GutoVeronezi · 2021-03-24T21:41:26Z

Can anyone review this PR?

DaanHoogland

code looks good (in spite of the unguarded formats in debug statements. but this needs (as said) extensive 3rd party testing.

DaanHoogland · 2021-03-25T17:07:59Z

+
 private static final Logger s_logger = Logger.getLogger(KVMHAMonitor.class);
- private final Map<String, NfsStoragePool> _storagePool = new ConcurrentHashMap<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> _storagePool = new ConcurrentHashMap<>();


you mean
Suggested change
privatefinalMap<String, NfsStoragePool> _storagePool = newConcurrentHashMap<>();
privatefinalMap<String, NfsStoragePool> storagePool = newConcurrentHashMap<>();
@RodrigoDLopez ;)
(will need work in the rest of the file!!!)

DaanHoogland · 2021-03-25T17:08:57Z


- private final String _hostIP; /* private ip address */
+ /* private ip address */
+ private final String _hostIP;


same here
Suggested change
privatefinalString_hostIP;
privatefinalStringhostIP;

If the contributor decides to proceed with renaming this variable, I would also suggest removing the commented line /* private IP address */, and leave simply hostIP, or then something as hostPrivateIP or privateIpAddress, as this is expected to be the HostVO.privateIpAddress.

DaanHoogland · 2021-03-25T17:16:04Z

Can anyone review this PR?

I just did, but this is not what you need most, I'll look in the company to find test effort for us, to get this merged. As a general rule we need two lgtm, on of which should have at least tested extensively and the other at least looked at the code.

blueorangutan · 2021-05-10T10:00:07Z

Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 547

blueorangutan · 2021-05-10T19:40:50Z

Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 561

blueorangutan · 2021-05-11T07:49:33Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 578

GutoVeronezi · 2021-05-24T13:29:38Z

@blueorangutan test

GabrielBrascher

Code LGTM.

I added a suggestion regarding the variable naming change; nothing that would block this PR anyway.

GabrielBrascher · 2021-05-25T19:11:00Z


- private final String _hostIP; /* private ip address */
+ /* private ip address */
+ private final String _hostIP;


If the contributor decides to proceed with renaming this variable, I would also suggest removing the commented line /* private IP address */, and leave simply hostIP, or then something as hostPrivateIP or privateIpAddress, as this is expected to be the HostVO.privateIpAddress.

GutoVeronezi · 2021-05-25T20:06:00Z

@DaanHoogland @GabrielBrascher rename done.

sureshanaparti · 2021-05-26T07:08:21Z

 # Should be disabled for users with unmanaged iscsi connections on their hosts
+#
+#This parameter specifies if the host must be rebooted when something go wrong with heartbeat.
+# reboot.host.and.alert.management.on.hearbeat.timeout=true


typo hearbeat => heartbeat

sureshanaparti · 2021-05-26T07:09:29Z

+ * Default value: 60000 (ms).
+ */
+ public static final AgentProperties<Integer> HEARTBEAT_UPDATE_TIMEOUT = new AgentProperties<Integer>("hearbeat.update.timeout", 60000);
+


check the typo error 'hearbeat', and correct it wherever applicable.

sureshanaparti · 2021-06-24T12:26:13Z

@GutoVeronezi is this PR ready for review?

GutoVeronezi · 2021-06-24T12:37:31Z

@sureshanaparti as I mentioned in #4585 (comment), this PR will be rebased if #4585 be merged, therefore it still is a WiP.

blueorangutan · 2021-07-29T20:26:57Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 701

RodrigoDLopez

The code LGTM.
i'll be able to test this one on this weekend. perhaps i'll post my results in the next monday.

DaanHoogland

cltgm

DaanHoogland · 2021-07-30T19:06:32Z

 # heartbeat.update.timeout=60000
+
+# This parameter specifies if the host must be rebooted when something goes wrong with the heartbeat.
+# reboot.host.and.alert.management.on.heartbeat.timeout=true


hope the newline is there ;)

@DaanHoogland It's there =)
GitHub warns when there is no newline at the end of the file, e.g.:

ah, you're right. forgot about that.

blueorangutan · 2021-07-31T04:48:41Z

Trillian test result (tid-1434)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49901 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4586-t1434-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_loadbalance.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 86 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_05_ping_in_cpvm_success	`Failure`	15.22	test_diagnostics.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Failure`	352.50	test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	345.59	test_routers_network_ops.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	532.46	test_vpc_redundant.py

blueorangutan · 2021-07-31T09:37:06Z

Trillian test result (tid-1437)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50106 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4586-t1437-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 88 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	474.21	test_vpc_redundant.py

DaanHoogland · 2021-08-02T07:22:20Z

test_vpc_redundant is failing all over the open PRs, but running one last time anyway to be as sure as possible

blueorangutan · 2021-08-02T17:15:53Z

Trillian test result (tid-1446)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33833 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4586-t1446-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2021-08-02T18:44:49Z

@RodrigoDLopez did you test this?

RodrigoDLopez · 2021-08-04T13:39:29Z

@RodrigoDLopez did you test this?

Sorry @DaanHoogland I couldn't run the tests yet. I'm having problems with my test environment, but I will run the tests as soon as possible.

GutoVeronezi · 2021-08-09T18:52:39Z

@blueorangutan package

blueorangutan · 2021-08-09T18:53:03Z

@GutoVeronezi a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-08-09T19:37:43Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 815

DaanHoogland · 2021-08-11T07:37:31Z

@blueorangutan test

blueorangutan · 2021-08-11T07:38:03Z

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

blueorangutan · 2021-08-11T18:41:15Z

Trillian test result (tid-1588)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37995 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4586-t1588-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2021-08-12T05:27:08Z

@RodrigoDLopez did you test this?
Sorry @DaanHoogland I couldn't run the tests yet. I'm having problems with my test environment, but I will run the tests as soon as possible.

@RodrigoDLopez anything to share yet?

RodrigoDLopez · 2021-08-12T18:49:22Z

I ran some tests with this propose. But I'm not sure about my environment. I couldn't see my kvm agent stop at any point.
anyway, I managed to verify that the behavior of the externalized parameter is in accordance with what was proposed.

forcing my heartbeat timeout on two kvm hosts it's possible to see that the ones without the parameter reboot.host.and.alert.management.on.heartbeat.timeout=false will log the mensage failed: timeout; stopping cloudstack-agent.

Kvm-nd01

heartbeat.update.timeout=10
reboot.host.and.alert.management.on.heartbeat.timeout=true

Thread-1:null) (logid:) The command (/usr/share/cloudstack-common/scripts/vm/hypervisor/kvm/kvmheartbeat.sh -i 201.200.102.10 -p /export/primary -m /mnt/0727b905-35cb-36cb-961c-ea27b1a83333 -h 201.200.101.3 ), to the pool [0727b905-35cb-36cb-961c-ea27b1a83333], has the result [timeout].
(Thread-1:null) (logid:) Write heartbeat for pool [0727b905-35cb-36cb-961c-ea27b1a83333] failed: timeout; try: 5 of 5.
(Thread-1:null) (logid:) [IGNORED] Interrupted between heartbeat retries.
java.lang.InterruptedException: sleep interrupted
at java.base/java.lang.Thread.sleep(Native Method)
at com.cloud.hypervisor.kvm.resource.KVMHAMonitor.runHeartBeat(KVMHAMonitor.java:129)
at com.cloud.hypervisor.kvm.resource.KVMHAMonitor.run(KVMHAMonitor.java:176)
at java.base/java.lang.Thread.run(Thread.java:829)
(Thread-1:null) (logid:) Write heartbeat for pool [0727b905-35cb-36cb-961c-ea27b1a83333] failed: timeout; stopping cloudstack-agent.

Kvm-nd02

heartbeat.update.timeout=10
reboot.host.and.alert.management.on.heartbeat.timeout=false

(Thread-1:null) (logid:) The command (/usr/share/cloudstack-common/scripts/vm/hypervisor/kvm/kvmheartbeat.sh -i 201.200.102.10 -p /export/primary -m /mnt/0727b905-35cb-36cb-961c-ea27b1a83333 -h 201.200.101.4 ), to the pool [0727b905-35cb-36cb-961c-ea27b1a83333], has the result [timeout].
(Thread-1:null) (logid:) Write heartbeat for pool [0727b905-35cb-36cb-961c-ea27b1a83333] failed: timeout; try: 5 of 5.
(Thread-1:null) (logid:) [IGNORED] Interrupted between heartbeat retries.
java.lang.InterruptedException: sleep interrupted
at java.base/java.lang.Thread.sleep(Native Method)
at com.cloud.hypervisor.kvm.resource.KVMHAMonitor.runHeartBeat(KVMHAMonitor.java:129)
at com.cloud.hypervisor.kvm.resource.KVMHAMonitor.run(KVMHAMonitor.java:176)
at java.base/java.lang.Thread.run(Thread.java:829)

GabrielBrascher

Thanks for the PR @GutoVeronezi.

LGTM based on code review and manual tests done by @RodrigoDLopez.

The test looks good, probably the host was not rebooted due to some test env details. The logic behind loading the agent configuration, validating it and changing the execution flow according to it seems OK.

…ration

GutoVeronezi · 2021-08-23T16:33:21Z

@DaanHoogland @GabrielBrascher @RodrigoDLopez is this good to go? Is there anything else to do?

nvazquez · 2021-08-23T17:21:48Z

Thanks for testing @RodrigoDLopez
@GutoVeronezi I think we can kick a new round of tests then should be good to go
@blueorangutan package

blueorangutan · 2021-08-23T17:22:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

GutoVeronezi · 2021-08-23T17:29:52Z

@nvazquez ok, thanks!

blueorangutan · 2021-08-23T18:03:59Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 981

nvazquez · 2021-08-23T18:08:16Z

@blueorangutan test

blueorangutan · 2021-08-23T18:09:02Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-08-24T03:50:03Z

Trillian test result (tid-1754)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33182 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4586-t1754-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

RodrigoDLopez suggested changes Jan 15, 2021
View reviewed changes

DaanHoogland added this to the 4.16.0.0 milestone Jan 19, 2021

DaanHoogland added complexity:normal Severity:Minor type:cleanup type:enhancement labels Jan 19, 2021

RodrigoDLopez approved these changes Feb 1, 2021
View reviewed changes

weizhouapache mentioned this pull request Feb 18, 2021
kvm: Handle storage issue on NFS/KVM in multiple ways #4708
Closed
12 tasks

DaanHoogland approved these changes Mar 25, 2021
View reviewed changes

GutoVeronezi mentioned this pull request Apr 5, 2021
Externalize KVM Agent's option to change migration thread timeout #4570
Merged
7 tasks

GabrielBrascher approved these changes May 25, 2021
View reviewed changes

sureshanaparti reviewed May 26, 2021
View reviewed changes

GutoVeronezi marked this pull request as draft May 29, 2021 15:25

GutoVeronezi mentioned this pull request May 29, 2021
Externalize kvm agent storage timeout configuration #4585
Merged
7 tasks

GutoVeronezi force-pushed the externalize-kvm-agent-storage-reboot-configuration branch from 1022328 to 53fb485Compare July 19, 2021 13:36

GutoVeronezi requested review from DaanHoogland and sureshanaparti July 19, 2021 13:37

RodrigoDLopez reviewed Jul 30, 2021
View reviewed changes

DaanHoogland reviewed Jul 30, 2021
View reviewed changes

GabrielBrascher approved these changes Aug 13, 2021
View reviewed changes

Merge branch 'main' into externalize-kvm-agent-storage-reboot-configu…
088941c
…ration

nvazquez approved these changes Aug 24, 2021
View reviewed changes

nvazquez merged commit 83c0b61 into apache:mainAug 24, 2021

weizhouapache mentioned this pull request Apr 8, 2024
KVMHAMonitor getting initialized without host ha enabled #8682
Closed

	privatefinalMap<String, NfsStoragePool> _storagePool = newConcurrentHashMap<>();
	privatefinalMap<String, NfsStoragePool> storagePool = newConcurrentHashMap<>();

Uh oh!

Conversation

GutoVeronezi commented Jan 14, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Jan 15, 2021

Uh oh!

DaanHoogland commented Jan 19, 2021

Uh oh!

RodrigoDLopez commented Feb 1, 2021

Uh oh!

GutoVeronezi commented Mar 24, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 25, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

blueorangutan commented May 11, 2021

Uh oh!

GutoVeronezi commented May 24, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented May 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Jun 24, 2021

Uh oh!

GutoVeronezi commented Jun 24, 2021

Uh oh!

blueorangutan commented Jul 29, 2021

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 31, 2021

Uh oh!

blueorangutan commented Jul 31, 2021

Uh oh!

DaanHoogland commented Aug 2, 2021

Uh oh!

blueorangutan commented Aug 2, 2021

Uh oh!