kvm: Do not set backing file format of DATADISK in vm start/migration by ustcweizhou · Pull Request #4800 · apache/cloudstack

ustcweizhou · 2021-03-11T15:07:39Z

Description

We do not need to set backing file format of DATADISK and ROOT disk which does not have a backing file.

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

yadvr · 2021-03-11T15:39:11Z

@blueorangutan package

blueorangutan · 2021-03-11T15:40:03Z

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

blueorangutan · 2021-03-11T16:02:27Z

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

shwstppr · 2021-03-11T21:16:16Z

@blueorangutan test

blueorangutan · 2021-03-11T21:17:03Z

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

blueorangutan · 2021-03-12T07:23:51Z

[S] Trillian test result (tid-91)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34323 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4800-t91-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	`Error`	80.66	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	48.22	test_vm_life_cycle.py

shwstppr

LGTM

DaanHoogland · 2021-03-12T10:18:40Z

@weizhouapache I'm confused by your description in context of the code change. If we do have a disk, but no backing format, we go ahead and rebase it as qcow2, am I right?
and if we don't have a file path or we do have a backing format, we do nothing.

weizhouapache · 2021-03-12T10:23:28Z

@weizhouapache I'm confused by your description in context of the code change. If we do have a disk, but no backing format, we go ahead and rebase it as qcow2, am I right?
and if we don't have a file path or we do have a backing format, we do nothing.

@DaanHoogland I changed the title and description.

we do nothing if (1) disk does not have a backing file ; (2) disk has backing file, and also backing file format.
we set backing file format only if disk has a backing file but backing file format is missing

DaanHoogland · 2021-03-12T10:47:07Z

@weizhouapache I'm confused by your description in context of the code change. If we do have a disk, but no backing format, we go ahead and rebase it as qcow2, am I right?
and if we don't have a file path or we do have a backing format, we do nothing.

@DaanHoogland I changed the title and description.

we do nothing if (1) disk does not have a backing file ; (2) disk has backing file, and also backing file format.
we set backing file format only if disk has a backing file but backing file format is missing

exactly what the code does, good text for the tin ;)

yadvr · 2021-03-12T12:46:51Z

            String backingFileFormat = info.get(new String("backing_file_format"));
