FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add missing HA config keys by mdominka · Pull Request #3776 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

Add missing HA config keys - #3776

Merged
DaanHoogland merged 1 commit into
apache:4.13from
mdominka:fix_ha_config_keys
Jan 14, 2020
ConversationCommits (1)ChecksFiles changed
Merged

Add missing HA config keys#3776
DaanHoogland merged 1 commit into
apache:4.13from
mdominka:fix_ha_config_keys

Conversation

mdominka commented Dec 19, 2019 •
edited by svenvogel
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request adds the missing configuration keys for the High Availability service for virtual machines. (High-Availability_Developer_Guide)

Added keys (missing in the global configuration):
time.between.cleanup
max.retries
time.to.sleep
time.between.failures

Rename keys in the confluence document:
ha.retry.wait -> restart.retry.interval
stop.retry.wait -> stop.retry.interval
workers -> ha.workers

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?

We tested it in production in our KVM environment.

svenvogel added this to the 4.14.0.0 milestone Dec 19, 2019

mdominka commented Dec 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Add Cluster scopes to config - as it crashes master when started from empty DB.
Build master and deploy with empty DB. Watch MS failing. Find out and tested by @anuragaw

In our environment the existing keys (stop.retry.interval, restart.retry.interval, investigate.retry.interval, migrate.retry.interval) are defined with scope = "null" and is_dynamic = false

svenvogel commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@andrijapanicsb can you kick the tests?

svenvogel changed the title Fix ha config keys Fix HA config keys Dec 19, 2019

andrijapanicsb commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Dec 19, 2019

Copy link
Copy Markdown

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

blueorangutan commented Dec 19, 2019

Copy link
Copy Markdown

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

andrijapanicsb commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Dec 19, 2019

Copy link
Copy Markdown

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

anuragaw suggested changes Dec 19, 2019
View reviewed changes

anuragaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some suggestions.

Comment thread engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java Resolved
Comment thread engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java Resolved
Comment thread engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java Resolved
Comment thread engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java Resolved

svenvogel commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@anuragaw can you check again?

anuragaw reviewed Dec 19, 2019
View reviewed changes
Comment thread server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java Outdated

value = params.get("migrate.retry.interval");
_migrateRetryInterval = NumbersUtil.parseInt(value, 2 * 60);
_migrateRetryInterval = NumbersUtil.parseInt(value, MIGRATE_RETRY_INTERVAL_SECONDS);

anuragaw Dec 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, sorry I missed this earlier but while we are exposing some config keys - perhaps we can also expose "migrate.retry.interval", "investigate.retry.interval", "restart.retry.interval", "stop.retry.interval", "force.ha" through ConfigKey?

mdominka Dec 19, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok, I will also changed this deprecated key definitions.

DaanHoogland Dec 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

and time.between.cleanup, time.between.failures, max.retries and time.to.sleep

anuragaw reviewed Dec 19, 2019
View reviewed changes

anuragaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One minor comment @mdominka . Thanks for addressing previous comments.

blueorangutan commented Dec 19, 2019

Copy link
Copy Markdown

Trillian test result (tid-647)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 27979 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3776-t647-kvm-centos7.zip
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

DaanHoogland approved these changes Dec 20, 2019
View reviewed changes

DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

seen the responses to review comments. looks good

Comment thread server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java Outdated

value = params.get("migrate.retry.interval");
_migrateRetryInterval = NumbersUtil.parseInt(value, 2 * 60);
_migrateRetryInterval = NumbersUtil.parseInt(value, MIGRATE_RETRY_INTERVAL_SECONDS);

DaanHoogland Dec 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

and time.between.cleanup, time.between.failures, max.retries and time.to.sleep

anuragaw reviewed Dec 20, 2019
View reviewed changes

anuragaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code looks good but haven't tested. I'll kick off tests to verify. We can merge after that.

Comment thread api/src/main/java/com/cloud/resource/ResourceState.java Resolved
Comment thread server/src/main/java/com/cloud/ha/dao/HighAvailabilityDao.java Resolved
Comment thread server/src/main/java/com/cloud/ha/dao/HighAvailabilityDaoImpl.java Resolved

anuragaw commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Dec 20, 2019

Copy link
Copy Markdown

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

blueorangutan commented Dec 20, 2019

Copy link
Copy Markdown

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

anuragaw commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

@mdominka - can you check if Travis Failures are related? I'll kick off tests in parallel.

@blueorangutan test

blueorangutan commented Dec 20, 2019

Copy link
Copy Markdown

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

