Enable account settings to be visible under domain settings by ravening · Pull Request #4215 · apache/cloudstack

ravening · 2020-07-17T07:38:19Z

Description

All the account settings can't be configured under domain
level settings right now.
By default, if account setting is not configured then
its value will be taken from global setting.
Add a global setting "enable.account.settings.for.domain"
so that if its enabled then all the account level settings
will be visible under domain levelsettings also.
If account level setting is configured then that value will
be considered else it will take domain scope value. If
domain scope value is not configured then it will pick
it up from global setting.

If domain level setting is not configured then by default
the value will be taken from global setting
Add another global setting "enable.domain.settings.for.child.domain"
so that when its true, if a value for domain setting is not
configured then its parent domain value is considered until
it reaches ROOT domain. If no value is configured till ROOT
domain then global setting value will be taken.

Also display all the settings configured under the domain level
in list domains api response

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?

This has been tested both from UI and cloudmonkey api

Cretae a domain relationship like

ROOT -> test1 -> test11

setting "enable.account.settings.for.domain" to false

The below output should not display config value

local) mgt01 > list configurations accountid=1431e-347c-4bae-a2ec-7c892cb name=router.service.offering
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Uuid of the service offering used by virtual routers; if NULL - system offering will be used",
      "isdynamic": true,
      "name": "router.service.offering",
      "scope": "account",
      "value": ""
    }
  ],
  "count": 1
}

After setting enable.account.settings.for.domain to true and set global value to f2d9-3803-4af-b84-96a369
The value should be of global setting value

(local) mgt01 > list configurations accountid=188b431e-347c-4bae-a2ec-7c8f1e4792cb name=router.service.offering
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Uuid of the service offering used by virtual routers; if NULL - system offering will be used",
      "isdynamic": true,
      "name": "router.service.offering",
      "scope": "account",
      "value": "f2d9-3803-4aef-b84-93ee69" <<<<<<<<<<<<< displays global value 
    }
  ],
  "count": 1
}

Assign different value for router.service.offering to domain test1 which is parent of test11
set the value of router.service.offering of domain "test1" to fca42701-6d28-4bc4-8c14-896d0878d07e

(local) mgt01 > list configurations name=router.service.offering domainid=e3a8ea3a-2318-4e81-99b3-8392513b80b0
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Uuid of the service offering used by virtual routers; if NULL - system offering will be used",
      "isdynamic": true,
      "name": "router.service.offering",
      "scope": "domain",
      "value": "fca42701-6d28-4bc4-8c14-896d0878d07e" <<<<<<<<<<<<< different value compared to global setting
    }
  ],
  "count": 1
}

setting "enable.account.settings.for.domain" to true
The value should still point to global setting

(local) mgt01 > list configurations name=router.service.offering accountid=188b431e-347c-4bae-a2ec-7c8f1e4792cb
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Uuid of the service offering used by virtual routers; if NULL - system offering will be used",
      "isdynamic": true,
      "name": "router.service.offering",
      "scope": "account",
      "value": "f2c5d7d9-3803-4aef-b884-9652f0a3ee69"
    }
  ],
  "count": 1
}

Now change the value of enable.domain.settings.for.child.domain to true
It should be display the value of test1 domain instead of global setting

(local) mgt01 > list configurations name=router.service.offering accountid=188b431e-347c-4bae-a2ec-7c8f1e4792cb
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Uuid of the service offering used by virtual routers; if NULL - system offering will be used",
      "isdynamic": true,
      "name": "router.service.offering",
      "scope": "account",
      "value": "fca42701-6d28-4bc4-8c14-896d0878d07e" <<<<<<<<<<<<< value assigned to test1 domain
    }
  ],
  "count": 1
}

All the settings enabled under domain will be displayed in list domains response

