Better tracking host maintanence and handling of migration jobs by anuragaw · Pull Request #3425 · apache/cloudstack

anuragaw · 2019-06-26T08:11:23Z

We want to update how host enters maintenance and it's states change. There have been instances when host was stuck in PrepareForMaintenance state indefinitely because all states weren't accounted for.

Also instead of moving direct to ErrorInMaintenance state on first fail, we want to wait for all legitimate operations to complete before entering ErrorInMaintenance state. If there are errors during in PrepareForMaintenance state, and errors are encountered, we enter the host into a transitory ErrorInPrepareForMaintenance state. This allows for better clarity if the operation had failed.

There are checks added to starting prepareHostForMaintenance. We should fail the API without any migration attempts if -

there are VMs in Starting, Error, Shutdown state on host or
if there are incoming migrations to the host.

Below is the flow of new host FSM

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?

Unit Tests
Marvin Tests
Manually

nvazquez · 2019-06-26T12:58:08Z

    @Override
    public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey<?>[] {HostMaintenanceRetries};
+        return new ConfigKey<?>[] {KvmSshToAgentEnabled};


No need to add this config key as it is already present on DB

For the sake of completeness (readability) should we not export it anyway? I mean if there are no repurcussions to it we should we not keep it as is?

Is this exported by some other manager/class? Then there is no need to re-export it.

anuragaw · 2019-06-27T04:41:22Z

    @Override
    public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey<?>[] {HostMaintenanceRetries};
+        return new ConfigKey<?>[] {KvmSshToAgentEnabled};


For the sake of completeness (readability) should we not export it anyway? I mean if there are no repurcussions to it we should we not keep it as is?

yadvr · 2019-06-28T06:30:43Z

Can you fix the conflicts @anuragaw and rebase against latest master?

nvazquez · 2019-06-29T22:37:40Z

