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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