(local) mgt01 > list domains id=04dfd1c6-c28e-11ea-9a43-0617bc003377
{
  "count": 1,
  "domain": [
    {
      "cpuavailable": "Unlimited",
      "cpulimit": "Unlimited",
      "cputotal": 1,
      "domaindetails": {
        "vmsnapshot.expire.interval": "30"
      },

Running integration test cases

nosetests --with-marvin --marvin-config=<config file> test_enable_account_settings_for_domain.py

Disable account settings for domain ... === TestName: test_01_disable_account_settings_for_domain | Status : SUCCESS ===
ok
Enable account settings for domain ... === TestName: test_02_enable_account_settings_for_domain | Status : SUCCESS ===
ok
Enable account settings for domain ... === TestName: test_03_enable_account_settings_for_domain | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 3 tests in 10.193s

OK

DaanHoogland · 2020-07-29T08:44:14Z

This quite a functional change @ravening .
Did you test settings for domains linked to ldap?
I read in your description that you are (kind of) mixing account level settings with domain level settings. some settings do not apply to domains and others do not apply to accounts.

I like your idea that the setting system should be hierarchical and agree, but it just isn't designed this way. the same issue exists in zone-pod-cluster-host.

ravening · 2020-07-29T09:22:23Z

This quite a functional change @ravening .
Did you test settings for domains linked to ldap?
I read in your description that you are (kind of) mixing account level settings with domain level settings. some settings do not apply to domains and others do not apply to accounts.

I like your idea that the setting system should be hierarchical and agree, but it just isn't designed this way. the same issue exists in zone-pod-cluster-host.

@DaanHoogland

I am enabling only account settings for domain and not the other way around. Also there are two global settings to control this.

To enable all the account settings visible under domain
To fetch values from domain if account setting value is not set

The code is not making domain settings visible under account

If the (1)st global setting is true then you can configure value under a domain and all accounts will see that value. If its disabled then account will get value from global setting.

I didnt test it for ldap linked domains

DaanHoogland · 2020-07-29T18:25:51Z

tnx @ravening, I'm still not sure if the bear i see on the road is a phantom or real. Your explanation is very clear though.

ravening · 2020-08-03T13:38:30Z

tnx @ravening, I'm still not sure if the bear i see on the road is a phantom or real. Your explanation is very clear though.

do you think this will break something?

DaanHoogland · 2020-08-04T18:08:21Z

tnx @ravening, I'm still not sure if the bear i see on the road is a phantom or real. Your explanation is very clear though.

do you think this will break something?

So far the only thing i can think of is that people will get the idea that domain level settings would do anything on accounts. This may be misleading.

…omain settings

… under domain settings" This reverts commit 0ecdaeb.

blueorangutan · 2021-03-23T14:49:39Z

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

blueorangutan · 2021-03-23T15:47:49Z

Trillian Build Failed (tid-238)

blueorangutan · 2021-03-24T17:49:01Z

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

blueorangutan · 2021-03-24T23:23:05Z

Trillian test result (tid-250)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35410 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4215-t250-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 86 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`	75.36	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	54.41	test_vm_life_cycle.py

blueorangutan · 2021-03-25T06:34:36Z

Trillian test result (tid-257)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38290 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4215-t257-kvm-centos7.zip
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_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	69.88	test_kubernetes_clusters.py
test_router_dns_guestipquery	`Failure`	613.63	test_router_dns.py
test_01_migrate_VM_and_root_volume	`Error`	69.20	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	53.15	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	7.25	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	4.13	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	0.04	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	302.55	test_hostha_kvm.py

DaanHoogland

two style-ish comments, otherwise looks good.
Also I made some functional comments before, but don't consider those serious.

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

DaanHoogland · 2021-03-26T14:34:26Z

engine/schema/src/main/java/com/cloud/domain/dao/DomainDetailsDaoImpl.java

    public String getConfigValue(long id, ConfigKey<?> key) {
-        DomainDetailVO vo = findDetail(id, key.key());
+        DomainDetailVO vo = null;
+        String enableDomainSettingsForChildDomain = _configDao.getValue("enable.domain.settings.for.child.domain");


this string should probably come from some static final as now it is encoded twice (at least)

yadvr · 2021-06-17T07:28:57Z

ping @ravening can you address any outstanding remarks, thanks

nvazquez · 2021-08-12T03:44:59Z

Ping @ravening

weizhouapache · 2021-08-19T06:13:16Z

@ravening could you please fix the conflicts ?

sureshanaparti · 2021-08-26T08:04:40Z

ping @ravening can you fix the conflicting files.

yadvr · 2021-09-08T04:50:26Z

Ping @ravening

ravening · 2021-09-08T08:13:14Z

@rhtyd @weizhouapache done. sorry for the delay

blueorangutan · 2021-09-14T14:38:02Z

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

blueorangutan · 2021-09-14T15:28:27Z

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

yadvr · 2021-09-15T05:30:19Z

Ping @ravening can you fix conflicts?

sureshanaparti · 2021-09-20T04:49:18Z

Hi @ravening can you resolve conflicts.

All the account settings can't be configured under domain level settings right now. By default, if account setting is not configured then its value will be taken from global setting. Add a global setting "enable.account.settings.for.domain" so that if its enabled then all the account level settings will be visible under domain levelsettings also. If account level setting is configured then that value will be considered else it will take domain scope value. If domain scope value is not configured then it will pick it up from global setting. If domain level setting is not configured then by default the value will be taken from global setting Add another global setting "enable.domain.settings.for.child.domain" so that when its true, if a value for domain setting is not configured then its parent domain value is considered until it reaches ROOT domain. If no value is configured till ROOT domain then global setting value will be taken. Also display all the settings configured under the domain level in list domains api response

sureshanaparti · 2021-09-21T09:19:26Z

@blueorangutan package

blueorangutan · 2021-09-21T09:20:03Z

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

blueorangutan · 2021-09-21T10:32:19Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 1345

sureshanaparti · 2021-09-23T07:55:01Z

@blueorangutan package

blueorangutan · 2021-09-23T07:56:02Z

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

blueorangutan · 2021-09-23T08:29:30Z

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

sureshanaparti · 2021-09-24T04:33:19Z

@blueorangutan test

blueorangutan · 2021-09-24T04:34:03Z

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

blueorangutan · 2021-09-24T04:34:33Z

Trillian Build Failed (tid-2190)

sureshanaparti · 2021-09-24T05:14:36Z

@blueorangutan test

blueorangutan · 2021-09-24T05:15:03Z

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

blueorangutan · 2021-09-24T15:19:24Z

Trillian test result (tid-2192)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34884 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4215-t2192-kvm-centos7.zip
Smoke tests completed. 90 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

borisstoyanov

hi @ravening, I cannot see domain settings as domain admin, you've tagged this PR in a separate issue mentioning it'll fix it or did I get this wrong?

weizhouapache · 2021-09-27T10:16:54Z

hi @ravening, I cannot see domain settings as domain admin, you've tagged this PR in a separate issue mentioning it'll fix it or did I get this wrong?

@borisstoyanov it is addressed in PR #4339

ravening · 2021-09-27T11:25:46Z

@borisstoyanov have you set this setting to true? "enable.account.settings.for.domain"

borisstoyanov · 2021-09-27T11:27:46Z

@ravening yes I did, looks like it's addressed in a separate PR, thanks. Will test this one later today.

ravening · 2021-09-27T13:56:35Z

@ravening yes I did, looks like it's addressed in a separate PR, thanks. Will test this one later today.

@borisstoyanov its supposed to be enabled in this pr itself. not sure why its not showing up. can you try hard refresh?

borisstoyanov

LGTM, manually tested

svenvogel added type:enhancement component:api component:management-server labels Jul 19, 2020

ravening mentioned this pull request Aug 3, 2020

Enable resetting config values to default value #4230

Merged

1 task

ravening mentioned this pull request Sep 23, 2020

Domain settings not visible to domain admin #3752

Closed

ustcweizhou added a commit to ravening/cloudstack that referenced this pull request Oct 6, 2020

apache#4230 apache#4215 Reset account settings which visible under d…

0ecdaeb

…omain settings

ustcweizhou added a commit to ravening/cloudstack that referenced this pull request Oct 6, 2020

Revert " apache#4230 apache#4215 Reset account settings which visible…

3970f8d

… under domain settings" This reverts commit 0ecdaeb.

ravening force-pushed the domain_settings branch from c702e6f to b4a3f92 Compare October 28, 2020 12:27

DaanHoogland added this to the 4.16.0.0 milestone Oct 28, 2020

ravening force-pushed the domain_settings branch from b4a3f92 to 32e3d08 Compare December 10, 2020 15:34

ravening force-pushed the domain_settings branch 2 times, most recently from 065782e to 1ed6637 Compare March 23, 2021 13:25

DaanHoogland approved these changes Mar 26, 2021

View reviewed changes

ravening force-pushed the domain_settings branch from 1ed6637 to 0ea0465 Compare September 8, 2021 08:12

ravening and others added 2 commits September 20, 2021 13:18

rename variables

bca8556

ravening force-pushed the domain_settings branch from 3f33af4 to bca8556 Compare September 20, 2021 11:18

nvazquez added the status:needs-testing label Sep 26, 2021

borisstoyanov self-assigned this Sep 27, 2021

borisstoyanov reviewed Sep 27, 2021

View reviewed changes

borisstoyanov approved these changes Sep 29, 2021

View reviewed changes

borisstoyanov removed their assignment Sep 29, 2021

DaanHoogland merged commit dcc02e0 into apache:main Sep 29, 2021

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

ravening commented Jul 17, 2020 • 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!

DaanHoogland commented Jul 29, 2020

Uh oh!

ravening commented Jul 29, 2020

Uh oh!

DaanHoogland commented Jul 29, 2020

Uh oh!

ravening commented Aug 3, 2020

Uh oh!

DaanHoogland commented Aug 4, 2020

Uh oh!

blueorangutan commented Mar 23, 2021

Uh oh!

blueorangutan commented Mar 23, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 25, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanHoogland Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 17, 2021

Uh oh!

nvazquez commented Aug 12, 2021

Uh oh!

weizhouapache commented Aug 19, 2021

Uh oh!

sureshanaparti commented Aug 26, 2021

Uh oh!

yadvr commented Sep 8, 2021

Uh oh!

ravening commented Sep 8, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

yadvr commented Sep 15, 2021

Uh oh!

sureshanaparti commented Sep 20, 2021

Uh oh!

sureshanaparti commented Sep 21, 2021

Uh oh!

blueorangutan commented Sep 21, 2021

Uh oh!

blueorangutan commented Sep 21, 2021

Uh oh!

sureshanaparti commented Sep 23, 2021

Uh oh!

blueorangutan commented Sep 23, 2021

Uh oh!

blueorangutan commented Sep 23, 2021

Uh oh!

sureshanaparti commented Sep 24, 2021

Uh oh!

blueorangutan commented Sep 24, 2021

Uh oh!

blueorangutan commented Sep 24, 2021

Uh oh!

sureshanaparti commented Sep 24, 2021

Uh oh!

blueorangutan commented Sep 24, 2021

Uh oh!

blueorangutan commented Sep 24, 2021

Uh oh!

borisstoyanov left a comment

ravening commented Jul 17, 2020 •

edited

Loading