KVM: Propagating changes on host parameters to the agents by ustcweizhou · Pull Request #3491 · apache/cloudstack

ustcweizhou · 2019-07-15T09:33:44Z

Description

When we change a global setting and it needs to be applied to all kvm agents, it would be nice that the new value can be propagated to all kvm agents (and save in agent.properties if needed) so we do not need to manually restart cloudstack agent or change agent.properties.

it supports only "router.aggregation.command.each.timeout" for now.

In the future, it would be good to add a new column in "configurations" table to determine whether the configuration needs to be propagated to kvm agents.

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?

(1) change router.aggregation.command.each.timeout in global setting
(2) check agent.properties to see if the value is changed as well.

svenvogel · 2019-07-15T14:52:03Z

@ustcweizhou small question. do you mean with "configuration" ... global settings? how can we add more options which already exists in the agent.properties with your code?

ustcweizhou · 2019-07-15T20:28:24Z

@svenvogel
if global configuration already exists, then add it to line 1859
https://github.com/apache/cloudstack/pull/3491/files#diff-47d8b846590fa4ae09924a4e9569328bR1859

If global configuration does not exist, we need to add a ConfigKey at first.
similar as https://github.com/apache/cloudstack/blob/4.11/engine/orchestration/src/com/cloud/agent/manager/AgentManagerImpl.java#L193-L210

wido · 2019-07-17T07:39:39Z

Thanks! This is very welcome. I also looked into this for #3500 , but my problem with it is that it's very hardcoded. Both on the Mgmt server and Agent side.

The name for the keys are repeated multiple times and that can cause problems. It doesn't really scale.

How could we handle that?

svenvogel · 2019-07-17T08:29:02Z

@wido @ustcweizhou yes could be cool if we find a method which is lesser hardcoded. maybe a global setting "," comma separated?

yadvr · 2019-12-07T22:14:26Z

@ustcweizhou I tried changing the base branch to master and 4.13, there are conflicts. 4.11 is very old, can you rebase to master?

yadvr · 2020-01-28T05:07:13Z

@ustcweizhou can you rebase to 4.13? I've tentatively set the milestone to 4.13.1.0.

weizhouapache · 2020-01-28T10:16:07Z

rebased with latest master

DaanHoogland · 2020-01-28T10:54:06Z

@weizhouapache do we want this on 4.13 or on master. it is now based on master but targetted towards 4.13. Can you rebase or should we change the target?

weizhouapache · 2020-01-28T11:34:54Z

@DaanHoogland a, my bad. I will rebase with 4.13

DaanHoogland · 2020-01-28T16:37:01Z

@blueorangutan package

blueorangutan · 2020-01-28T16:37:17Z

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

blueorangutan · 2020-01-28T17:00:39Z

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

DaanHoogland · 2020-01-29T12:57:07Z

@blueorangutan test

blueorangutan · 2020-01-29T12:57:21Z

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

blueorangutan · 2020-01-30T00:03:22Z

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

Test	Result	Time (s)	Test File
test_hostha_enable_ha_when_host_in_maintenance	`Error`	302.87	test_hostha_kvm.py

DaanHoogland · 2020-01-30T09:22:29Z

@nvazquez can you review please. I see one ha error biut no relation to the code, thanks.

blueorangutan · 2020-02-17T15:39:34Z

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

blueorangutan · 2020-02-17T16:36:48Z

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

yadvr · 2020-02-18T04:23:18Z

@blueorangutan package

blueorangutan · 2020-02-18T04:23:35Z

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

blueorangutan · 2020-02-18T05:07:05Z

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

yadvr · 2020-02-18T08:41:13Z

@blueorangutan test

blueorangutan · 2020-02-18T08:41:38Z

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

blueorangutan · 2020-02-18T16:39:14Z

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

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	171.54	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	172.53	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	231.01	test_privategw_acl.py

yadvr · 2020-02-19T07:45:52Z

@nvazquez @DaanHoogland @andrijapanicsb can you review?

DaanHoogland

code looks good, refactoring is sane. The multiple occurances of the "global.setting.string"s can maybe be helped by constants. @wido 's remark would be good for a follow up PR.
@nvazquez can you review/test please?

DaanHoogland · 2020-02-19T12:36:39Z

tested according to Wei's spec and looks good. On advise from @nvazquez I also tested if the other pushed properties where still fine after the push:
host=10.2.2.171,10.2.2.168@static 👍

nvazquez · 2020-02-19T12:51:01Z

+ s_logger.debug("Propagating changes on host parameters to the agents");
+ Map<Long, List<Long>> hostsPerZone = getHostsPerZone();
+ Map<String, String> params = new HashMap<String, String>();
+ params.put("router.aggregation.command.each.timeout", _configDao.getValue("router.aggregation.command.each.timeout"));


What about removing this one from here and adding params as parameter?

@nvazquez it is absolutely good.
I will create another PR. this PR has already been merged into 4.13 and 4.14 ...

nvazquez · 2020-02-19T12:51:43Z

