Destroyvm also removes volumes #18

houthuis · 2018-07-24T12:11:07Z

Description

TBC

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

…ctoring to follow

DaanHoogland · 2018-07-26T10:47:41Z

@houthuis seems ypiu have conflicts, can you resolve those , please?

…hapeblue/cloudstack into destroyvm-also-removes-volumes

… destroyvm-also-removes-volumes # Conflicts: # api/src/main/java/org/apache/cloudstack/api/ApiConstants.java # server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

DaanHoogland

looks good

DaanHoogland · 2018-07-31T14:49:16Z

api/src/main/java/com/cloud/storage/VolumeApiService.java


-import java.net.MalformedURLException;
-
+import com.cloud.exception.ResourceAllocationException;


why do these imports need reordering?

DaanHoogland · 2018-07-31T14:51:45Z

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java

-
-import org.apache.log4j.Logger;
-
+import com.cloud.event.EventTypes;


no reason to reorder these imports is there?

DaanHoogland · 2018-07-31T14:52:34Z

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java

+                type = CommandType.LIST,
+                collectionType = CommandType.UUID,
+                entityType = VolumeResponse.class,
+                description = "Comma separated list of volume UUIDs",


I think you want to add to this description whether these are the volumes that will be preserved (or not)

DaanHoogland · 2018-07-31T14:53:48Z

api/src/main/java/org/apache/cloudstack/api/command/user/volume/DetachVolumeCmd.java

-import org.apache.cloudstack.api.BaseAsyncCmd;
-import org.apache.log4j.Logger;
-
+import com.cloud.event.EventTypes;


no change in imports....

DaanHoogland · 2018-07-31T14:54:43Z

api/src/main/java/org/apache/cloudstack/api/command/user/volume/DetachVolumeCmd.java

 // under the License.
 package org.apache.cloudstack.api.command.user.volume;

-import org.apache.cloudstack.api.BaseAsyncCmd;


actually this file is not changed safe the reordering of imports, please remove it from the PR

DaanHoogland · 2018-07-31T14:58:07Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

        }
    }

+    public Volume detachVolumesFromVM(long vmId, long volumeId) {


detachVolumesFromVM() takes only one volumeId so probably use singular in teh method name

refactored to detachVolumeViaDestroyVM(...) to avoid confusion

DaanHoogland · 2018-07-31T14:59:10Z

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java

        return expunge;
    }

+    public List<Long> getVolumes() {


getVolume(UU)Ids() ?

done, renamed field + api parameter

DaanHoogland · 2018-07-31T15:03:24Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

        }
        s_logger.debug("Found no ongoing snapshots on volume of type ROOT, for the vm with id " + vmId);

+        List<VolumeVO> volumes = new ArrayList<>();


It would seem that you would first want to destroy the VM and only after try the volumes

DaanHoogland · 2018-07-31T15:05:03Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+    private void detachAndDeleteVolumes(List<VolumeVO> volumes) {
+
+        for (VolumeVO volume : volumes) {
+            _volumeService.detachVolumesFromVM(volume.getInstanceId(), volume.getId());


are we stopping if one fails?

DaanHoogland · 2018-07-31T15:05:14Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+        }
+
+        for (VolumeVO volume : volumes) {
+            _volumeService.deleteVolume(volume.getId(), CallContext.current().getCallingAccount());


same here: are we stopping if one fails?

DaanHoogland · 2018-07-31T17:43:53Z

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java

                collectionType = CommandType.UUID,
                entityType = VolumeResponse.class,
-                description = "Comma separated list of volume UUIDs",
+                description = "Comma separated list of volume UUIDs that will be deleted",


"Comma separated list of UUIDs for volumes that will be deleted" ?

DaanHoogland · 2018-07-31T17:45:22Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

    }

-    public Volume detachVolumesFromVM(long vmId, long volumeId) {
+    public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {


Why would this be specific to destroy vm ? isn't it just a detach method and didn't it already exist?

DaanHoogland · 2018-07-31T17:56:18Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

        }
    }

+    public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {


couple of questions here:
Is this really public? it may a only be called as part of destroy volume!
How different is it really (even being consequential) from regular detachVolumeFromVM? it should at least also fire a detach event to keep usage data in check.

DaanHoogland · 2018-07-31T17:58:08Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+
+        checkForUnattachedVolumes(vmId, volumes);
+        validateVolumes(volumes);
+


detach here

DaanHoogland · 2018-07-31T17:58:22Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

            }
        }

