kvm/ceph: When create a new RBD image use modern flags/features for t… by wido · Pull Request #2985 · apache/cloudstack

wido · 2018-10-30T13:24:44Z

…he image

These additional RBD features allow for faster lookups of how much space a RBD
image is using, but with the exclusive locking we prevent two VMs from writing
to the same RBD image at the same time.

These are the default features used by Ceph for any new RBD image.

Signed-off-by: Wido den Hollander wido@widodh.nl

Description

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)

Screenshots (if appropriate):

How Has This Been Tested?

wido · 2018-10-30T13:25:06Z

This also partially fixes #2650

yadvr · 2018-10-30T13:30:07Z

+        (1 << 4): RBD_FEATURE_FAST_DIFF
+        (1 << 5): RBD_FEATURE_DEEP_FLATTEN
+     */
+    private int rbdFeatures = (1 << 0) + (1 << 2) + (1 << 3) + (1 << 4) + (1 << 5);


How about define the feature as various final int hex values and rbdFeatures can be the or-ed value of those features. Should there also be some kind of version vs feature check?

Yeah, I could also just write 61 as that's the default.

I took these values from the C-header file from Ceph so that people see what we actually use.

That's mainly a code style thing.

Agree that it is mainly a code style thing. But here are some suggestions as well.

private final int RBD_FEATURE_LAYERING = 1; private final int RBD_FEATURE_EXCLUSIVE_LOCK = 4; private final int RBD_FEATURE_OBJECT_MAP = 8; private final int RBD_FEATURE_FAST_DIFF = 16; private final int RBD_FEATURE_DEEP_FLATTEN = 32; private int rbdFeatures = RBD_FEATURE_LAYERING + RBD_FEATURE_EXCLUSIVE_LOCK + RBD_FEATURE_OBJECT_MAP + RBD_FEATURE_FAST_DIFF + RBD_FEATURE_DEEP_FLATTEN;

or

/** Ceph RBD features to use when creating a new RBD image RBD_FEATURE_LAYERING: 1 RBD_FEATURE_EXCLUSIVE_LOCK: 4 RBD_FEATURE_OBJECT_MAP: 8 RBD_FEATURE_FAST_DIFF: 16 RBD_FEATURE_DEEP_FLATTEN: 32 Total:61 */ private final int RBF_FEATURES = 61;

I like the first suggestion @GabrielBrascher makes, @wido . it comes down to "self documenting code by proper naming"

yadvr · 2018-10-30T13:30:35Z

@blueorangutan package

yadvr · 2018-10-31T09:47:10Z

@blueorangutan package

blueorangutan · 2018-10-31T09:47:24Z

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

blueorangutan · 2018-10-31T10:18:51Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2397

yadvr · 2018-10-31T11:28:10Z

@blueorangutan test

blueorangutan · 2018-10-31T11:28:23Z

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

GabrielBrascher

Thanks for the PR @wido. Code LGTM.

GabrielBrascher · 2018-10-31T12:49:43Z

+        (1 << 4): RBD_FEATURE_FAST_DIFF
+        (1 << 5): RBD_FEATURE_DEEP_FLATTEN
+     */
+    private int rbdFeatures = (1 << 0) + (1 << 2) + (1 << 3) + (1 << 4) + (1 << 5);


Agree that it is mainly a code style thing. But here are some suggestions as well.

private final int RBD_FEATURE_LAYERING = 1; private final int RBD_FEATURE_EXCLUSIVE_LOCK = 4; private final int RBD_FEATURE_OBJECT_MAP = 8; private final int RBD_FEATURE_FAST_DIFF = 16; private final int RBD_FEATURE_DEEP_FLATTEN = 32; private int rbdFeatures = RBD_FEATURE_LAYERING + RBD_FEATURE_EXCLUSIVE_LOCK + RBD_FEATURE_OBJECT_MAP + RBD_FEATURE_FAST_DIFF + RBD_FEATURE_DEEP_FLATTEN;

or

