CLOUDSTACK-9432: cluster/host dedicated to a domain is owned by the root domain by GabrielBrascher · Pull Request #2124 · apache/cloudstack

GabrielBrascher · 2017-05-21T01:11:52Z

Issue: when dedicating a resource (cluster or host) to a domain, the affinity group which is created is visible to everyone rather than only to the domain that the cluster is dedicated to.

Cause: if a resource is not dedicated to an account but a domain, the account parameter is null. The code then uses the domain id from the system account, making the affinity group visible by all accounts.

Proposal: if the account is null, then it should use the domain id passed as a parameter, instead of the domain id from the system account.

Test scenario: a cluster is dedicated to a domain with id '3'. The affinity group domain map should have '3' as its domain id.

Before: id '8' links affinity group '11' to domain '1' (ROOT).

select * from affinity_group_domain_map;
+----+-----------+-------------------+------------------+
| id | domain_id | affinity_group_id | subdomain_access |
+----+-----------+-------------------+------------------+
|  8 |         1 |                11 |                1 |
+----+-----------+-------------------+------------------+

After: id '9' links affinity group '12' to domain '3'.

select * from affinity_group_domain_map;
+----+-----------+-------------------+------------------+
| id | domain_id | affinity_group_id | subdomain_access |
+----+-----------+-------------------+------------------+
|  9 |         3 |                12 |                1 |
+----+-----------+-------------------+------------------+

rafaelweingartner

@GabrielBrascher thanks for the PR.
Could you take a look at some issues that I pointed out?

rafaelweingartner · 2017-05-26T15:56:49Z

-    private AffinityGroupVO createAffinityGroup(final AffinityGroupProcessor processor, final Account owner, final ACLType aclType, final String affinityGroupName, final String affinityGroupType, final String description) {
+    private AffinityGroupVO createAffinityGroup(final AffinityGroupProcessor processor, final Account owner, final ACLType aclType, final String affinityGroupName, final String affinityGroupType, final String description, boolean domainLevel, Long domainId) {
+
+        Long domainIdBasedOnDomainLevel = owner.getDomainId();


@GabrielBrascher what about extracting lines 181-184 to a method and then create a test case for it?
Also a documentation describing the method seems a good idea.

rafaelweingartner · 2017-05-26T15:57:41Z

-                              // created only as an admin/system operation.
-                }
-                types.add(processor.getType());
+            if (processor.isAdminControlledGroup()) {


Are these formatting changes needed?
Are you using the CloudStack code formatter style? Or was the code not formatted properly?

At first I tried to submit without those changes in lines which I did not touch; however, it failed to pass in check style validation. When my environment formatted the code (with the CloudStack style), all checks have passed. I can review those changes and see if I can minimize them. But some will be necessary.

Ah, ok.
So, let the changes there

remibergsma · 2017-06-17T19:10:18Z

Hi @GabrielBrascher thanks for the fix! Works better for me, although I think the affinity_group table still has IDs 1 for domain and account. Do you see that too?

GabrielBrascher · 2017-06-21T19:52:41Z

+        }
+        return domainIdBasedOnDomainLevel;
+    }
+


@rafaelweingartner thanks for the review. The code now is in a method with tests and documentation.

When dedicating a resource (cluster or host) to a domain, the affinity group which is created is visible to everyone rather than only to domain that the cluster is dedicated to.

GabrielBrascher · 2017-06-22T01:04:09Z

Hi @remibergsma . Thanks for the feedback!

(i) the account ID in the affinity_group table: I kept the account ID as the owner account id (in this case 1 because of the system account id) because I don't know the effect of letting it null; thus, the account ID stayed as it was being managed before this fix. Please feel free to provide any different approach if you have something in mind.

(i) the domain ID in affinity_group table: if I didn't miss anything, the domain ID should be from the dedicated domain ID (instead of 1). Line 186 AffinityGroupVO group = new AffinityGroupVO(affinityGroupName, affinityGroupType, description, affinityGroupDomainId, owner.getId(), aclType); creates an AffinityGroupVO with the domain id affinityGroupDomainId. I will test this again to be sure.

yadvr · 2017-07-27T11:55:08Z

@blueorangutan package

blueorangutan · 2017-07-27T11:55:36Z

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

blueorangutan · 2017-07-27T12:23:05Z

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

yadvr · 2017-07-27T12:28:59Z

@blueorangutan test

blueorangutan · 2017-07-27T12:29:39Z

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

blueorangutan · 2017-07-28T00:32:14Z

Trillian test result (tid-1265)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42728 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2124-t1265-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_iso.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Test completed. 48 look ok, 5 have error(s)