+ }
+ if (globalSettingUpdated.equals(ApiServiceConfiguration.ManagementServerAddresses.key()) ||
+ globalSettingUpdated.equals(IndirectAgentLBServiceImpl.IndirectAgentLBAlgorithm.key())) {
+ _indirectAgentLB.propagateMSListToAgents();


I think we can unify both calls as they do basically the same

nvazquez · 2020-02-19T12:51:56Z

+ globalSettingUpdated.equals(IndirectAgentLBServiceImpl.IndirectAgentLBAlgorithm.key())) {
+ _indirectAgentLB.propagateMSListToAgents();
+ } else if (globalSettingUpdated.equals(Config.RouterAggregationCommandEachTimeout.toString())) {
+ _agentManager.propagateChangeToAgents();


nvazquez

Thanks @weizhouapache code LGTM. Just left some small nice to have comments

DaanHoogland · 2020-02-19T13:11:51Z

merging this as is but created #3896 for follow up work

* 4.13: KVM: Propagating changes on host parameters to the agents (#3491)

@nvazquez

Make some changes based on @nvazquez 's comments in PR #3491 Fix a bug in #3491

ustcweizhou mentioned this pull request Jul 15, 2019
cloud0 subnet should not be hardcoded to 169.254.0.1/16 #3488
Closed

svenvogel added component:kvm type:enhancement labels Jul 15, 2019

yadvr changed the base branch from 4.11 to masterDecember 7, 2019 22:13

yadvr changed the base branch from master to 4.13December 7, 2019 22:13

yadvr changed the base branch from 4.13 to 4.11December 7, 2019 22:13

yadvr changed the base branch from 4.11 to 4.13January 28, 2020 05:06

yadvr added this to the 4.13.1.0 milestone Jan 28, 2020

ustcweizhou force-pushed the kvm-propagate-host-params branch from 0d6dd8d to 65f4d53Compare January 28, 2020 10:15

ustcweizhou force-pushed the kvm-propagate-host-params branch from 65f4d53 to 88ed321Compare January 28, 2020 11:48

DaanHoogland requested review from nvazquez and yadvr January 29, 2020 10:34

apache deleted a comment from blueorangutan Jan 29, 2020

DaanHoogland changed the base branch from master to 4.13February 19, 2020 09:29

DaanHoogland approved these changes Feb 19, 2020
View reviewed changes

DaanHoogland modified the milestones: 4.14.0.0, 4.13.1.0 Feb 19, 2020

nvazquez reviewed Feb 19, 2020
View reviewed changes

nvazquez approved these changes Feb 19, 2020
View reviewed changes

DaanHoogland mentioned this pull request Feb 19, 2020
merge propagation code of host parameters to the agents #3896
Closed

DaanHoogland merged commit ac7bcde into apache:4.13Feb 19, 2020

DaanHoogland added a commit that referenced this pull request Feb 19, 2020
Merge release branch 4.13 to master
b01e011
* 4.13: KVM: Propagating changes on host parameters to the agents (#3491)

weizhouapache mentioned this pull request Feb 19, 2020
Algorithm not set in agent.properties, when adding a new KVM hosts #3895
Closed

nvazquez pushed a commit to shapeblue/cloudstack that referenced this pull request Feb 21, 2020
KVM: Propagating changes on host parameters to the agents (apache#3491)
ef174a5

nvazquez pushed a commit to shapeblue/cloudstack that referenced this pull request Feb 21, 2020
KVM: Propagating changes on host parameters to the agents (apache#3491)
eca2b28

ustcweizhou mentioned this pull request Feb 24, 2020
kvm: fix/optimize propogating configs #3911
Merged
5 tasks

ggoodrich-ipp pushed a commit to ippathways/cloudstack that referenced this pull request Feb 24, 2020
KVM: Propagating changes on host parameters to the agents (apache#3491)
f02d288

ustcweizhou added a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
KVM: Propagating changes on host parameters to the agents (apache#3491)
77dbd9c

yadvr added a commit that referenced this pull request Mar 5, 2020
kvm: fix/optimize propogating configs (#3911)
b4fdf22
Make some changes based on @nvazquez 's comments in PR #3491 Fix a bug in #3491

weizhouapache mentioned this pull request Aug 17, 2023
Cloudstack agent should proactively check for changes made for Global setting parameter (host) #7341
Closed

Uh oh!

Conversation

ustcweizhou commented Jul 15, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

svenvogel commented Jul 15, 2019

Uh oh!

ustcweizhou commented Jul 15, 2019

Uh oh!

wido commented Jul 17, 2019

Uh oh!

svenvogel commented Jul 17, 2019

Uh oh!

yadvr commented Dec 7, 2019

Uh oh!

yadvr commented Jan 28, 2020

Uh oh!

weizhouapache commented Jan 28, 2020

Uh oh!

DaanHoogland commented Jan 28, 2020

Uh oh!

weizhouapache commented Jan 28, 2020

Uh oh!

DaanHoogland commented Jan 28, 2020

Uh oh!

blueorangutan commented Jan 28, 2020

Uh oh!

blueorangutan commented Jan 28, 2020

Uh oh!

DaanHoogland commented Jan 29, 2020

Uh oh!

blueorangutan commented Jan 29, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!

yadvr commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

yadvr commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

yadvr commented Feb 19, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 19, 2020

Uh oh!

nvazquezFeb 19, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapacheFeb 19, 2020

Choose a reason for hiding this comment

Uh oh!

nvazquezFeb 19, 2020

Choose a reason for hiding this comment

Uh oh!

nvazquezFeb 19, 2020

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 19, 2020

Uh oh!