Allow VM that has never started to have volumes attached by anuragaw · Pull Request #3276 · apache/cloudstack

anuragaw · 2019-04-15T12:14:50Z

With this patch b766bf7
we started tracking disks in attaching state so that other attach request can fail gracefully. However this missed the case where disks were in allocated state but attach was requested.

For the use case where users want to attach disk in allocated state but not ready, we need to have allocated-attaching transition as well. We must take care of returning to the original state - allocated or ready - when attach request has completed.

For the use case of unstarted vm's the disk must proceed as follows - "Allocated" -> Attaching -> Allocated. When VM is started, the disk is "created" and pool is assigned. For the use case of started VMs it's more trivial and disk proceeds as follows - Ready -> Attaching -> Ready.

Test this by creating a VM with "startvm=false", create a disk and try attaching it in allocated state. It would give an exception on latest 4.11 but will be fixed on this patch.

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?

Manually (using CloudMonkey, dashboard) and through Marvin Test

for the use case where users want to attach disk in allocated state but not ready, we need to have more states for keeping track of attaching state. Test this by creating a VM with "startvm=false", create a disk and try attaching it in allocated state. It would give an exception on latest 4.11 but will be fixed on this patch.

anuragaw · 2019-04-15T12:15:52Z

@blueorangutan package

blueorangutan · 2019-04-15T12:16:27Z

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

blueorangutan · 2019-04-15T12:37:14Z

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

anuragaw · 2019-04-15T12:41:49Z

@blueorangutan test

blueorangutan · 2019-04-15T12:42:27Z

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

blueorangutan · 2019-04-15T22:28:03Z

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

Test	Result	Time (s)	Test File
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80	`Failure`	416.00	test_internal_lb.py

yadvr · 2019-04-16T11:08:42Z

@anuragaw can you implement a marvin test for the test-case.

anuragaw · 2019-04-17T09:18:20Z

@blueorangutan package

blueorangutan · 2019-04-17T09:19:35Z

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

blueorangutan · 2019-04-17T09:40:59Z

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

anuragaw · 2019-04-17T09:50:20Z

@blueorangutan test

anuragaw · 2019-04-17T09:50:53Z

@anuragaw can you implement a marvin test for the test-case.

Done @rhtyd , let me know if this covers the case . Thanks!

yadvr · 2019-04-17T10:11:45Z

        UploadError("Volume upload encountered some error"),
        UploadAbandoned("Volume upload is abandoned since the upload was never initiated within a specificed time"),
-        Attaching("The volume is attaching to a VM");
+        AttachingFromReady("The volume is attaching to a VM from Ready State"),


@anuragaw don't change the original state Attaching to honour db volume states that may already be in Attaching state in db. This may cause usage issues post an upgrade.

Ah, my bad. Fixing.

yadvr · 2019-04-17T10:14:57Z

-            handleTargetsForVMware(hostId, volumePool.getHostAddress(), volumePool.getPort(), volume.get_iScsiName());
+            // volumePool() should be null if the VM we are detaching the disk from has never been started before
+            // only revoke access on volumes that are actually on a datastore
+            if (volumePool != null) {


Should the original logic be retained? What happens if volumePool is null?

If the volumePool was null initially the dataStore was null in previous implementation and revokeAccess did nothing. That happens after this check as well.
Also I don't think it should be null as the handleTargetsForVMWare(hostId,volumePool.getHostAddress()... would crash.

yadvr · 2019-04-17T10:15:48Z

        UploadAbandoned("Volume upload is abandoned since the upload was never initiated within a specificed time"),
-        Attaching("The volume is attaching to a VM");
+        AttachingFromReady("The volume is attaching to a VM from Ready State"),
+        AttachingFromAllocated("The volume is attaching to a VM from Allocated State");


I think we can still fix the issue without introducing a new state. All we're saying is to allow volumes's attaching state whether their previous state is Ready or Allocated.

The reason we need a new state is that if an attach fails we want the disk to return to the original state it was in - Ready -> Attaching -> FAIL -> Ready, or Allocated -> Attaching -> FAIL -> Allocated.

Perhaps I am misunderstanding something?

Looking back at the original patch that caused regression, it seems that simple Allocated->Attaching would suffice. I'll change.

yadvr · 2019-04-17T10:16:28Z

-            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Ready, Event.AttachRequested, Attaching, null));
-            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Attaching, Event.OperationSucceeded, Ready, null));
-            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Attaching, Event.OperationFailed, Ready, null));
+            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Allocated, Event.AttachRequested, AttachingFromAllocated, null));


Let's keep this only and rest can follow the same, i.e. allow both Ready and Allocated state volumes to go into Attaching state on the event.

yadvr · 2019-04-17T10:17:24Z

        self.assertEqual(root_volume.podname, list_pods.name)