-            if (org.apache.commons.lang.StringUtils.isEmpty(backingFileFormat)) {
+            if (org.apache.commons.lang.StringUtils.isNotBlank(backingFilePath)
+                    && org.apache.commons.lang.StringUtils.isEmpty(backingFileFormat)) {


What if the backing file format is not empty?

@rhtyd then it is fine, we do nothing.

Maybe some log messages for debug/warn purposes would be interesting. Or do you think that it is something that system admins should not be bothered with?

I am thinking especially on cases such as backingFileFormat not empty and blank/empty backingFilePath, or vice versa.

Additionally, what do you think of replacing StringUtils.isEmpty(backingFileFormat) with StringUtils.isBlank(backingFileFormat)? In such case it would then check if backingFileFormat is whitespace, empty ("") or null.

Maybe some log messages for debug/warn purposes would be interesting. Or do you think that it is something that system admins should not be bothered with?

I am thinking especially on cases such as backingFileFormat not empty and blank/empty backingFilePath, or vice versa.

Additionally, what do you think of replacing StringUtils.isEmpty(backingFileFormat) with StringUtils.isBlank(backingFileFormat)? In such case it would then check if backingFileFormat is whitespace, empty ("") or null.

@GabrielBrascher there is a info next to these lines

s_logger.info("Setting backing file format of " + volPath);

I will add a line as comment to explain why we need this change.
https://libvirt.org/kbase/backing_chains.html#vm-refuses-to-start-due-to-misconfigured-backing-store-format

StringUtils.isBlank(backingFileFormat) and StringUtils.isEmpty(backingFileFormat) have no difference I think. there are only 2 possible values: empty and 'qcow2'. I will change to isBlank

GabrielBrascher

Thanks for the PR @weizhouapache.

GabrielBrascher · 2021-03-12T13:06:47Z

            String backingFileFormat = info.get(new String("backing_file_format"));
-            if (org.apache.commons.lang.StringUtils.isEmpty(backingFileFormat)) {
+            if (org.apache.commons.lang.StringUtils.isNotBlank(backingFilePath)
+                    && org.apache.commons.lang.StringUtils.isEmpty(backingFileFormat)) {


Maybe some log messages for debug/warn purposes would be interesting. Or do you think that it is something that system admins should not be bothered with?

I am thinking especially on cases such as backingFileFormat not empty and blank/empty backingFilePath, or vice versa.

Additionally, what do you think of replacing StringUtils.isEmpty(backingFileFormat) with StringUtils.isBlank(backingFileFormat)? In such case it would then check if backingFileFormat is whitespace, empty ("") or null.

GabrielBrascher

LGTM

shwstppr · 2021-03-15T08:48:54Z

@blueorangutan package

blueorangutan · 2021-03-15T08:51:10Z

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

blueorangutan · 2021-03-15T08:51:40Z

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

shwstppr · 2021-03-17T11:32:45Z

@blueorangutan package

blueorangutan · 2021-03-17T11:34:04Z

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

blueorangutan · 2021-03-17T12:18:12Z

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

yadvr · 2021-03-18T07:44:10Z

@blueorangutan test

blueorangutan · 2021-03-18T07:45:03Z

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

rubieHess · 2021-03-21T18:26:49Z

@@ -4266,7 +4266,11 @@ public void setBackingFileFormat(String volPath) {
            Map<String, String> info = qemu.info(file);
            String backingFilePath = info.get(new String("backing_file"));


I detect that this code is problematic. According to the Performance (PERFORMANCE), Dm: Method invokes inefficient new String(String) constructor (DM_STRING_CTOR).
Using the java.lang.String(String) constructor wastes memory because the object so constructed will be functionally indistinguishable from the String passed as a parameter. Just use the argument String directly.

@rubieHess good point. thanks ! I will create a pr for it.

Happy to help.

yadvr · 2021-03-24T06:56:58Z

@blueorangutan test

blueorangutan · 2021-03-24T06:58:03Z

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

blueorangutan · 2021-03-24T17:43:09Z

Trillian test result (tid-245)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36842 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4800-t245-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	`Error`	82.99	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	52.30	test_vm_life_cycle.py

@rubieHess

Thanks @rubieHess to point it out. see #4800 (comment)

kvm: Do not rebase DATADISK in vm start/migration

1dbcb06

weizhouapache mentioned this pull request Mar 11, 2021

Qemu 2.10 requires -U flag to read volume metadata #4567

Merged

7 tasks

yadvr added this to the 4.15.1.0 milestone Mar 12, 2021

shwstppr approved these changes Mar 12, 2021

View reviewed changes

weizhouapache changed the title ~~kvm: Do not rebase DATADISK in vm start/migration~~ kvm: Do not set backing file format of DATADISK in vm start/migration Mar 12, 2021

DaanHoogland approved these changes Mar 12, 2021

View reviewed changes

yadvr reviewed Mar 12, 2021

View reviewed changes

GabrielBrascher reviewed Mar 12, 2021

View reviewed changes

apache#4800: add comment and use isBlank

ef0321d

GabrielBrascher approved these changes Mar 12, 2021

View reviewed changes

rubieHess reviewed Mar 21, 2021

View reviewed changes

yadvr self-assigned this Mar 24, 2021

DaanHoogland merged commit 952b242 into apache:4.15 Mar 24, 2021

ustcweizhou mentioned this pull request Mar 25, 2021

kvm: remove unnecessary new String #4870

Merged

12 tasks

yadvr pushed a commit that referenced this pull request Apr 4, 2021

kvm: remove unnecessary new String (#4870)

0942838

Thanks @rubieHess to point it out. see #4800 (comment)

		@@ -4266,7 +4266,11 @@ public void setBackingFileFormat(String volPath) {
		Map<String, String> info = qemu.info(file);
		String backingFilePath = info.get(new String("backing_file"));

Uh oh!

Conversation

ustcweizhou commented Mar 11, 2021 • edited by weizhouapache 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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvr commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

shwstppr commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 12, 2021

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 12, 2021

Uh oh!

weizhouapache commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Mar 12, 2021

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!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Mar 15, 2021

Uh oh!

blueorangutan commented Mar 15, 2021

Uh oh!

blueorangutan commented Mar 15, 2021

Uh oh!

shwstppr commented Mar 17, 2021

Uh oh!

blueorangutan commented Mar 17, 2021

Uh oh!

blueorangutan commented Mar 17, 2021

Uh oh!

yadvr commented Mar 18, 2021

Uh oh!

blueorangutan commented Mar 18, 2021

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!

yadvr commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

ustcweizhou commented Mar 11, 2021 •

edited by weizhouapache

Loading

weizhouapache commented Mar 12, 2021 •

edited

Loading