mdominka commented Dec 20, 2019

Copy link
Copy Markdown
Contributor Author

One travis test failed, but i see no relation to my changes.
Can you test again @anuragaw

DaanHoogland commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

@mdominka it is because we strain travis too much but it is unrelated to the tests @anuragaw is doing. The log was longer than travis allows and we've been getting those more and more lately.

anuragaw commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

Travis failure was unrelated @mdominka . The tests that I've run are more exhaustive and should succeed in sometime.

Did you see unrelated files' changes ? Not something that's terrible but it's a matter of choice - personally I don't like having redundant noise in my PRs to keep clean git history by not committing unintended changes.

svenvogel commented Jan 8, 2020 via email

Copy link
Copy Markdown
Contributor

svenvogel commented Jan 8, 2020 via email

Copy link
Copy Markdown
Contributor

mdominka commented Jan 8, 2020

Copy link
Copy Markdown
Contributor Author

when will it be merged?

andrijapanicsb commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Jan 8, 2020

Copy link
Copy Markdown

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

svenvogel commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

@DaanHoogland does we have the time to rebase it to 4.13.0.0 or we need to move it in 4.14.0.0?

@mdominka tried it but it seems not easy. @DaanHoogland if you can help us how to rebase it and ensure that it will be forward merged into 4.14.0.0? this is important for us.

can you tell us some git commands how to rebase it.

blueorangutan commented Jan 9, 2020

Copy link
Copy Markdown

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

Test Result Time (s) Test File

andrijapanicsb commented Jan 9, 2020

Copy link
Copy Markdown
Contributor

@DaanHoogland can you please review briefly

DaanHoogland commented Jan 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

only 200 lines are changed and there are a lot of unneeded import statement moves in these files that would make porting difficult. If you want it on 4.13 as well

  1. start with reverting those import changes in a new commit
  2. and than git rebase -i HEAD~12 on (a copy of) your branch.
  3. I would say squash all changes.
  4. than git cherry-pick <the resulting commitish> on a new branch of off 4.13
  5. rename the new branch to fix_ha_config_keys
  6. force it here by git push mdominka fix_ha_config_keys --force
  7. change this PR to point to the other base branch.

hope this helps

svenvogel commented Jan 12, 2020

Copy link
Copy Markdown
Contributor

@DaanHoogland thanks for help. 👍 @mdominka will rebase it on 4.13 on monday. if its finished can you ensure that it will be ported to 4.14?

DaanHoogland commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

@mdominka @svenvogel any thing that goes on the 4.13 branch before 4.14 is released will be forward merged so it will be in master before 4.14 is cut. (A.K.A. don't worry™)

svenvogel modified the milestones: 4.14.0.0, 4.13.1.0 Jan 13, 2020

svenvogel commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

(A.K.A. don't worry™) 😄

@DaanHoogland here we go. can you check now?

@andrijapanicsb can you rerun the tests please?

anuragaw commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

@svenvogel @mdominka - before rerunning tests may be we can do a quick rebase to resolve conflicts? There are merge conflicts.

DaanHoogland changed the base branch from master to 4.13 January 13, 2020 15:02

DaanHoogland commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

@anuragaw It was still on master, while now being targetted at 4.13. all fine now

andrijapanicsb commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Jan 13, 2020

Copy link
Copy Markdown

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

blueorangutan commented Jan 13, 2020

Copy link
Copy Markdown

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

anuragaw commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Jan 13, 2020

Copy link
Copy Markdown

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

blueorangutan commented Jan 14, 2020

Copy link
Copy Markdown

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

Test Result Time (s) Test File

DaanHoogland changed the title Fix HA config keys Add missing HA config keys Jan 14, 2020
DaanHoogland merged commit 54cc73a into apache:4.13 Jan 14, 2020
DaanHoogland added a commit that referenced this pull request Jan 15, 2020
Add missing HA config keys (#3776) (#3814)
4780a27 
* Add missing HA config keys (#3776)

* merge conflict-bugs fixed

Co-authored-by: mdominka <50666672+mdominka@users.noreply.github.com>
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
Add missing HA config keys (apache#3776)
839d63b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland approved these changes
svenvogel svenvogel approved these changes
weizhouapache Awaiting requested review from weizhouapache
andrijapanicsb Awaiting requested review from andrijapanicsb
GabrielBrascher Awaiting requested review from GabrielBrascher
+1 more reviewer
anuragaw anuragaw approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:management-server type:bug type:enhancement

Projects

None yet

Milestone

4.13.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL