Externalize KVM Agent's option to change migration thread timeout #4570

GutoVeronezi · 2021-01-05T18:42:40Z

Description

We noticed that sometimes migration in KVM fails due to a latency to move the VM process (domain) from the source host to the target. This has been noticed in Rhel7 and CentOS7. An exception can be observed due to the default (hard-coded) timeout that is extrapolated and ACS can't find the migrated VM's domain in the target host.

This PR intends to externalize that option in the KVM agent configuration. Default timeout will remain 10 seconds.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

It has been tested locally.

RodrigoDLopez

Hi @GutoVeronezi
Can you please explain to me the context? and how it has been tested?
Reading your description I got an idea, but I couldn't reproduce it.

GutoVeronezi · 2021-01-08T18:25:03Z

@RodrigoDLopez

I had two hosts with CentOS7.

I tried to migrate a VM from a host to another, but one of my hosts was with overloaded RAM, so, it would take more than 10 seconds (the hardcoded timeout) to migrate the VM.

With the externalized configuration, I could change the timeout to 20 seconds and my migration ran just fine.

DaanHoogland · 2021-01-18T15:04:12Z

@GutoVeronezi makes sense functionally and the code looks good except for the naming, why do you use dest in the names of the method and the setting? isn't this a local setting of the agent we are on at this moment?

GutoVeronezi · 2021-01-19T11:37:40Z

@GutoVeronezi makes sense functionally and the code looks good except for the naming, why do you use dest in the names of the method and the setting? isn't this a local setting of the agent we are on at this moment?

@DaanHoogland

Yes, it is local, but it refers to a migration from one host to the other, so it is verified if the VMs were correctly migrated from the source host to the destination host. The naming corresponds to the verification of the VM's DOM on the destination host.

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

Can anyone review this PR?

DaanHoogland · 2021-03-25T17:17:53Z

@wido @weizhouapache @GabrielBrascher @ravening can one of you guys look at this, please?

wido · 2021-03-25T19:13:24Z

I honestly don't get it yet. I need to think about this for a moment.

GabrielBrascher · 2021-03-25T19:37:41Z

@wido from my understanding, KVM with Rhel/CentOS does some kind of processing which results in Libvirt taking longer to get the domain ID when migrating the VM. Ubuntu, on the other hand, retrieves domain ID in less than 10 seconds.

Am I right, @GutoVeronezi?

GabrielBrascher

The variable dest.domain.migrate.retrieve.timeout is going to be placed at the agent.properties, right?

If so, I would recommend adding it on agent/conf/agent.properties. Something similar to what was done at PR #4585 where it was added commented lines as documentation as well as the variable with the default value.

wido · 2021-03-26T10:48:50Z

The variable dest.domain.migrate.retrieve.timeout is going to be placed at the agent.properties, right?

If so, I would recommend adding it on agent/conf/agent.properties. Something similar to what was done at PR #4585 where it was added commented lines as documentation as well as the variable with the default value.

In addition, shouldn't this be prefixed with 'vm' then as other migration options have the same prefix.

GutoVeronezi · 2021-03-26T19:01:35Z

@wido from my understanding, KVM with Rhel/CentOS does some kind of processing which results in Libvirt taking longer to get the domain ID when migrating the VM. Ubuntu, on the other hand, retrieves domain ID in less than 10 seconds.

Am I right, @GutoVeronezi?

Yes, that is the case.

GabrielBrascher · 2021-03-29T20:04:43Z

@GutoVeronezi what do you think of @wido's suggestion?

At the agent.properties there are already some parameters regarding VM migrations, such as vm.migrate.speed:

# set the vm migrate speed, by default, it will try to guess the speed of the guest network
# In MegaBytes per second
#vm.migrate.speed=0

# set target downtime at end of livemigration, the 'hiccup' for final copy. Higher numbers
# make livemigration easier, lower numbers may cause migration to never complete. Less than 1
# means hypervisor default (20ms).
#vm.migrate.downtime=0

# Busy VMs may never finish migrating, depending on environment. When its available, we will
# want to add support for autoconvergence migration flag which should fix this. Set an upper
# limit in milliseconds for how long live migration should wait, at which point VM is paused and
# migration will finish quickly.  Less than 1 means disabled.
#vm.migrate.pauseafter=0