+        final boolean hasVmsInFailureStates = CollectionUtils.isNotEmpty(errorVms);
+        errorVms.addAll(failedMigrations);
+
+        if (!hasPendingMigrationWorks && (hasRunningVms || (!hasRunningVms && !hasMigratingVms && hasVmsInFailureStates))) {


Maybe this could be a separate method with a javadoc to explain it? To improve readability

Moving to another method.

nvazquez · 2019-06-29T22:37:49Z

+            return setHostIntoErrorInMaintenance(host, errorVms);
+        }
+
+        if ((hasVmsInFailureStates || hasFailedMigrations) && (hasPendingMigrationWorks || hasMigratingVms || CollectionUtils.isNotEmpty(_vmDao.findByHostInStates(hostId, State.Stopping)))) {


Same as above

Refactoring to different methods seems like an overhead. I've refactored a little with some javadocs to better readability.

borisstoyanov · 2019-07-02T06:30:08Z

@blueorangutan package

blueorangutan · 2019-07-02T06:30:31Z

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

blueorangutan · 2019-07-02T07:07:31Z

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

yadvr · 2019-07-03T06:38:59Z

@blueorangutan test

blueorangutan · 2019-07-03T06:39:35Z

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

yadvr · 2019-07-03T06:40:13Z

+    ErrorInMaintenance,
+    Maintenance,
+    Error,
+    PrepareForMaintenanceErrorsPresent;


Can you rename PrepareForMaintenanceErrorsPresent to ErrorInPrepareForMaintenance to be uniform with other names such as ErrorInMaintenance.

PrepareForMaintenanceErrorsPresent is a transitional state that can go back to PrepareForMaintenance if Admin fixes errors.

This ultimately ends in ErrorInMaintenance or Maintenance state which are final states to be reached. Let me update the PR description @rhtyd

(minor nit)

I understand, if the intent of the new state to say errors happened while doing ... then I'm simply asking the name of the new state to be uniform with other state names. For example, Maintenance has ErrorInMaintenance; so PrepareForMaintenance can have a similar ErrorInPrepareForMaintenance state.

Okay. I get your point. Fixing in the next commit.

yadvr · 2019-07-03T06:41:07Z


    protected static final Logger s_logger = Logger.getLogger(HighAvailabilityManagerImpl.class);
+    private ConfigKey<Integer> MaxRetries = new ConfigKey<>("Advanced", Integer.class,
+            "max.retries","5",


max.retries is too generic, can you rename this to vm.ha.migration.max.retries or something suitable?

This is an old config file in the code but was not exposed to be configured. Renaming makes sense. Doing it.

If the global setting already exists, then don't rename them (for backward compatibility reasons) or re-define them. If they are new settings rename them.

These existed before. Leaving them as is.

I checked 4.13, the global setting max.retries does not exist in it however in code it is indeed referenced. Okay to leave as is, in that case.

Yes. This was a bug because max.retries wasn't accessible via the API. We have fixed that here.

@anuragaw @rhtyd
sorry I have some late comments as this PR has been merged.

max.retries is not used in the past so I suggest to rename it before 4.14 release.

this config has scope=Cluster but cluster-wide configuration is never used in code.

yadvr · 2019-07-03T06:42:32Z

    }

-    @Test
-    public void testCheckAndMaintainEnterMaintenanceMode() throws NoTransitionException {


@anuragaw Can you add unit tests for the new cases, or instead of removing old ones fix/refactor them?

Doing it @rhtyd , along with @nvazquez refactoring suggestions.

yadvr · 2019-07-03T06:43:22Z

@nvazquez @borisstoyanov have you reviewed and tested it?

blueorangutan · 2019-07-03T15:05:37Z

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

anuragaw · 2019-07-08T06:58:41Z

Refactoring and addressing reviews today. Apologies but I got occupied on other PRs.

yadvr · 2019-07-11T08:21:47Z

Moved this to 4.13.1.0 unless we can manage to get it reviewed/tested cc @borisstoyanov @PaulAngus

borisstoyanov · 2019-07-18T08:56:15Z

@blueorangutan package

blueorangutan · 2019-07-18T08:56:45Z

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

blueorangutan · 2019-07-18T09:04:38Z

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-149

borisstoyanov

@anuragaw there seems to be some issues building this one
[INFO] Apache CloudStack Server .......................... FAILURE [1:22.280s]

[ERROR] Failures:
[ERROR] ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations:203
Wanted but not invoked:
resourceManagerImpl.setHostIntoErrorInMaintenance(
host,
[vm1, vm2]
);
-> at com.cloud.resource.ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations(ResourceManagerImplTest.java:203)

However, there were other interactions with this mock:
resourceManagerImpl.checkAndMaintain(1);
-> at com.cloud.resource.ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations(ResourceManagerImplTest.java:201)

resourceManagerImpl.attemptMaintain(host);
-> at com.cloud.resource.ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations(ResourceManagerImplTest.java:201)

resourceManagerImpl.setHostIntoMaintenance(
host
);
-> at com.cloud.resource.ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations(ResourceManagerImplTest.java:201)

resourceManagerImpl.resourceStateTransitTo(
host,
InternalEnterMaintenance,
2886795268
);
-> at com.cloud.resource.ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceFailedMigrations(ResourceManagerImplTest.java:201)

[ERROR] ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceMigratingVms:195
[ERROR] ResourceManagerImplTest.testCheckAndMaintainErrorInMaintenanceRunningVms:187
[INFO]
[ERROR] Tests run: 791, Failures: 3, Errors: 0, Skipped: 5
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:

anuragaw · 2019-07-18T09:21:08Z

@borisstoyanov - looking at it Bobby, looks like I may have missed pushing some changes to the PR before transitioning to some urgent work.

yadvr · 2019-12-07T22:10:19Z

Is this still in progress or ready for review/testing - @anuragaw cc @borisstoyanov @andrijapanicsb ?

anuragaw · 2019-12-08T13:39:47Z

This has undergone some testing as far as I know from @borisstoyanov and should be ready for merging once either @andrijapanicsb or @PaulAngus give up a thumbs up. I'll remove the WIP after that so that it can be merged.

borisstoyanov

LGTM based on code review and test results
test_results.xlsx

andrijapanicsb · 2019-12-17T23:39:07Z

ping @anuragaw seems some conflicts - can you please handle it

anuragaw · 2019-12-18T03:10:21Z

Rebased against master.
ping for final review @rhtyd , @nvazquez , @DaanHoogland , @borisstoyanov , @andrijapanicsb , @shwstppr

anuragaw · 2019-12-18T04:16:06Z

@blueorangutan package

blueorangutan · 2019-12-18T04:16:12Z

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

blueorangutan · 2019-12-18T04:34:51Z

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

anuragaw · 2019-12-18T04:36:35Z

@blueorangutan test matrix

blueorangutan · 2019-12-18T04:37:13Z

@anuragaw a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

anuragaw · 2019-12-18T15:48:54Z

@andrijapanicsb - I've reverted to the commit that existed in the morning as per Github history per our offline conversation.

@rhtyd , @DaanHoogland , @shwstppr , @nvazquez - can someone of you also take a quick look and LGTM? It has only one LGTM from @borisstoyanov

anuragaw · 2019-12-18T15:49:17Z

@blueorangutan package

blueorangutan · 2019-12-18T15:50:15Z

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

blueorangutan · 2019-12-18T16:09:03Z

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

andrijapanicsb · 2019-12-18T22:33:20Z

@blueorangutan test matrix

blueorangutan · 2019-12-18T22:34:16Z

@andrijapanicsb a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

andrijapanicsb · 2019-12-18T22:36:03Z

Let's wait for the tests to confirm no regression since PR was rebased and commit/revert commit in the meantime. Otherwise, I give it LGTM

shwstppr

Code changes LGTM

blueorangutan · 2019-12-19T07:45:51Z

Trillian test result (tid-643)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31523 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3425-t643-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

anuragaw · 2019-12-19T07:52:42Z

KVM and XenServer results are expected with more delay as the job had failed initially.

blueorangutan · 2019-12-19T15:14:32Z

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

Test	Result	Time (s)	Test File
test_01_scale_vm	`Failure`	13.35	test_scale_vm.py

blueorangutan · 2019-12-19T15:27:27Z

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

Test	Result	Time (s)	Test File

anuragaw · 2019-12-19T15:32:35Z

Test failure in XenServer isnt related. @andrijapanicsb , @DaanHoogland - ready to merge?

andrijapanicsb · 2019-12-19T15:36:09Z

Looks good - all tests fine and enough LGTM.

svenvogel added type:enhancement component:management-server labels Jun 26, 2019

yadvr requested a review from nvazquez June 26, 2019 09:21

yadvr added this to the 4.13.0.0 milestone Jun 26, 2019

nvazquez reviewed Jun 26, 2019

View reviewed changes

anuragaw commented Jun 27, 2019

View reviewed changes

nvazquez reviewed Jun 29, 2019

View reviewed changes

anuragaw force-pushed the host_maintainence branch from c02ee4d to 5a15b8d Compare July 1, 2019 09:02

yadvr reviewed Jul 3, 2019

View reviewed changes

yadvr modified the milestones: 4.13.0.0, 4.13.1.0 Jul 11, 2019

anuragaw force-pushed the host_maintainence branch 2 times, most recently from bb20254 to d660930 Compare July 12, 2019 07:34

borisstoyanov requested changes Jul 18, 2019

View reviewed changes

yadvr added this to the 4.14.0.0 milestone Dec 7, 2019

borisstoyanov approved these changes Dec 10, 2019

View reviewed changes

anuragaw force-pushed the host_maintainence branch from 1c90d88 to a43482a Compare December 18, 2019 03:09

anuragaw requested a review from borisstoyanov December 18, 2019 03:10

anuragaw force-pushed the host_maintainence branch from a43482a to 1c90d88 Compare December 18, 2019 15:35

shwstppr approved these changes Dec 19, 2019

View reviewed changes

anuragaw changed the title ~~[WIP DO NOT MERGE] Better tracking host maintanence success and failure~~ Better tracking host maintanence and handling of migration jobs Dec 19, 2019

andrijapanicsb merged commit 4b43c26 into apache:master Dec 19, 2019

Conversation

anuragaw commented Jun 26, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Jul 2, 2019

Uh oh!

blueorangutan commented Jul 2, 2019

Uh oh!

blueorangutan commented Jul 2, 2019

Uh oh!

yadvr commented Jul 3, 2019

Uh oh!

blueorangutan commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragaw Jul 3, 2019 • 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!

yadvr commented Jul 3, 2019

Uh oh!

blueorangutan commented Jul 3, 2019

Uh oh!

anuragaw commented Jul 8, 2019

Uh oh!

yadvr commented Jul 11, 2019

Uh oh!

borisstoyanov commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

anuragaw commented Jun 26, 2019 •

edited

Loading

anuragaw Jul 3, 2019 •

edited

Loading

anuragaw commented Dec 18, 2019 •

edited

Loading