+        detachAndDeleteVolumes(volumes);


delete detached volumes here

DaanHoogland · 2018-07-31T18:03:42Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java


+        List<VolumeVO> volumes = new ArrayList<>();
+
+        if (cmd.getVolumeIds() != null) {


can you extract this bit into a method as well?

DaanHoogland · 2018-07-31T18:04:04Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

        }
        return false;
    }
+


the below methods can do with unit tests

borisstoyanov

Hi Henko, looking at the attached picture, this is a VM with no volumes attached. I think it'll make sense if the user select the checkbox, instead of display radio buttons of IDs, to display a simple text message 'No Volumes attached.'

…hapeblue/cloudstack into destroyvm-also-removes-volumes

borisstoyanov

LGTM, all remarks from my side has been addressed.

borisstoyanov · 2018-08-06T09:07:26Z

@houthuis I think you're in position to submit this to community now

…g a vm

rohityadavcloud · 2018-12-24T07:03:08Z

@dhlaluku can you pick this up, resubmit the changes?

dhlaluku · 2019-01-06T10:47:48Z

@rhtyd I am working on it, I will submit a new PR when it's complete since I can't reopen this one.

* Explicitly controlling VMware VM hardware version * Fix unit test

Fixes: #17 Fixes: #18 These dependencies are necessary or eslint will fail with the standard vue.js rule set. The PR also includes fixes for all lint errors. Signed-off-by: Rohit Yadav <[email protected]>

henko holtzhausen and others added 4 commits July 9, 2018 12:54

WIP volume validation, volume detach

4a154b6

WIP delete volume code added. functionally complete, cleanup and refa…

880b38e

…ctoring to follow

WIP ui code

eb929a0

Adding a test for DestroyVM with volumes

12e627b

henko holtzhausen added 5 commits July 31, 2018 15:10

WIP front end complete

e953db9

Merge branch 'destroyvm-also-removes-volumes' of https://github.com/s…

fbc3fdf

…hapeblue/cloudstack into destroyvm-also-removes-volumes

code cleanup

d2f3b35

Merge branch 'master' of https://github.com/shapeblue/cloudstack into…

bff19b3

… destroyvm-also-removes-volumes # Conflicts: # api/src/main/java/org/apache/cloudstack/api/ApiConstants.java # server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

improved wording

3c7e0cc

DaanHoogland approved these changes Jul 31, 2018

View reviewed changes

changes following code review

83ae447

DaanHoogland reviewed Jul 31, 2018

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

}

return false;

}

Copy link

Member

DaanHoogland Jul 31, 2018

Choose a reason for hiding this comment

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

the below methods can do with unit tests

changes following code review

6129eb6

borisstoyanov reviewed Aug 2, 2018

View reviewed changes

borisstoyanov and others added 3 commits August 2, 2018 12:46

Adding a test for DestroyVM with volumes

ff5004e

ui enhancement when no volumes are attached to a vm

eb02664

Merge branch 'destroyvm-also-removes-volumes' of https://github.com/s…

9590da1

…hapeblue/cloudstack into destroyvm-also-removes-volumes

borisstoyanov approved these changes Aug 6, 2018

View reviewed changes

added code to allow users to detach and delete volumes when destroyin…

d996e77

…g a vm

houthuis closed this Aug 6, 2018

shwstppr pushed a commit that referenced this pull request Aug 24, 2020

[4.9.1] Explicitly controlling VMware VM hardware version (#18)

745a7b3

* Explicitly controlling VMware VM hardware version * Fix unit test


		import java.net.MalformedURLException;

		import com.cloud.exception.ResourceAllocationException;


		import org.apache.log4j.Logger;

		import com.cloud.event.EventTypes;


		checkForUnattachedVolumes(vmId, volumes);
		validateVolumes(volumes);


		List<VolumeVO> volumes = new ArrayList<>();

		if (cmd.getVolumeIds() != null) {

Destroyvm also removes volumes #18

Destroyvm also removes volumes #18

Uh oh!

Conversation

houthuis commented Jul 24, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

DaanHoogland commented Jul 26, 2018

Uh oh!

DaanHoogland left a comment

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!

DaanHoogland Jul 31, 2018 • 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!

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!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Aug 6, 2018

Uh oh!

rohityadavcloud commented Dec 24, 2018

Uh oh!

dhlaluku commented Jan 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

DaanHoogland Jul 31, 2018 •

edited

Loading