Test	Result	Time (s)	Test File
test_04_rvpc_privategw_static_routes	`Failure`	515.76	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	962.64	test_privategw_acl.py
ContextSuite context=TestInternalLb>:setup	`Error`	0.00	test_internal_lb.py
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_change_service_offering_for_vm_with_snapshots	Skipped	0.00	test_vm_snapshots.py
test_09_copy_delete_template	Skipped	0.01	test_templates.py
test_06_copy_template	Skipped	0.00	test_templates.py
test_static_role_account_acls	Skipped	0.02	test_staticroles.py
test_11_ss_nfs_version_on_ssvm	Skipped	0.02	test_ssvm.py
test_01_scale_vm	Skipped	0.00	test_scale_vm.py
test_01_primary_storage_iscsi	Skipped	0.03	test_primary_storage.py
test_nested_virtualization_vmware	Skipped	0.00	test_nested_virtualization.py
test_06_copy_iso	Skipped	0.00	test_iso.py
test_deploy_vgpu_enabled_vm	Skipped	0.02	test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support	Skipped	0.03	test_deploy_vgpu_enabled_vm.py

rafaelweingartner · 2017-09-13T20:23:02Z

From me you have an LGTM.
@remibergsma what do you think here?

yadvr · 2017-09-16T06:36:51Z

@blueorangutan package

blueorangutan · 2017-09-16T06:37:40Z

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

blueorangutan · 2017-09-16T07:19:51Z

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

houthuis

tested locally, it works as described

GabrielBrascher · 2017-11-28T12:26:13Z

Thanks for the review @rafaelweingartner @houthuis @remibergsma @rhtyd. I think this PR is ready for merge.

yadvr · 2017-11-28T13:16:06Z

@GabrielBrascher okay, I'll run regression tests on master later. Kindly use 'squash merge' in future and commits should include the JIRA id in their commit summary such as CLOUDSTACK-xxxx: short summary...

GabrielBrascher closed this May 21, 2017

GabrielBrascher reopened this May 21, 2017

GabrielBrascher force-pushed the CLOUDSTACK-9432 branch from 49ad69d to 1e4d30c Compare May 21, 2017 02:46

GabrielBrascher changed the title ~~CLOUDSTACK-9432: cluster/host dedicated to domain is owned by the root domain~~ CLOUDSTACK-9432: cluster/host dedicated to a domain is owned by the root domain May 21, 2017

kiwiflyer added the type:bug label May 22, 2017

rafaelweingartner requested changes May 26, 2017

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-9432 branch from 1e4d30c to 05bc1b3 Compare June 21, 2017 19:46

GabrielBrascher commented Jun 21, 2017

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-9432 branch from 05bc1b3 to 448744f Compare June 21, 2017 19:55

Resource dedicated to a domain is owned by the root domain

cd56112

When dedicating a resource (cluster or host) to a domain, the affinity group which is created is visible to everyone rather than only to domain that the cluster is dedicated to.

GabrielBrascher force-pushed the CLOUDSTACK-9432 branch from 448744f to cd56112 Compare June 21, 2017 20:55

rafaelweingartner approved these changes Nov 10, 2017

View reviewed changes

houthuis approved these changes Nov 24, 2017

View reviewed changes

GabrielBrascher merged commit bd56044 into apache:master Nov 28, 2017

Conversation

GabrielBrascher commented May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner May 26, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner May 26, 2017

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher May 26, 2017 • 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!

rafaelweingartner May 27, 2017

Choose a reason for hiding this comment

Uh oh!

remibergsma commented Jun 17, 2017

Uh oh!

GabrielBrascher Jun 21, 2017

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jun 22, 2017

Uh oh!

yadvr commented Jul 27, 2017

Uh oh!

blueorangutan commented Jul 27, 2017

Uh oh!

blueorangutan commented Jul 27, 2017

Uh oh!

yadvr commented Jul 27, 2017

Uh oh!

blueorangutan commented Jul 27, 2017

Uh oh!

blueorangutan commented Jul 28, 2017

Uh oh!

rafaelweingartner commented Sep 13, 2017

Uh oh!

yadvr commented Sep 16, 2017

Uh oh!

blueorangutan commented Sep 16, 2017

Uh oh!

blueorangutan commented Sep 16, 2017

Uh oh!

houthuis left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Nov 28, 2017

Uh oh!

yadvr commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

GabrielBrascher commented May 21, 2017 •

edited

Loading

GabrielBrascher May 26, 2017 •

edited

Loading