Maybe adding this parameter nearby those (e.g. after vm.migrate.pauseafter) would help users to find it among other VM migration parameters. One option would be to change from dest.domain.migrate.retrieve.timeout to vm.migrate.domain.retrieve.timeout.

GutoVeronezi · 2021-03-31T13:19:28Z

@GabrielBrascher @wido I have changed the config name, is there something else to do?

GabrielBrascher

Code LGTM

GabrielBrascher · 2021-04-01T02:21:06Z

@blueorangutan package

blueorangutan · 2021-04-01T02:22:03Z

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

blueorangutan · 2021-04-01T02:46:48Z

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

weizhouapache · 2021-04-01T07:48:47Z

...vm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java

@@ -288,6 +295,17 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
        return new MigrateAnswer(command, result == null, result, null);
    }

+    private int parseAgentPropertiesVmMigrateDomainRetrieveTimeout() throws IOException {
+        final File agentPropertiesFile = PropertiesUtil.findConfigFile(KeyStoreUtils.AGENT_PROPSFILE);


@GutoVeronezi
you can use PropertiesStorage instead.

@weizhouapache I took a look into this class and I am a bit confused. It seems like a facade of PropertiesUtils mixed with an another purpouse.

It seems to me that this class is badly structured, without a clear flow and with nonsense logs. I think it will add a meaningless complexity to the code instead of facility it.

In other hand, in PR #4586 there is a proposal of a handler that will simplify this code. If it be approved, I will change this PR to use the handler.

GutoVeronezi · 2021-05-10T14:02:48Z

@weizhouapache can you look at this?

GutoVeronezi · 2021-05-17T14:18:16Z

Waiting for PR 4586 to be merged to use the AgentPropertiesFileHandler.

rohityadavcloud · 2021-05-25T13:40:41Z

@GutoVeronezi just another example where related PRs should be clubbed together, commits can be separate to allow reviewing of them individually.

GabrielBrascher · 2021-05-25T14:30:25Z

@rhtyd @GutoVeronezi my 2 cents here is: I think that PR #4586 has nothing to do with #4570.

The first one externalizes the KVM agent storage reboot configuration (#4586), while the second one (#4570) allows configuring migration thread timeout. They are completely different variables, that impact distinct components and change different behaviors.

The only thing that links both PRs is the AgentPropertiesFileHandler, which is basically using the same means to do a completely different thing.

I see it as a personal way of seeing. One could keep two PRs as they affect different parts and could easily be debugged and done separately. On the other hand, one could have them on a generic PR such as Externalize some KVM variables.

GutoVeronezi · 2021-07-29T19:43:41Z

@DaanHoogland @GabrielBrascher @RodrigoDLopez @sureshanaparti I'm pinging you here because you all reviewed #4585 and #4586.

CC @weizhouapache

#4585 introduced a handler that facilitate agent properties' file reading, which can be used here. However, #4585 was merged and reverted, therefore I had to submit a new work (#5239), which was merged a few days ago. Now I rebased this one and applied only the real changes.

GutoVeronezi · 2021-07-29T19:48:21Z

@blueorangutan package

blueorangutan · 2021-07-29T19:49:03Z

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

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

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

weizhouapache · 2021-07-29T20:32:36Z

@blueorangutan test

blueorangutan · 2021-07-29T20:33:03Z

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

RodrigoDLopez

Code LGTM.
I'll test it and then post my results as soon as possible.

blueorangutan · 2021-07-30T19:31:05Z

Trillian test result (tid-1425)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 66185 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4570-t1425-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_async_job.py
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
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_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 84 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80	`Failure`	430.95	test_internal_lb.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3610.36	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	3613.87	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.06	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	90.92	test_kubernetes_clusters.py
test_router_dns_guestipquery	`Failure`	600.37	test_router_dns.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Failure`	358.53	test_routers_network_ops.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	497.64	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	535.94	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	535.96	test_vpc_redundant.py

…rate-thread-timeout

GabrielBrascher

Proposal and Code LGTM

weizhouapache · 2021-08-17T15:27:44Z

@blueorangutan package

blueorangutan · 2021-08-17T15:28:03Z

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

blueorangutan · 2021-08-17T15:54:06Z

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

DaanHoogland

clgtm

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

Could we re-run the tests in this one?

nvazquez · 2021-08-25T03:08:39Z

@GutoVeronezi can you please fix the conflicts? We can start a new round of tests after that

…rate-thread-timeout

GutoVeronezi · 2021-08-25T11:56:19Z

@GutoVeronezi can you please fix the conflicts? We can start a new round of tests after that

@nvazquez done, thanks!

nvazquez · 2021-08-25T15:06:27Z

@blueorangutan package

blueorangutan · 2021-08-25T15:07:03Z

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

blueorangutan · 2021-08-25T15:38:27Z

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

nvazquez · 2021-08-25T16:02:42Z

@blueorangutan test

blueorangutan · 2021-08-25T16:03:02Z

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

RodrigoDLopez

The proposal is okay and CLGTM.

blueorangutan · 2021-08-26T03:16:27Z

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

Test	Result	Time (s)	Test File

RodrigoDLopez reviewed Jan 6, 2021

View reviewed changes

DaanHoogland added this to the 4.16.0.0 milestone Jan 18, 2021

DaanHoogland added complexity:simple a few days to a couple of weeks component:kvm Severity:Minor type:enhancement labels Jan 18, 2021

GabrielBrascher reviewed Mar 25, 2021

View reviewed changes

GutoVeronezi requested a review from GabrielBrascher March 29, 2021 18:08

GabrielBrascher approved these changes Mar 31, 2021

View reviewed changes

weizhouapache reviewed Apr 1, 2021

View reviewed changes

GutoVeronezi requested a review from weizhouapache April 22, 2021 19:11

GutoVeronezi marked this pull request as draft May 17, 2021 14:15

GutoVeronezi requested review from DaanHoogland and sureshanaparti July 29, 2021 19:44

RodrigoDLopez reviewed Jul 30, 2021

View reviewed changes

GutoVeronezi added 2 commits August 16, 2021 10:04

Merge branch 'main' into externalize-kvm-agent-option-dest-domain-mig…

4ecfaab

…rate-thread-timeout

Update javadoc

09e0320

GabrielBrascher approved these changes Aug 17, 2021

View reviewed changes

DaanHoogland approved these changes Aug 17, 2021

View reviewed changes

Merge branch 'main' into externalize-kvm-agent-option-dest-domain-mig…

729933f

…rate-thread-timeout

RodrigoDLopez approved these changes Aug 25, 2021

View reviewed changes

nvazquez approved these changes Aug 26, 2021

View reviewed changes

nvazquez merged commit 159c72f into apache:main Aug 26, 2021

Externalize KVM Agent's option to change migration thread timeout #4570

Externalize KVM Agent's option to change migration thread timeout #4570

Uh oh!

Conversation

GutoVeronezi commented Jan 5, 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!

GutoVeronezi commented Jan 8, 2021

Uh oh!

DaanHoogland commented Jan 18, 2021

Uh oh!

GutoVeronezi commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GutoVeronezi commented Mar 24, 2021

Uh oh!

DaanHoogland commented Mar 25, 2021

Uh oh!

wido commented Mar 25, 2021

Uh oh!

GabrielBrascher commented Mar 25, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

wido commented Mar 26, 2021

Uh oh!

GutoVeronezi commented Mar 26, 2021

Uh oh!

GabrielBrascher commented Mar 29, 2021

Uh oh!

GutoVeronezi commented Mar 31, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Apr 1, 2021

Uh oh!

blueorangutan commented Apr 1, 2021

Uh oh!

blueorangutan commented Apr 1, 2021

Uh oh!

weizhouapache Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented May 10, 2021

Uh oh!

GutoVeronezi commented May 17, 2021

Uh oh!

rohityadavcloud commented May 25, 2021

Uh oh!

GabrielBrascher commented May 25, 2021

Uh oh!

GutoVeronezi commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GutoVeronezi commented Jul 29, 2021

Uh oh!

blueorangutan commented Jul 29, 2021

Uh oh!

blueorangutan commented Jul 29, 2021

Uh oh!

weizhouapache commented Jul 29, 2021

Uh oh!

blueorangutan commented Jul 29, 2021

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 30, 2021

GutoVeronezi commented Jan 19, 2021 •

edited

Loading

GutoVeronezi commented Jul 29, 2021 •

edited

Loading