Prevent KVM from performing volume migrations of running instances #4562

GutoVeronezi · 2020-12-30T20:59:16Z

Description

KVM has a limitation to modify instances definitions while they are on running state. Therefore, it is not possible to change volumes backend location easily.

There is a problem in the migrateVolume API. This API command ignores that limitation and causes an inconsistence on the database. ACS processes the migrate command, copies the volume to the destination storage, modifies the database and finishes the process with success. However, the running backend is still using the "old volume file".

This PR intends to prevent KVM to perform volumes migrations while KVM instances are in the running state and inform the user of an alternative API command that enables such operation on running instances.

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

Bug Severity

How Has This Been Tested?

It has been tested locally.

GabrielBrascher

Code LGTM.

DaanHoogland

code lgtm

yadvr · 2021-01-05T07:36:54Z

@blueorangutan package

blueorangutan · 2021-01-05T07:37:29Z

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

blueorangutan · 2021-01-05T08:19:48Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2524

weizhouapache

code makes sense.

could you please create pr for 4.15 instead of master ? (some changes on pom.xml needs to be reverted)

yadvr · 2021-01-05T08:32:38Z

The PR base branch is 4.15 @weizhouapache, I think you mean rebase against 4.15

weizhouapache · 2021-01-05T08:51:25Z

The PR base branch is 4.15 @weizhouapache, I think you mean rebase against 4.15

@rhtyd
as I see, this PR is based on 4.15, but it have some commits from latest master (contains version update to 4.16.0.0) which are not needed. should remove or revert these commits.

by the way, is there same issue in 4.14 ?

RodrigoDLopez

code LGTM.
great job @GutoVeronezi

what do you mean with:

How Has This Been Tested?
It has been tested locally.

could you please explain how to test this branch?

I tested this branch using the following behavior.

Create a virtual machine using KVM as a hypervisor
Run the 'migrateVolume' command with the instance running.
I checked the database to see if the information on that disk had been 'changed'
check the storage pool to see if the target volume has actually been moved as requested
at this point we can verify an inconsistency, the database tells us that the volume has been moved to the target storage pool, however it is not possible to identify the volume in this storage pool.

is this correct my test form?

GutoVeronezi · 2021-01-07T18:58:35Z

@RodrigoDLopez it's a good way to test it.

Before the commit, what you described in step 5 is what was happening.

To test it...

I created two primary storages.
I created one instance (using KVM as hypervisor) in one primary storage.
I updated hypervisor_capabilities to support storage motion of a KVM.
Via GUI, I tried to migrate the volume to the other primary storage.
Before the modifications, it would claim that the volume was migrated, but actually it would create an inconsistency in the database and in the storage pool. Now, it's throwing an exception explaining why it is not possible and how to get around the situation.

weizhouapache · 2021-01-13T13:35:58Z

@GutoVeronezi @rhtyd
can we have this in 4.14, 4.15 and master ?

GutoVeronezi · 2021-01-13T13:56:19Z

Thanks for the review guys. To us, it is fine if it only goes to master.

weizhouapache · 2021-01-15T08:40:11Z

Thanks for the review guys. To us, it is fine if it only goes to master.

@GutoVeronezi
the issue exists in 4.14 LTS and coming 4.15 LTS, we should fix it in the branches as well.
please update this pr

GutoVeronezi · 2021-01-15T18:10:50Z

Thanks for the review guys. To us, it is fine if it only goes to master.

@GutoVeronezi
the issue exists in 4.14 LTS and coming 4.15 LTS, we should fix it in the branches as well.
please update this pr

@weizhouapache
We would rather not open for 4.14, it would be more work on our side. We already fixed it in our 4.13 internal branch, and now we are proposing the patch to contribute back with the community. Committers could execute a back port (if they think it is interesting) after this one is merged in master.

weizhouapache · 2021-01-15T21:46:31Z

Thanks for the review guys. To us, it is fine if it only goes to master.

