You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/11/03 13:37:24 UTC
[GitHub] [cloudstack] JoaoJandre opened a new pull request, #6868: Set root volume as destroyed when destroying a VM
JoaoJandre opened a new pull request, #6868:
URL: https://github.com/apache/cloudstack/pull/6868
### Description
When a user removes a VM, ACS puts it in a `Destroy` state and stops counting it towards the user resource limit. However, the VM root volume remains in `Ready` state and consuming resources until the VM cleanup task runs on ACS.
The `destroy.root.volume.on.vm.destruction` setting was created so that when it is set to true, when destroying a VM its root volume will also be placed in the `Destroy` state, no longer counting towards the user resource limit.
Also, while this setting is active, root volumes in the destroy state will not be removed by the ACS volume cleanup task, so that it is possible to restore the VM again. However, the VM cleanup task will still remove the VMs along with their respective volumes.
### 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)
- [X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
This was tested in a local lab.
| Nº | Test | Result |
| ------ | ------ | ------ |
|1| With the setting disabled, a VM was destroyed | the VM was in `Destroy` state and its root volume in `Ready` |
|2| With the setting enabled, a VM was destroyed | the VM was in `Destroy` state and its root volume in `Destroy` too |
|3| With the setting enabled, the test 2 VM was restored | The VM was correctly restored along with its root volume |
|4| Test 1 VM was restored | The VM was correctly restored |
|5| With the setting enabled, the test 3 VM was destroyed, its volume restored, and then the VM restored | When the volume was restored, it became `DATADISK` type (since it is detached when being recovered), then the VM was restored, the volume was reattached and the VM worked correctly |
|6| With the setting enabled, a VM was destroyed, and the cleanup task had its interval shortened | The cleanup task did not purge ROOT volume from VM |
|7| With the setting disabled, and with a ROOT volume in `Destroy` state the cleanup task had its interval shortened | ROOT volume was purged |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1302729629
> I think you are missing a (not necessarily new) use-case where the root disk is expunged and the VM restored. both with and without this setting this will go wrong. You just made it a bit more explicit with your code.
Right. Because vm can be recovered, the root disk should not be removed.
If root disk is set to Destroyed, it will be cleaned after a period, then the vm cannot be recovered.
If you want to exclude destroyed vm in resource count, please look at the methods for resource calculation. @JoaoJandre
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326736618
@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1330545623
> @weizhouapache does this solve all our concerns? @JoaoJandre please add guards for debug statements (that require parameter evaluation)
@DaanHoogland
yes, code lgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on code in PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#discussion_r1030736104
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1325,6 +1328,12 @@ public void cleanupStorage(boolean recurring) {
List<VolumeVO> vols = _volsDao.listVolumesToBeDestroyed(new Date(System.currentTimeMillis() - ((long)StorageCleanupDelay.value() << 10)));
for (VolumeVO vol : vols) {
+ if (Type.ROOT.equals(vol.getVolumeType()) && userVmManager.getDestroyRootVolumeOnVmDestruction(vol.getDomainId())) {
Review Comment:
@JoaoJandre
the vm state should be checked instead of the global/domain setting, because the setting can be changed.
if vm state is in Destroyed state, skip it and continue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on code in PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#discussion_r1014243203
##########
engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java:
##########
@@ -759,4 +759,11 @@ public void updateDiskOffering(long volumeId, long diskOfferingId) {
throw new CloudRuntimeException(e);
}
}
+ @Override
+ public VolumeVO getInstanceRootVolume(long instanceId, String instanceUuid) {
Review Comment:
This is a leftover from an earlier version, i'll remove it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi merged pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
GutoVeronezi merged PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1303854774
> I think you are missing a (not necessarily new) use-case where the root disk is expunged and the VM restored. both with and without this setting this will go wrong. You just made it a bit more explicit with your code.
I'm sorry @DaanHoogland, but I couldn't quite grasp what you meant here. You are correct, if you try to recover a VM which has had its root volume expunged it won't work. But I don't understand what that use case has to do with this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326454073
@JoaoJandre thanks for the update.
code looks ok to me.
it would be good if anyone can test it. cc @GutoVeronezi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1302207218
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6868)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_coverage&view=list) [14.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1303965473
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6868)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6868&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6868&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_coverage&view=list) [14.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6868&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1308821086
> > That is not what this PR intends. The current problem is: when a user destroys a VM, its root volume will remain in the ready state, but generally speaking, when you destroy a VM, you intend to destroy its root volume as well.
>
> @JoaoJandre that's not true. when destroy a vm, the vm is just marked as Destroyed. none of its resources (nics, storage, vm snapshots, etc) are removed. If you want to completely remove a vm and its resources, please use `expungeVirtualMachine`, or `destroyVirtualMachine` with `expunge=true`
>
> > The proposed solution is to create a global setting ( which will be disabled by default ), that when enabled, ACS will destroy the root volume of a VM when that VM is destroyed. However, because of this setting, a new use case for restoring VMs is created, to restore a VM with a destroyed root volume. Therefore, I created an exception on the volume restore process, so that when you try to restore a VM with a destroyed root volume, it will not fail.
Yes, I know that when you destroy a VM it is only marked as "destroyed". When VMs are marked as "destroyed", they no longer count towards the users' used resources. Therefore, after destroying a VM, they can right away create another one with the "freed quota". However, the same logic is not applied to root volumes; they are maintained in "ready" state. What I am addressing here is this inconsistency; the idea is to make the root volume and VMs to have the same behavior.
Let me give you a detailed example. Let's say a user has the following resource limits:
Therefore, the user can only have 1 VM and 1 volume. Then, if the user creates a VM, this is what its resource limit will look like:
Which is exactly as expected. Then, if the user destroys the VM, this is what the resource count will look like:
Therefore, if the user tries to create a new VM (he already destroyed the other one he had), this is what will happen:
That is the problem the PR is addressing. That is why I proposed this PR, to create a configuration that when enabled will also mark the root volume as destroyed to stop counting it towards users' resource quota.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1325446652
> >
> > @JoaoJandre in this case, the vm is destroyed, but volume is gone. right ? is it expected ?
>
> Here is all the possible cases:
>
> Nº Configuration state Action Result Cleanup
> 1 Disabled VM is destroyed Only the VM is set to `Destroyed` state, its root volume remains in `Ready` state Here the volume cleanup job will not expunge the volume, because the root volume is in `Ready` state, but the VM cleanup job will expunge both the VM and its volume
> 2 Enabled VM is destroyed Both the VM and its root volume are set to `Destroyed` state Here the volume cleanup job will not expunge the VM's root volume, because the volume's type is `ROOT` and the configuration is enabled, but the VM cleanup job will expunge both the VM and its volume
> 3 Enabled VM is destroyed, then the configuration is disabled Both the VM and its root volume are set to Destroyed state Here the volume cleanup job will expunge the VM's root volume, because the configuration was disabled, the VM cleanup job will expunge the VM (and its volume, if this runs before the volume cleanup job)
> The first case is current behavior, this PR does not change that. This PR introduces cases 2 and 3.
>
> So answering your question:
>
> > @JoaoJandre in this case, the vm is destroyed, but volume is gone. right ? is it expected ?
>
> Yes, this will happen in case 3, that is expected.
@JoaoJandre thanks for your reply.
in case 3, ROOT volume can be expunged, this is what we need to prevent, as the vm is not recoverable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326451878
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1305873054
> That is not what this PR intends. The current problem is: when a user destroys a VM, its root volume will remain in the ready state, but generally speaking, when you destroy a VM, you intend to destroy its root volume as well.
@JoaoJandre
that's not true. when destroy a vm, the vm is just marked as Destroyed. none of its resources (nics, storage, vm snapshots, etc) are removed.
If you want to completely remove a vm and its resources, please use `expungeVirtualMachine`, or `destroyVirtualMachine` with `expunge=true`
> The proposed solution is to create a global setting ( which will be disabled by default ), that when enabled, ACS will destroy the root volume of a VM when that VM is destroyed. However, because of this setting, a new use case for restoring VMs is created, to restore a VM with a destroyed root volume. Therefore, I created an exception on the volume restore process, so that when you try to restore a VM with a destroyed root volume, it will not fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a diff in pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on code in PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#discussion_r1018245216
##########
server/src/main/java/com/cloud/vm/UserVmManager.java:
##########
@@ -55,6 +55,10 @@ public interface UserVmManager extends UserVmService {
ConfigKey<Boolean> DisplayVMOVFProperties = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.display.ovf.properties", "false",
"Set display of VMs OVF properties as part of VM details", true);
+ ConfigKey<Boolean> DestroyRootVolumeOnVmDestruction = new ConfigKey<Boolean>("Advanced", Boolean.class, "destroy.root.volume.on.vm.destruction", "false",
+ "Destroys the VM's root volume when the VM is destroyed. When set to true, volume cleanup task will not expunge destroyed volumes, but the VM cleanup task will.",
Review Comment:
```suggestion
"Indicates whether the VM's ROOT volume will be marked as 'Destroy' when the VM is destroyed. When set to false (default), the current behavior will be kept (the volume will continue as 'Ready' and the volume cleanup task will expunge it). When set to true, the volume will be marked as `Destroy` and the VM cleanup task will expunge it along with the VM (the volume cleanup task will not expunge it).",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1339987731
@weizhouapache, I tested the PR and had the following results:
First I configured the environment to run the cleanup tasks every 5 seconds, with a delay before expunging the resources of 45 seconds for volumes and 90 seconds for VMs:
```
storage.cleanup.interval = 5
expunge.interval = 5
storage.cleanup.delay = 45
expunge.delay = 90
```
I then created a specific account for the tests.
---
With the configuration `destroy.root.volume.on.vm.destruction` as `false` (i.e.: the current behavior), I did 3 tests:
1. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM. The ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. After 45 seconds, the data disk was expunged[^data-disk-current-behavior] and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
2. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM while the ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. Then, I recovered the VM and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the ROOT disk. The account was still consuming 1 volume. Finally, I recovered the data disk and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the data disk. Also, I verified that the account was consuming 2 volumes after recovering both ROOT and data disks.
3. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM. The ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. I changed the configuration `destroy.root.volume.on.vm.destruction` to `true`. Like the first test, after 45 seconds, the data disk was expunged[^data-disk-current-behavior] and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
Both tests presented the expected result: ROOT volume stayed in `Ready` state until the VM expunge and the account kept consuming 1 volume until the ROOT disk was expunged.
---
With the configuration `destroy.root.volume.on.vm.destruction` as `true` (i.e.: the current behavior), I did 3 tests:
1. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. After 45 seconds, the data disk was expunged[^data-disk-current-behavior] and the ROOT disk stayed in `Destroy` state. After 90 seconds, the ROOT volume was expunged along with the VM.
2. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. Then, I recovered the VM and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the ROOT disk and the account was consuming 1 volume. Finally, I recovered the data disk and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the data disk. Also, I verified that the account was consuming 2 volumes after recovering both ROOT and data disks.
3. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. I changed the configuration `destroy.root.volume.on.vm.destruction` to `false`. Like the first test, after 45 seconds, the data disk was expunged[^data-disk-current-behavior] and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
Both tests presented the expected result: ROOT volume was put in `Destroy` state and was properly expunged only with the VM expunge task, and the account was consuming 0 volumes.
---
Based on the results of the manual test, [Trillian](https://github.com/apache/cloudstack/pull/6868#issuecomment-1327034590)[^error-trillian] tests results, Travis results, [packaging results](https://github.com/apache/cloudstack/pull/6868#issuecomment-1326501535) and approvals, I will merge this PR.
[^data-disk-current-behavior]: as the data disks are detached from the VM, the disk expunge task will expunge it. This is the current behavior and was not touched by the PR.
[^error-trillian]: error in Trillian does not seem related to the PR changes: `AssertionError: Expected '1' routers at state 'PRIMARY', but found '0'!`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] codecov[bot] commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1302212302
# [Codecov](https://codecov.io/gh/apache/cloudstack/pull/6868?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6868](https://codecov.io/gh/apache/cloudstack/pull/6868?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3cd0281) into [main](https://codecov.io/gh/apache/cloudstack/commit/fa39e61a4ccd33ccb9b81481eb57c64b3adaf6f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fa39e61) will **increase** coverage by `0.00%`.
> The diff coverage is `21.73%`.
```diff
@@ Coverage Diff @@
## main #6868 +/- ##
=========================================
Coverage 10.84% 10.85%
- Complexity 7104 7105 +1
=========================================
Files 2485 2485
Lines 245417 245439 +22
Branches 38326 38329 +3
=========================================
+ Hits 26627 26632 +5
- Misses 215521 215537 +16
- Partials 3269 3270 +1
```
| [Impacted Files](https://codecov.io/gh/apache/cloudstack/pull/6868?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...main/java/com/cloud/storage/dao/VolumeDaoImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6868/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZW5naW5lL3NjaGVtYS9zcmMvbWFpbi9qYXZhL2NvbS9jbG91ZC9zdG9yYWdlL2Rhby9Wb2x1bWVEYW9JbXBsLmphdmE=) | `23.62% <0.00%> (-0.23%)` | :arrow_down: |
| [...ain/java/com/cloud/storage/StorageManagerImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6868/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3N0b3JhZ2UvU3RvcmFnZU1hbmFnZXJJbXBsLmphdmE=) | `0.89% <0.00%> (-0.01%)` | :arrow_down: |
| [.../src/main/java/com/cloud/vm/UserVmManagerImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6868/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1VzZXJWbU1hbmFnZXJJbXBsLmphdmE=) | `6.79% <28.57%> (+0.07%)` | :arrow_up: |
| [...rver/src/main/java/com/cloud/vm/UserVmManager.java](https://codecov.io/gh/apache/cloudstack/pull/6868/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1VzZXJWbU1hbmFnZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1310167126
> DATADISK and ROOT disks are different. When you destroy a vm, DATADISK will be detached from the VM, but ROOT disk will not. The VM can be recovered, no matter what the state of DATADISK is, because DATADISK is not attached.
@weizhouapache default behavior is that the datadisk is not detached. Datadisks will only be detached if you pass the Datadisk id to the destroyVIrtualMachine API, then they will be detached and destroyed.
> I prefer to make changes with minimum impact. If you want to change the calculation of resource count, add a global config in ResourceLimitService, and use it in ResourceLimitManagerImpl.
Your solution is almost the same as mine, but without changing the root volume's state, which could make thinks confusing for users, because not counting a volume which is in ready state is almost an inconsistency.
What do you think @DaanHoogland ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1310244450
> > DATADISK and ROOT disks are different. When you destroy a vm, DATADISK will be detached from the VM, but ROOT disk will not. The VM can be recovered, no matter what the state of DATADISK is, because DATADISK is not attached.
>
> @weizhouapache default behavior is that the datadisk is not detached. Datadisks will only be detached if you pass the Datadisk id to the destroyVIrtualMachine API, then they will be detached and destroyed.
>
@JoaoJandre
I just had a quick test. The datadisk id is not passed to destroyVIrtualMachine API, but it is detached when destroy a vm.
> > I prefer to make changes with minimum impact. If you want to change the calculation of resource count, add a global config in ResourceLimitService, and use it in ResourceLimitManagerImpl.
>
> Your solution is almost the same as mine, but without changing the root volume's state, which could make thinks confusing for users, because not counting a volume which is in ready state is almost an inconsistency. What do you think @DaanHoogland ?
@JoaoJandre
there is a big difference: the root volume is set to Destroy state in your PR, which is what I disagree.
Can you please answer the question in my previous comment ?
```
For example, the configuration is true when you destroy a vm, the volume is marked as Destroy. If you change the configuraton to false afterwards, the volume will be removed, right ?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326452316
@weizhouapache a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1327034590
<b>Trillian test result (tid-5264)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40549 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6868-t5264-kvm-centos7.zip
Smoke tests completed. 103 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 458.36 | test_vpc_redundant.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1322286583
> Can you please answer the question in my previous comment ?
For example, the configuration is true when you destroy a vm, the volume is marked as Destroy. If you change the configuraton to false afterwards, the volume will be removed, right ?
Yes, if you have the configuration enabled with a root volume marked as destroy, and you disable the configuration, the root volume will be subject to the volume cleanup job. This is expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1309069696
@weizhouapache, about the comments:
> @JoaoJandre When vm is destroyed, it is not usable, so vm is not considered in the resource count calculation of instance and cpu/memory. But the ROOT volume is still stored on primary storage. I think it makes sense to consider it in calculation of primary storage resource count. There is a similar scenario: the volumes in Destroy state are also considered in resource count calculation.
and
> If you want users are able to create a new vm after they destroy a vm, please expunge the vm instead of destroy. There are two global settings : allow.user.expunge.recover.vm and allow.user.expunge.recover.volume, when they are set to 0, users can expunge vms and volumes.
Destroying a volume without expunging is a background security measure that businesses adopt in case of user mistake/regret or even ransomware attacks. This kind of situation normally is handled by other means instead of blocking the user (e.g: the plan/billing already covers this kind of situation). Also, enabling users to expunge the volume is counter-productive for those background security measures. Therefore, the options we have in ACS do not match the use case.
Regarding your comment `the volumes in Destroy state are also considered in resource count calculation`, it is not correct; if you put a volume in `Destroy` state, they are not accounted for the domain/account resources.
Furthermore, the proposed behavior is controlled by a configuration, meaning that operators will decide to use it or not; the default behavior is maintained. Also, if you have a VM with DATADISKs and destroy the VM selecting the DATADISKs, they will be put in `Destroy` state; however, the ROOT is kept as `Ready`, which is not consistent.
> Back to your topic, I think it is a bad idea to mark the root volume as Destroyed, as the root volume will be finally removed by background thread and vm cannot be recovered.
If you look at the code you will notice that the expunge volume thread will not expunge a ROOT volume if the configuration is enabled; the volume expunging will be done by the VM thread, as it is the current behavior with volumes in `Ready` state.
With that, as the proposed behavior is optional and the use case is concrete, the proposal and the changes make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1332342930
> Trillian test result (tid-5264) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 40549 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6868-t5264-kvm-centos7.zip Smoke tests completed. 103 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:
> Test Result Time (s) Test File
> test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers `Failure` 458.36 test_vpc_redundant.py
I checked the logs and this error does not seem related to my change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1340017215
> @weizhouapache, I tested the PR and had the following results:
>
> First I configured the environment to run the cleanup tasks every 5 seconds, with a delay before expunging the resources of 45 seconds for volumes and 90 seconds for VMs:
>
> ```
> storage.cleanup.interval = 5
> expunge.interval = 5
> storage.cleanup.delay = 45
> expunge.delay = 90
> ```
>
> I then created a specific account for the tests.
>
> With the configuration `destroy.root.volume.on.vm.destruction` as `false` (i.e.: the current behavior), I did 3 tests:
>
> 1. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM. The ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. After 45 seconds, the data disk was expunged[1](#user-content-fn-data-disk-current-behavior-a0e85227db694c14ea74db2a4fff2a27) and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
> 2. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM while the ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. Then, I recovered the VM and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the ROOT disk. The account was still consuming 1 volume. Finally, I recovered the data disk and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the data disk. Also, I verified that the account was consuming 2 volumes after recovering both ROOT and data disks.
> 3. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the data disk was put in `Destroy` state and it was detached from the VM. The ROOT disk stayed in `Ready` state and attached to the VM. After deleting the VM, I verified that the account was consuming 1 volume. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. I changed the configuration `destroy.root.volume.on.vm.destruction` to `true`. Like the first test, after 45 seconds, the data disk was expunged[1](#user-content-fn-data-disk-current-behavior-a0e85227db694c14ea74db2a4fff2a27) and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
>
> Both tests presented the expected result: ROOT volume stayed in `Ready` state until the VM expunge and the account kept consuming 1 volume until the ROOT disk was expunged.
>
> With the configuration `destroy.root.volume.on.vm.destruction` as `true` (i.e.: the current behavior), I did 3 tests:
>
> 1. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. After 45 seconds, the data disk was expunged[1](#user-content-fn-data-disk-current-behavior-a0e85227db694c14ea74db2a4fff2a27) and the ROOT disk stayed in `Destroy` state. After 90 seconds, the ROOT volume was expunged along with the VM.
> 2. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. Then, I recovered the VM and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the ROOT disk and the account was consuming 1 volume. Finally, I recovered the data disk and a record of type `VOLUME.CREATE` was persisted in table `cloud.usage_event` for the data disk. Also, I verified that the account was consuming 2 volumes after recovering both ROOT and data disks.
> 3. I created a VM with a data disk and verified that the account was consuming 2 volumes. Then, I destroyed the VM along with the data disk (without expunging); the ROOT and the data disks were put in `Destroy` state and were detached from the VM. After deleting the VM, I verified that the account was consuming 0 volumes. Two records of type `VOLUME.DELETE` were persisted in table `cloud.usage_event`. I changed the configuration `destroy.root.volume.on.vm.destruction` to `false`. Like the first test, after 45 seconds, the data disk was expunged[1](#user-content-fn-data-disk-current-behavior-a0e85227db694c14ea74db2a4fff2a27) and the ROOT disk stayed in `Ready` state and attached to the VM. After 90 seconds, the ROOT volume was expunged along with the VM. Finally, I verified that the account was consuming 0 volumes.
>
> Both tests presented the expected result: ROOT volume was put in `Destroy` state and was properly expunged only with the VM expunge task, and the account was consuming 0 volumes.
>
> Based on the results of the manual test, [Trillian](https://github.com/apache/cloudstack/pull/6868#issuecomment-1327034590)[2](#user-content-fn-error-trillian-a0e85227db694c14ea74db2a4fff2a27) tests results, Travis results, [packaging results](https://github.com/apache/cloudstack/pull/6868#issuecomment-1326501535) and approvals, I will merge this PR.
>
> ## Footnotes
> 1. as the data disks are detached from the VM, the disk expunge task will expunge it. This is the current behavior and was not touched by the PR. [↩](#user-content-fnref-data-disk-current-behavior-a0e85227db694c14ea74db2a4fff2a27) [↩2](#user-content-fnref-data-disk-current-behavior-2-a0e85227db694c14ea74db2a4fff2a27) [↩3](#user-content-fnref-data-disk-current-behavior-3-a0e85227db694c14ea74db2a4fff2a27) [↩4](#user-content-fnref-data-disk-current-behavior-4-a0e85227db694c14ea74db2a4fff2a27)
> 2. error in Trillian does not seem related to the PR changes: `AssertionError: Expected '1' routers at state 'PRIMARY', but found '0'!`. [↩](#user-content-fnref-error-trillian-a0e85227db694c14ea74db2a4fff2a27)
Great, thanks for your testing. @GutoVeronezi
The test results looks pretty good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#discussion_r1013061381
##########
engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java:
##########
@@ -759,4 +759,11 @@ public void updateDiskOffering(long volumeId, long diskOfferingId) {
throw new CloudRuntimeException(e);
}
}
+ @Override
+ public VolumeVO getInstanceRootVolume(long instanceId, String instanceUuid) {
Review Comment:
why add the uuid parameter?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2335,6 +2337,15 @@ public UserVm recoverVirtualMachine(RecoverVMCmd cmd) throws ResourceAllocationE
return _vmDao.findById(vmId);
}
+ protected void recoverRootVolume(VolumeVO volume, Long vmId) {
+ if (Volume.State.Destroy.equals(volume.getState())) {
+ _volumeService.recoverVolume(volume.getId());
+ _volsDao.attachVolume(volume.getId(), vmId, ROOT_DEVICE_ID);
+ } else {
+ _volumeService.publishVolumeCreationUsageEvent(volume);
+ }
Review Comment:
doesn´t this mean a volume creation event gets published when the volume is in a state of Expunging/Expunged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1309120368
@GutoVeronezi
plaase see my comments inline.
> @weizhouapache, about the comments:
>
> > @JoaoJandre When vm is destroyed, it is not usable, so vm is not considered in the resource count calculation of instance and cpu/memory. But the ROOT volume is still stored on primary storage. I think it makes sense to consider it in calculation of primary storage resource count. There is a similar scenario: the volumes in Destroy state are also considered in resource count calculation.
>
> and
>
> > If you want users are able to create a new vm after they destroy a vm, please expunge the vm instead of destroy. There are two global settings : allow.user.expunge.recover.vm and allow.user.expunge.recover.volume, when they are set to 0, users can expunge vms and volumes.
>
> Destroying a volume without expunging is a background security measure that businesses adopt in case of user mistake/regret or even ransomware attacks. This kind of situation normally is handled by other means instead of blocking the user (e.g: the plan/billing already covers this kind of situation). Also, enabling users to expunge the volume is counter-productive for those background security measures. Therefore, the options we have in ACS do not match the use case.
>
understood. so you just want to exclude the ROOT volumes of Destroyed VM in the resoure count calculation, right ?
why not just add a global configuration which indicate whether the resource count include or exclude the ROOT volumes of Destroyed VM ?
> Regarding your comment `the volumes in Destroy state are also considered in resource count calculation`, it is not correct; if you put a volume in `Destroy` state, they are not accounted for the domain/account resources.
OK, got it. I guess this may be why you want to set the root volume to Destroy state.
>
> Furthermore, the proposed behavior is controlled by a configuration, meaning that operators will decide to use it or not; the default behavior is maintained. Also, if you have a VM with DATADISKs and destroy the VM selecting the DATADISKs, they will be put in `Destroy` state; however, the ROOT is kept as `Ready`, which is not consistent.
>
DATADISK and ROOT disks are different. When you destroy a vm, DATADISK will be detached from the VM, but ROOT disk will not. The VM can be recovered, no matter what the state of DATADISK is, because DATADISK is not attached.
> > Back to your topic, I think it is a bad idea to mark the root volume as Destroyed, as the root volume will be finally removed by background thread and vm cannot be recovered.
>
> If you look at the code you will notice that the expunge volume thread will not expunge a ROOT volume if the configuration is enabled; the volume expunging will be done by the VM thread, as it is the current behavior with volumes in `Ready` state.
>
I know. But what if the configuration is changed ?
For example, the configuration is true when you destroy a vm, the volume is marked as Destroy. If you change the configuraton to false afterwards, the volume will be removed, right ?
> With that, as the proposed behavior is optional and the use case is concrete, the proposal and the changes make sense.
I prefer to make changes with minimum impact. If you want to change the calculation of resource count, add a global config in `ResourceLimitService`, and use it in `ResourceLimitManagerImpl`.
The change will be simple (<20 lines, not including unit test). If you need my assistance, let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1329435541
@weizhouapache does this solve all our concerns?
@JoaoJandre please add guards for debug statements (that require parameter evaluation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326501535
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4641
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1332427714
> @weizhouapache does this solve all our concerns? @JoaoJandre please add guards for debug statements (that require parameter evaluation)
@DaanHoogland As there are no processes being executed only to log the data (the data is already loaded) and we do not have a convention in using is...Enabled for a simple String construction, such the one I added, I would rather not add it and improve the code readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1322326559
> > Can you please answer the question in my previous comment ?
> > For example, the configuration is true when you destroy a vm, the volume is marked as Destroy. If you change the configuraton to false afterwards, the volume will be removed, right ?
>
> Yes, if you have the configuration enabled with a root volume marked as destroy, and you disable the configuration, the root volume will be subject to the volume cleanup job. This is expected.
@JoaoJandre
in this case, the vm is destroyed, but volume is gone. right ? is it expected ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1325099417
> > > Can you please answer the question in my previous comment ?
> > > For example, the configuration is true when you destroy a vm, the volume is marked as Destroy. If you change the configuraton to false afterwards, the volume will be removed, right ?
> >
> >
> > Yes, if you have the configuration enabled with a root volume marked as destroy, and you disable the configuration, the root volume will be subject to the volume cleanup job. This is expected.
>
> @JoaoJandre in this case, the vm is destroyed, but volume is gone. right ? is it expected ?
Here is all the possible cases:
| Nº | Configuration state | Action | Result | Cleanup |
| ------ | ------ | ------ | ------ | ------ |
| 1 | Disabled | VM is destroyed | Only the VM is set to `Destroyed` state, its root volume remains in `Ready` state | Here the volume cleanup job will not expunge the volume, because the root volume is in `Ready` state, but the VM cleanup job will expunge both the VM and its volume |
| 2 | Enabled | VM is destroyed | Both the VM and its root volume are set to `Destroyed` state | Here the volume cleanup job will not expunge the VM's root volume, because the volume's type is `ROOT` and the configuration is enabled, but the VM cleanup job will expunge both the VM and its volume |
| 3 | Enabled | VM is destroyed, then the configuration is disabled | Both the VM and its root volume are set to Destroyed state | Here the volume cleanup job will expunge the VM's root volume, because the configuration was disabled, the VM cleanup job will expunge the VM (and its volume, if this runs before the volume cleanup job)|
The first case is current behavior, this PR does not change that. This PR introduces cases 2 and 3.
So answering your question:
> @JoaoJandre in this case, the vm is destroyed, but volume is gone. right ? is it expected ?
Yes, this will happen in case 3, that is expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1326736050
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1305849978
> > > > I think you are missing a (not necessarily new) use-case where the root disk is expunged and the VM restored. both with and without this setting this will go wrong. You just made it a bit more explicit with your code.
> > >
> > >
> > > Right. Because vm can be recovered, the root disk should not be removed. If root disk is set to Destroyed, it will be cleaned after a period, then the vm cannot be recovered.
> > > If you want to exclude destroyed vm in resource count, please look at the methods for resource calculation. @JoaoJandre
> >
> >
> > The VM is already being excluded in the resource count, but its root volume is not. This is what this PR proposes to change.
>
> That is not the point @JoaoJandre, and you are right that you are not introducing the fact that the restore will fail. What your PR does is trying to restore the VM with the volume when it has any other state than Destroyed, which includes Expunging and Expunged. These are two cases that should be excluded from that logic, I think. Would you agree?
That is not what this PR intends. The current problem is: when a user destroys a VM, its root volume will remain in the `ready` state, but generally speaking, when you destroy a VM, you intend to destroy its root volume as well.
The proposed solution is to create a global setting ( which will be disabled by default ), that when enabled, ACS will destroy the root volume of a VM when that VM is destroyed. However, because of this setting, a new use case for restoring VMs is created, to restore a VM with a destroyed root volume. Therefore, I created an exception on the volume restore process, so that when you try to restore a VM with a destroyed root volume, it will not fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1308896306
> > > That is not what this PR intends. The current problem is: when a user destroys a VM, its root volume will remain in the ready state, but generally speaking, when you destroy a VM, you intend to destroy its root volume as well.
> >
> >
> > @JoaoJandre that's not true. when destroy a vm, the vm is just marked as Destroyed. none of its resources (nics, storage, vm snapshots, etc) are removed. If you want to completely remove a vm and its resources, please use `expungeVirtualMachine`, or `destroyVirtualMachine` with `expunge=true`
> > > The proposed solution is to create a global setting ( which will be disabled by default ), that when enabled, ACS will destroy the root volume of a VM when that VM is destroyed. However, because of this setting, a new use case for restoring VMs is created, to restore a VM with a destroyed root volume. Therefore, I created an exception on the volume restore process, so that when you try to restore a VM with a destroyed root volume, it will not fail.
>
> Yes, I know that when you destroy a VM it is only marked as "destroyed". When VMs are marked as "destroyed", they no longer count towards the users' used resources. Therefore, after destroying a VM, they can right away create another one with the "freed quota". However, the same logic is not applied to root volumes; they are maintained in "ready" state. What I am addressing here is this inconsistency; the idea is to make the root volume and VMs to have the same behavior.
>
> Let me give you a detailed example. Let's say a user has the following resource limits:  Therefore, the user can only have 1 VM and 1 volume. Then, if the user creates a VM, this is what its resource limit will look like:  Which is exactly as expected. Then, if the user destroys the VM, this is what the resource count will look like:  Therefore, if the user tries to create a new VM (he already destroyed the other one he had), this is what will happen: 
>
> That is the problem the PR is addressing. That is why I proposed this PR, to create a configuration that when enabled will also mark the root volume as destroyed to stop counting it towards users' resource quota.
@JoaoJandre
When vm is destroyed, it is not usable, so vm is not considered in the resource count calculation of instance and cpu/memory.
But the ROOT volume is still stored on primary storage. I think it makes sense to consider it in calculation of primary storage resource count. There is a similar scenario: the volumes in Destroy state are also considered in resource count calculation.
Back to your topic, I think it is a bad idea to mark the root volume as Destroyed, as the root volume will be finally destroyed by background thread and vm cannot be recovered.
If you want users are able to create a new vm after they destroy a vm, please expunge the vm instead of destroy. There are two global settings : allow.user.expunge.recover.vm and allow.user.expunge.recover.volume, when they are set to 0, users can expunge vms and volumes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1310273806
@JoaoJandre @GutoVeronezi
I had a discussion with @DaanHoogland . I am ok with your PR, if you can solve the issue in my last comment, (probably by checking the instance the ROOT volumes attached to, instead of a global setting).
Please consider all potential issues which might be caused by your change, for example, the used capacity of primary storage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on code in PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#discussion_r1014242724
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2335,6 +2337,15 @@ public UserVm recoverVirtualMachine(RecoverVMCmd cmd) throws ResourceAllocationE
return _vmDao.findById(vmId);
}
+ protected void recoverRootVolume(VolumeVO volume, Long vmId) {
+ if (Volume.State.Destroy.equals(volume.getState())) {
+ _volumeService.recoverVolume(volume.getId());
+ _volsDao.attachVolume(volume.getId(), vmId, ROOT_DEVICE_ID);
+ } else {
+ _volumeService.publishVolumeCreationUsageEvent(volume);
+ }
Review Comment:
This is current behavior, I just added the case where the root volume is destroyed. If you look at the current code, no verification is made. An event is always published.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] JoaoJandre commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1303856851
> > I think you are missing a (not necessarily new) use-case where the root disk is expunged and the VM restored. both with and without this setting this will go wrong. You just made it a bit more explicit with your code.
>
> Right. Because vm can be recovered, the root disk should not be removed. If root disk is set to Destroyed, it will be cleaned after a period, then the vm cannot be recovered.
>
> If you want to exclude destroyed vm in resource count, please look at the methods for resource calculation. @JoaoJandre
The VM is already being excluded in the resource count, but its root volume is not. This is what this PR proposes to change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #6868: Set root volume as destroyed when destroying a VM
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6868:
URL: https://github.com/apache/cloudstack/pull/6868#issuecomment-1304235170
> > > I think you are missing a (not necessarily new) use-case where the root disk is expunged and the VM restored. both with and without this setting this will go wrong. You just made it a bit more explicit with your code.
> >
> >
> > Right. Because vm can be recovered, the root disk should not be removed. If root disk is set to Destroyed, it will be cleaned after a period, then the vm cannot be recovered.
> > If you want to exclude destroyed vm in resource count, please look at the methods for resource calculation. @JoaoJandre
>
> The VM is already being excluded in the resource count, but its root volume is not. This is what this PR proposes to change.
That is not the point @JoaoJandre, and you are right that you are not introducing the fact that the restore will fail. What your PR does is trying to restore the VM with the volume when it has any other state than Destroyed, which includes Expunging and Expunged. These are two cases that should be excluded from that logic, I think. Would you agree?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org