/** Ceph RBD features to use when creating a new RBD image RBD_FEATURE_LAYERING: 1 RBD_FEATURE_EXCLUSIVE_LOCK: 4 RBD_FEATURE_OBJECT_MAP: 8 RBD_FEATURE_FAST_DIFF: 16 RBD_FEATURE_DEEP_FLATTEN: 32 Total:61 */ private final int RBF_FEATURES = 61;

blueorangutan · 2018-10-31T18:30:13Z

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

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	42.02	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	461.52	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	319.77	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	343.88	test_vpc_redundant.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.32	test_host_maintenance.py

wido · 2018-11-05T10:30:56Z

Thanks for the feedback! See the updated code

DaanHoogland

won't pass without the static keyword

DaanHoogland · 2018-11-05T14:56:41Z


    private String rbdTemplateSnapName = "cloudstack-base-snap";
-    private int rbdFeatures = (1 << 0); /* Feature 1<<0 means layering in RBD format 2 */
+    private final int RBD_FEATURE_LAYERING = 1;


the errors with checkstuyle are due to the vars not being static (final). full capitalised is reserved for those constants only.

DaanHoogland

won't pass without the static keyword

DaanHoogland

won't pass without the static keyword

…he image These additional RBD features allow for faster lookups of how much space a RBD image is using, but with the exclusive locking we prevent two VMs from writing to the same RBD image at the same time. These are the default features used by Ceph for any new RBD image. Signed-off-by: Wido den Hollander <wido@widodh.nl>

wido · 2018-11-05T16:21:46Z

Yes, I trusted my IDE to help with this, but fixed it now! Compiles :)

yadvr · 2018-11-12T06:03:22Z

@blueorangutan package

blueorangutan · 2018-11-12T06:04:19Z

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

blueorangutan · 2018-11-12T06:47:32Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2423

yadvr · 2018-11-12T07:05:42Z

@blueorangutan test

blueorangutan · 2018-11-12T07:06:20Z

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

GabrielBrascher · 2018-11-12T12:08:27Z

@wido @DaanHoogland @rhtyd merging it to the master branch, based on code review (3 LGTMs) and that all checks have passed.

blueorangutan · 2018-11-12T14:19:14Z

Trillian test result (tid-3163)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 24282 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2985-t3163-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 66 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	32.93	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	484.29	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	305.61	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	327.72	test_vpc_redundant.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	230.04	test_vpc_vpn.py

wido added component:kvm component:ceph labels Oct 30, 2018

wido added this to the 4.12.0.0 milestone Oct 30, 2018

wido requested review from GabrielBrascher, kiwiflyer and yadvr October 30, 2018 13:24

yadvr approved these changes Oct 30, 2018

View reviewed changes

GabrielBrascher approved these changes Oct 31, 2018

View reviewed changes

DaanHoogland approved these changes Oct 31, 2018

View reviewed changes

wido force-pushed the rbd-features branch from 85e0296 to 56f95ec Compare November 5, 2018 10:30

DaanHoogland reviewed Nov 5, 2018

View reviewed changes

wido force-pushed the rbd-features branch from 56f95ec to 7d9399b Compare November 5, 2018 16:18

GabrielBrascher merged commit 4809fe7 into apache:master Nov 12, 2018

yadvr mentioned this pull request Dec 6, 2018

Ceph RBD Feature flag update #2650

Closed

Uh oh!

Conversation

wido commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

wido commented Oct 30, 2018

Uh oh!

yadvr Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

wido Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 30, 2018

Uh oh!

yadvr commented Oct 31, 2018

Uh oh!

blueorangutan commented Oct 31, 2018

Uh oh!

blueorangutan commented Oct 31, 2018

Uh oh!

yadvr commented Oct 31, 2018

Uh oh!

blueorangutan commented Oct 31, 2018

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 31, 2018

Uh oh!

wido commented Nov 5, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

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

wido commented Nov 5, 2018

Uh oh!

yadvr commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

yadvr commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

GabrielBrascher commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

wido commented Oct 30, 2018 •

edited

Loading