@GutoVeronezi
the issue exists in 4.14 LTS and coming 4.15 LTS, we should fix it in the branches as well.
please update this pr

@weizhouapache
We would rather not open for 4.14, it would be more work on our side. We already fixed it in our 4.13 internal branch, and now we are proposing the patch to contribute back with the community. Committers could execute a back port (if they think it is interesting) after this one is merged in master.

@GutoVeronezi when you create a pr, it will trigger jenkins test, travis test, trillian test, and also be reviewed and/or manual tested by community. no matter which dest branch is.

GutoVeronezi · 2021-01-18T17:01:29Z

Thanks for the review guys. To us, it is fine if it only goes to master.

@GutoVeronezi
the issue exists in 4.14 LTS and coming 4.15 LTS, we should fix it in the branches as well.
please update this pr

@weizhouapache
We would rather not open for 4.14, it would be more work on our side. We already fixed it in our 4.13 internal branch, and now we are proposing the patch to contribute back with the community. Committers could execute a back port (if they think it is interesting) after this one is merged in master.

@GutoVeronezi when you create a pr, it will trigger jenkins test, travis test, trillian test, and also be reviewed and/or manual tested by community. no matter which dest branch is.

@weizhouapache
Ok, I see how it works. Anyways, to us it is fine it if it goes only to master.

weizhouapache · 2021-01-19T08:36:40Z

@GutoVeronezi ok, got it.
@DaanHoogland what's your opinion ? merge it into master then backport to 4.14/4.15 ?

shwstppr

LGTM

@weizhouapache @DaanHoogland @rhtyd do we plan to backport this or should this go into 4.15.1.0?

DaanHoogland · 2021-01-25T11:39:25Z

@weizhouapache @shwstppr @GutoVeronezi @rhtyd I am always for merging in the oldest applicable branch first. If we want this in those branches we should rebase it first (imnsho).

weizhouapache · 2021-02-11T08:56:25Z

@weizhouapache @shwstppr @GutoVeronezi @rhtyd I am always for merging in the oldest applicable branch first. If we want this in those branches we should rebase it first (imnsho).

@DaanHoogland I agree. merge it to 4.14 at first, then merge to 4.15/master.

@GutoVeronezi can you create this branch from 4.14, apply the code change and force-push to github ?

KVM has a limitation to modify instances definitions while they are on running state. Therefore, it is not possible to change volumes backend location easily. There is a problem in the `migrateVolume` API. This API command ignores that limitation and causes an inconsistence on the database. ACS processes the migrate command, copies the volume to the destination storage, modifies the database and finishes the process with success. However, the running backend is still using the "old volume file". This PR intends to prevent KVM to perform volumes migrations while KVM instances are in the running state and inform the user of an alternative API command that enables such operation on running instances.

GutoVeronezi · 2021-02-12T15:16:03Z

@weizhouapache done

weizhouapache

code lgtm

weizhouapache · 2021-02-12T15:20:26Z

@weizhouapache done

@GutoVeronezi good, thanks

yadvr · 2021-02-15T08:31:46Z

@blueorangutan package

blueorangutan · 2021-02-15T08:32:27Z

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

blueorangutan · 2021-02-15T09:13:43Z

Packaging result: ✔centos7 ✖centos8 ✖debian. JID-2708

DaanHoogland · 2021-02-15T09:58:59Z

@blueorangutan test

blueorangutan · 2021-02-15T09:59:33Z

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

blueorangutan · 2021-02-15T19:03:27Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2021-02-16T14:23:21Z

not sure if we need more third party testing on this. @rhtyd @weizhouapache, can we ?

yadvr · 2021-02-19T09:04:22Z