+    @attr(tags = ["advanced", "advancedns", "smoke", "basic"], required_hardware="true")
+    def test_11_attach_volume_with_unstarted_vm(self):


Test case LGTM. Also check Travis if this passes OK.

…g pool id in attaching state

anuragaw · 2019-04-18T09:51:15Z

@blueorangutan package

anuragaw · 2019-04-22T09:20:12Z

@blueorangutan package

blueorangutan · 2019-04-22T09:20:44Z

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

blueorangutan · 2019-04-22T09:43:30Z

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

anuragaw · 2019-04-22T10:04:51Z

@blueorangutan test

blueorangutan · 2019-04-22T10:05:44Z

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

anuragaw · 2019-04-22T13:18:04Z

@blueorangutan package

blueorangutan · 2019-04-22T13:18:44Z

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

blueorangutan · 2019-04-22T13:40:15Z

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

blueorangutan · 2019-04-22T14:34:16Z

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

Test	Result	Time (s)	Test File

blueorangutan · 2019-04-22T18:54:03Z

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

Test	Result	Time (s)	Test File

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2019-04-23T19:47:13Z

@blueorangutan package

blueorangutan · 2019-04-23T19:47:50Z

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

blueorangutan · 2019-04-23T20:12:03Z

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

yadvr · 2019-04-23T20:27:55Z

@blueorangutan test matrix

blueorangutan · 2019-04-23T20:28:51Z

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

blueorangutan · 2019-04-24T04:31:50Z

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

Test	Result	Time (s)	Test File

blueorangutan · 2019-04-24T04:38:59Z

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

Test	Result	Time (s)	Test File
test_01_scale_vm	`Error`	17.52	test_scale_vm.py

borisstoyanov

LGTM

yadvr changed the title ~~Add more states for attaching disk in allocated state~~ [DO NOT MERGE WIP] Add more states for attaching disk in allocated state Apr 16, 2019

yadvr added component:managed-storage component:primary-storage labels Apr 16, 2019

yadvr added this to the 4.11.3.0 milestone Apr 16, 2019

yadvr requested review from mike-tutkowski and rafaelweingartner and removed request for rafaelweingartner April 16, 2019 11:06

Add Marvin test for the use case of attaching volume to unstarted VM

d6a1e30

yadvr reviewed Apr 17, 2019

View reviewed changes

Addressed code review comments

bdd1021

DaanHoogland changed the title ~~[DO NOT MERGE WIP] Add more states for attaching disk in allocated state~~ [WIP DO NOT MERGE] Add more states for attaching disk in allocated state Apr 18, 2019

Revert to allocated state when cleaning up jobs for volumes not havin…

c5f7524

…g pool id in attaching state

Fix for tracking allocated vs ready state when attaching

c518aba

Don't transition at all for Allocated state

4952655

Simplify bugfix implementation

36646bf

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr changed the title ~~[WIP DO NOT MERGE] Add proper state transitions for attaching disk in allocated state~~ [WIP DO NOT MERGE] Allow VM that has never started to have volumes attached Apr 23, 2019

reduce diff

fa7e8bf

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

borisstoyanov approved these changes Apr 24, 2019

View reviewed changes

yadvr approved these changes Apr 24, 2019

View reviewed changes

yadvr changed the title ~~[WIP DO NOT MERGE] Allow VM that has never started to have volumes attached~~ Allow VM that has never started to have volumes attached Apr 24, 2019

yadvr requested review from GabrielBrascher, rafaelweingartner and wido April 24, 2019 11:32

yadvr approved these changes May 10, 2019

View reviewed changes

yadvr merged commit f9b61bc into apache:4.11 May 10, 2019

Conversation

anuragaw commented Apr 15, 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!

anuragaw commented Apr 15, 2019

Uh oh!

blueorangutan commented Apr 15, 2019

Uh oh!

blueorangutan commented Apr 15, 2019

Uh oh!

anuragaw commented Apr 15, 2019

Uh oh!

blueorangutan commented Apr 15, 2019

Uh oh!

blueorangutan commented Apr 15, 2019

Uh oh!

yadvr commented Apr 16, 2019

Uh oh!

anuragaw commented Apr 17, 2019

Uh oh!

blueorangutan commented Apr 17, 2019

Uh oh!

blueorangutan commented Apr 17, 2019

Uh oh!

anuragaw commented Apr 17, 2019

Uh oh!

anuragaw commented Apr 17, 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!

anuragaw Apr 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragaw Apr 18, 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!

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 commented Apr 18, 2019

Uh oh!

anuragaw commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

anuragaw commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

anuragaw commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

blueorangutan commented Apr 22, 2019

Uh oh!

yadvr commented Apr 23, 2019

Uh oh!

anuragaw commented Apr 15, 2019 •

edited

Loading

anuragaw Apr 18, 2019 •

edited

Loading

anuragaw Apr 18, 2019 •

edited

Loading