Simple fix, I add a space between if (.... No testing needed, we don't support live volume migration with VM running.

…es (apache#4562) * Prevent KVM from performing volume migrations of running instances KVM has a limitation to modify instances definitions while they are on running state. Therefore, it is not possible to change volumes backend location easily. There is a problem in the `migrateVolume` API. This API command ignores that limitation and causes an inconsistence on the database. ACS processes the migrate command, copies the volume to the destination storage, modifies the database and finishes the process with success. However, the running backend is still using the "old volume file". This PR intends to prevent KVM to perform volumes migrations while KVM instances are in the running state and inform the user of an alternative API command that enables such operation on running instances. * Update VolumeApiServiceImpl.java Co-authored-by: Daniel Augusto Veronezi Salvador <[email protected]> Co-authored-by: Rohit Yadav <[email protected]>

GabrielBrascher added the component:kvm label Dec 30, 2020

GabrielBrascher assigned GutoVeronezi Dec 30, 2020

GabrielBrascher modified the milestone: 4.15.1.0 Dec 30, 2020

GabrielBrascher approved these changes Dec 30, 2020

View reviewed changes

DaanHoogland approved these changes Jan 4, 2021

View reviewed changes

GutoVeronezi force-pushed the throw-exception-when-live-migrate-volume-in-kvm branch from 66fa777 to 5113863 Compare January 4, 2021 16:38

yadvr requested a review from shwstppr January 5, 2021 07:22

yadvr changed the base branch from master to 4.15 January 5, 2021 07:36

weizhouapache approved these changes Jan 5, 2021

View reviewed changes

yadvr changed the base branch from 4.15 to master January 5, 2021 09:49

RodrigoDLopez approved these changes Jan 6, 2021

View reviewed changes

GabrielBrascher modified the milestone: 4.16.0.0 Jan 6, 2021

shwstppr approved these changes Jan 25, 2021

View reviewed changes

GutoVeronezi changed the base branch from master to 4.14 February 12, 2021 14:50

GutoVeronezi force-pushed the throw-exception-when-live-migrate-volume-in-kvm branch from 5113863 to bc760f2 Compare February 12, 2021 14:51

weizhouapache approved these changes Feb 12, 2021

View reviewed changes

DaanHoogland added this to the 4.14.2.0 milestone Feb 13, 2021

Update VolumeApiServiceImpl.java

dff9c56

yadvr merged commit 3b5f99a into apache:4.14 Feb 19, 2021

Prevent KVM from performing volume migrations of running instances #4562

Prevent KVM from performing volume migrations of running instances #4562

Uh oh!

Conversation

GutoVeronezi commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

GabrielBrascher 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!

yadvr commented Jan 5, 2021

Uh oh!

blueorangutan commented Jan 5, 2021

Uh oh!

blueorangutan commented Jan 5, 2021

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

weizhouapache commented Jan 5, 2021

Uh oh!

RodrigoDLopez 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!

GutoVeronezi commented Jan 7, 2021

Uh oh!

weizhouapache commented Jan 13, 2021

Uh oh!

GutoVeronezi commented Jan 13, 2021

Uh oh!

weizhouapache commented Jan 15, 2021

Uh oh!

GutoVeronezi commented Jan 15, 2021

Uh oh!

weizhouapache commented Jan 15, 2021

Uh oh!

GutoVeronezi commented Jan 18, 2021

Uh oh!

weizhouapache commented Jan 19, 2021

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jan 25, 2021

Uh oh!

weizhouapache commented Feb 11, 2021

Uh oh!

GutoVeronezi commented Feb 12, 2021

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Feb 12, 2021

Uh oh!

yadvr commented Feb 15, 2021

Uh oh!

blueorangutan commented Feb 15, 2021

Uh oh!

blueorangutan commented Feb 15, 2021

Uh oh!

DaanHoogland commented Feb 15, 2021

Uh oh!

blueorangutan commented Feb 15, 2021

Uh oh!

blueorangutan commented Feb 15, 2021

Uh oh!

DaanHoogland commented Feb 16, 2021

GutoVeronezi commented Dec 30, 2020 •

edited

Loading

yadvr commented Jan 5, 2021 •

edited

Loading

RodrigoDLopez left a comment •

edited

Loading