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 2020/11/23 08:15:35 UTC

[GitHub] [cloudstack] harikrishna-patnala opened a new pull request #4493: Fix for the issue of recover VM not able to attach the data disks whi…

harikrishna-patnala opened a new pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493


   
   ### Description
   
   This PR  fixes: #4462
   
   Problem Statement: 
   In case of VMware, when a VM having multiple data disk is destroyed (without expunge) and tried to recover the VM then the previous data disks are not attached to the VM like before destroy. Only root disk is attached to the VM.
   
   Root cause:
   All data disks were removed as part of VM destroy. Only the volumes which are selected to delete (while destroying VM) are supposed to be detached and destroyed.
   
   Solution:
   During VM destroy, detach and destroy only volumes which are selected during VM destroy. Detach the other volumes during expunge of VM.
   
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### 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)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [ ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [ ] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   
   
   ### How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-748807312


   Yes - my opinion - keep it simple, maintain the backward compatibility which has existed since early versions.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] PaulAngus commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-747950146


   I think some people are not understanding what the state '_destroyed, but not expunged_' is.
   
   'not expunged' is the same as 'recycle bin'/'trash'/'deleted items'.   It's an intermediate state before 'gone forever' to give the opportunity to undo if you made a mistake.
   
   No user is going to think "I'll put this VM in the recycle bin just in case I need it in the next _2_ days".
   + the user may not have the option expunge/recover as per global settings.
   
   Therefore when a user destroys a VM, then CloudStack should behave like the VM has been destroyed.
   At this point, we have to remember that volumes are **first-class citizens**, they do not _belong_ to a VM.  Therefore data disks must be detached from the destroyed VM and either be expunged (if that is what the user requested) or be left detached and available for any volume actions - attach, expunge, extract, etc.
   
   A specific API parameter to _**try**_ to reattach previously attached data volume has some merit, with a suitable error if the volume(s) aren't available.  Although it's only combining two operations:
   'Recover VM' then 'attach data volume(s)' 
   
   But the premise that data volumes should automatically be reattached is contrary to a core CloudStack principle that data volumes are first-class citizens,
   
   Anything that moves CloudStack further away from that principle is wrong.  If there are already instances where data disks are automatically attached, then they are the bugs.
   
    


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732226548


   > @harikrishna-patnala
   > 
   > I'm not following the logic of what you're trying to achieve here.
   > 
   > If a user deletes a VM which has data disks (without expunging) then the disks get detached - as expected.
   > Disks are seen as first-class citizens; they can be attached to a VM, but they do not **_belong_** to a VM.
   > 
   > Therefore, at best a warning in the UI that the disks will be detached and will have to be re-attached to be used again is the most that could be required.
   > 
   > Restoring a VM is a fix for deleting something that you didn't mean to; so if:
   > 
   > A user has deleted their VM, then attaches the disks to another VM - which (because of above) is very likely (as otherwise, they would have deleted the data disks as well). Then they realise that they need the old VM back for some reason maybe get a config off it, - how are the disk(s) going to be reattached without breaking the second VM?
   > 
   > so right now i'm -1 on the PR and it's premise.
   
   so the question is,
   if we destroy a vm ("Expunge" is not checked), should the volumes checked on UI be detached or not ?
   I prefer not to detach the volumes
   
   ![image](https://user-images.githubusercontent.com/57355700/99977292-bc2ec400-2da4-11eb-9080-d8d4d3d93539.png)
   
   if we expunge a vm, it is clear that the volumes will be detached and removed.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816483908


   @rhtyd a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732091658


   @harikrishna-patnala a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-814605498


   @rhtyd As I mentioned in my last reply, we still need this PR in 4.15.1 to keep the operations same as 4.14 and below.
   The behaviour(what we are discussing now in this thread) of detaching disks from VM upon destroy is completely a new behaviour though it is right one, I would prefer to do it in 4.16 and document the new behaviour as an improvement.
   
   Please review my changes in the PR.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] vladimirpetrov edited a comment on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
vladimirpetrov edited a comment on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-736330769


   I did some testing (without the fix) and here are the results:
   
   Test cases:
   1. VM destroy and recover VM
   - created VM with two data disks
   - destroyed VM (without expunge) along with one data disk
   - recovered VM, VM successfully came up with other data disk which is not selected to delete
   2. VM destroy with expunge
   - created VM with two data disks
   - destroyed VM with expunge
   - VM expunged successfully and two data disks were detached and ready to use for other VMs
   3. VM destroy with expunge and leave a data disk not to delete
   - created VM with two data disks
   - destroy VM with expunge and select only one data disk to delete
   - VM expunged successfully and one data disk is deleted and other data disk was detached and ready to use for other VMs
   
   4.14: vSphere
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   4.14: KVM
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   4.15: vSphere
   TC 1: after VM recovering, the remaining data disk is left unattached
   TC 2: passed OK
   TC 3: passed OK
   
   4.15: KVM
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   Scenarios:
   
   **Scenario A (most likely):**
   1. VM is 'deleted' (but not expunged), Disks are not 'deleted'
   2. User attaches disk(s) to another VM (why would user keep disks if they weren't planning to use it).
   3. VM is restored.
   
   **Scenario B:**
   1. VM is 'deleted' (but not expunged), Disks are also deleted (but not expunged)
   2. Expunge time for disk is less than expunge for VM, so disks get expunged.
   3. VM is restored.
   
   **Scenario C:**
   1. VM is 'deleted' (but not expunged), Disk are not deleted
   2. User deletes and expunges disks
   3. VM is restored.
   
   4.14: vSphere
   Scenario A: the disks are not detached during the VM destroying so they cannot be attached to another VM
   Scenario B: the disks are not detached during the VM destroying so they cannot be deleted
   Scenario C: the disks are not detached during the VM destroying so they must be detached first and then deleted - no issues there
   
   4.15: vSphere
   Scenario A: the disk is detached when destroying the VM and is never reattached - so we can reattach it to another VM
   Scenario B: the disk is detached when destroying the VM and is never reattached - so it's a separate entity now
   Scenario C: the disk is detached when destroying the VM and is never reattached - so no problem when expunging it and then recovering the VM - it is recovered successfully without the data disk.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-781934479


   @harikrishna-patnala given Paul and Daan's comment, is this PR valid? If not, can you make changes in line with Paul/Daan's comment, and is this valid for 4.15 (4.15.1.0 milestone)?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4493: Fix for the issue of recover VM not able to attach the data disks whi…

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732021800


   @borisstoyanov can you test/verify this fixes your issue in #4462?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732118123


   @blueorangutan test centos7 vmware-67u3


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-734270102


   @harikrishna-patnala a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-733847933


   > > @PaulAngus @weizhouapache
   > > We have not changed anything related to existing behaviour of detach or delete of volumes during the destroy and recover VM. We are trying to fix the issue to keep the same behaviour as before.
   > > Disks will still remain attached to VM when VM is destroyed (not expunged), when VM actually gets expunged then the disks will be detached from VM.
   > > I'll quickly explain about the previous behaviour of destroy and recover VM, then what is the regression caused and how is this PR addressing that issue.
   > > **Existing behaviour:**
   > > 
   > > 1. When destroy VM without expunge and when no data disk is selected for deletion
   > >    
   > >    * VM is stopped and data disks will still shown as attached to original VM
   > >    * When tried to recover the VM, the VM is made ready along with its data disks as before.
   > > 2. When destroy VM without expunge and when one or more data disks are selected for deletion
   > >    
   > >    * VM is stopped and data disks which are selected for deletion will be deleted and other data disks will still remain attached
   > >    * When tried to recover the VM, the VM is made ready along with remaining data disks which are not deleted
   > > 3. When destroy VM with expunge and when no data disk is selected for deletion
   > >    
   > >    * All data disks are detached from VM
   > >    * VM will be destroyed and expunged completely
   > > 4. When destroy VM with expunge and few data disks are selected for deletion
   > >    
   > >    * Data disks marked for deletion are deleted and the remaining disks are detached from VM
   > >    * VM will be destroyed and expunged completely.
   > > 
   > > **What is the issue and what caused the regression:**
   > > The issue is when VM is tried to recover, VM comes up only with root disk. Data disks are detached from the VM during destroy (which is not supposed to be as per the expected behaviour)
   > > During the new changes of VMware vSphere from PR 4307, we had to reconcile the disks location into CloudStack based on the actual location in storage. (It can happen that disks can be moved across datastores in a datastore cluster, this has to be reconciled in CloudStack during detach of Volumes from CloudStack).
   > > To address this, a clean detach of volumes is performed from service layers (previously it used to be from VMware resource layer).
   > > This detach has to be done during VM expunge but I did it during VM destroy which caused the issue now.
   > > **Fix:**
   > > so now we are detaching of the volumes logic during VM expunge operation which is original behaviour.
   > 
   > @rhtyd, I've addressed the comments here, this PR fixes an issue which broke the existing behaviour of destroy and recover VM. So this PR needs to be merged.
   
   @harikrishna-patnala thanks for your explanation. looks good to me.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-734602891


   <b>Trillian test result (tid-3258)</b>
   Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 50503 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4493-t3258-vmware-65u2.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 84 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 781.93 | test_kubernetes_clusters.py
   test_04_deploy_and_scale_kubernetes_cluster | `Failure` | 3608.80 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 3609.92 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.10 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.10 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.09 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 86.56 | test_kubernetes_clusters.py
   test_05_rvpc_multi_tiers | `Failure` | 250.75 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Error` | 287.33 | 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-748000166


   > I think some people are not understanding what the state '_destroyed, but not expunged_' is.
   > 
   > 'not expunged' is the same as 'recycle bin'/'trash'/'deleted items'. It's an intermediate state before 'gone forever' to give the opportunity to undo if you made a mistake.
   > 
   > No user is going to think "I'll put this VM in the recycle bin just in case I need it in the next _2_ days".
   > 
   > * the user may not have the option expunge/recover as per global settings.
   > 
   > Therefore when a user destroys a VM, then CloudStack should behave like the VM has been destroyed.
   > At this point, we have to remember that volumes are **first-class citizens**, they do not _belong_ to a VM. Therefore data disks must be detached from the destroyed VM and either be expunged (if that is what the user requested) or be left detached and available for any volume actions - attach, expunge, extract, etc.
   > 
   > A specific API parameter to _**try**_ to reattach previously attached data volume has some merit, with a suitable error if the volume(s) aren't available. Although it's only combining two operations:
   > 'Recover VM' then 'attach data volume(s)'
   > 
   > But the premise that data volumes should automatically be reattached is contrary to a core CloudStack principle that data volumes are first-class citizens,
   > 
   > Anything that moves CloudStack further away from that principle is wrong. If there are already instances where data disks are automatically attached, then they are the bugs.
   
   it looks we need to reach an agreement what's the expected behavior at first.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732651497


   @PaulAngus @weizhouapache 
   
   We have not changed anything related to existing behaviour of detach or delete of volumes during the destroy and recover VM. We are trying to fix the issue to keep the same behaviour as before.
   Disks will still remain attached to VM when VM is destroyed (not expunged), when VM actually gets expunged then the disks will be detached from VM.
   
   I'll quickly explain about the previous behaviour of destroy and recover VM, then what is the regression caused and how is this PR addressing that issue.
   
   **Existing behaviour:**
   1. When destroy VM without expunge and when no data disk is selected for deletion
       - VM is stopped and data disks will still shown as attached to original VM
       - When tried to recover the VM, the VM is made ready along with its data disks as before.
   2. When destroy VM without expunge and when one or more data disks are selected for deletion
       - VM is stopped and data disks which are selected for deletion will be deleted and other data disks will still remain attached
       - When tried to recover the VM, the VM is made ready along with remaining data disks which are not deleted
   3. When destroy VM with expunge and when no data disk is selected for deletion
       - All data disks are detached from VM
       - VM will be destroyed and expunged completely
   4. When destroy VM with expunge and few data disks are selected for deletion
       - Data disks marked for deletion are deleted and the remaining disks are detached from VM
       - VM will be destroyed and expunged completely.
   
   **What is the issue and what caused the regression:**
   The issue is when VM is tried to recover, VM comes up only with root disk. Data disks are detached from the VM during destroy (which is not supposed to be as per the expected behaviour)
   
   During the new changes of VMware vSphere from PR 4307, we had to reconcile the disks location into CloudStack based on the actual location in storage. (It can happen that disks can be moved across datastores in a datastore cluster, this has to be reconciled in CloudStack during detach of Volumes from CloudStack). 
   To address this,  a clean detach of volumes is performed from service layers (previously it used to be from VMware resource layer). 
   
   This detach has to be done during VM expunge but I did it during VM destroy which caused the issue now.
   
   **Fix:**
   so now we are detaching of the volumes logic during VM expunge operation which is original behaviour.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] andrijapanicsb commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-736382282


   1. document and fix in 4.16 (as we are damn late with the 4.15....)
   or
   2. fix now, test, test and ensure no regression and be late with 4.15 even more...
   
   I vote for the first one.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-820182477


   Merging based on reviews and tests by Vladi https://github.com/apache/cloudstack/pull/4493#issuecomment-736330769 where the case is only inconsistent for VMware. This PR brings consistency across hypervisors and retains backward compatibility but longer term we need the improvement https://github.com/apache/cloudstack/issues/4902


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] PaulAngus commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-781374978


   Thanks @rhtyd 
   
   I added my comments on 18 Dec 2020
   https://github.com/apache/cloudstack/pull/4493#issuecomment-747950146
   
   I've re-read then and still believe them to be correct.
   Data volumes are first-class citizens, they do not belong to a VM, they are merely attached to it for some period of time.  Therefore if the VM to which they were attached is destroyed, they become free agents.
   
   As stated above, a compromise could be:
   
   > A specific API parameter to try to reattach previously attached data volume has some merit, with a suitable error if the volume(s) aren't available. Although it's only combining two operations:
   > 'Recover VM' then 'attach data volume(s)'
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732841089


   Please re-investigate and address comments @harikrishna-patnala let me know how we can help, thnx.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Fix for the issue of recover VM not able to attach the data disks whi…

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732013785


   @harikrishna-patnala can you kick pkging and tests yourself? 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] vladimirpetrov commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
vladimirpetrov commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-736330769


   I did some testing and here are the results:
   
   Test cases:
   1. VM destroy and recover VM
   - created VM with two data disks
   - destroyed VM (without expunge) along with one data disk
   - recovered VM, VM successfully came up with other data disk which is not selected to delete
   2. VM destroy with expunge
   - created VM with two data disks
   - destroyed VM with expunge
   - VM expunged successfully and two data disks were detached and ready to use for other VMs
   3. VM destroy with expunge and leave a data disk not to delete
   - created VM with two data disks
   - destroy VM with expunge and select only one data disk to delete
   - VM expunged successfully and one data disk is deleted and other data disk was detached and ready to use for other VMs
   
   4.14: vSphere
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   4.14: KVM
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   4.15: vSphere
   TC 1: after VM recovering, the remaining data disk is left unattached
   TC 2: passed OK
   TC 3: passed OK
   
   4.15: KVM
   TC 1: passed OK
   TC 2: passed OK
   TC 3: passed OK
   
   Scenarios:
   
   **Scenario A (most likely):**
   1. VM is 'deleted' (but not expunged), Disks are not 'deleted'
   2. User attaches disk(s) to another VM (why would user keep disks if they weren't planning to use it).
   3. VM is restored.
   
   **Scenario B:**
   1. VM is 'deleted' (but not expunged), Disks are also deleted (but not expunged)
   2. Expunge time for disk is less than expunge for VM, so disks get expunged.
   3. VM is restored.
   
   **Scenario C:**
   1. VM is 'deleted' (but not expunged), Disk are not deleted
   2. User deletes and expunges disks
   3. VM is restored.
   
   4.14: vSphere
   Scenario A: the disks are not detached during the VM destroying so they cannot be attached to another VM
   Scenario B: the disks are not detached during the VM destroying so they cannot be deleted
   Scenario C: the disks are not detached during the VM destroying so they must be detached first and then deleted - no issues there
   
   4.15: vSphere
   Scenario A: the disk is detached when destroying the VM and is never reattached - so we can reattach it to another VM
   Scenario B: the disk is detached when destroying the VM and is never reattached - so it's a separate entity now
   Scenario C: the disk is detached when destroying the VM and is never reattached - so no problem when expunging it and then recovering the VM - it is recovered successfully without the data disk.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816483590


   Okay @harikrishna-patnala please raise a new PR against 4.16/master. @DaanHoogland @andrijapanicsb @wido @weizhouapache @vladimirpetrov @borisstoyanov and others - do you agree with the approach?
   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] PaulAngus commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732203808


   @harikrishna-patnala 
   
   I'm not following the logic of what you're trying to achieve here.
   
   If a user deletes a VM which has data disks (without expunging) then the disks get detached - as expected.
   Disks are seen as first-class citizens; they can be attached to a VM, but they do not **_belong_** to a VM.
   
   Therefore, at best a warning in the UI that the disks will be detached and will have to be re-attached to be used again is the most that could be required.
   
   Restoring a VM is a fix for deleting something that you didn't mean to; so if:
   
   A user has deleted their VM, then attaches the disks to another VM - which (because of above) is very likely (as otherwise, they would have deleted the data disks as well). Then they realise that they need the old VM back for some reason maybe get a config off it, - how are the disk(s) going to be reattached without breaking the second VM?
   
   so right now i'm -1 on the PR and it's premise.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-734269776


   @blueorangutan test matrix


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-733462347


   > @PaulAngus @weizhouapache
   > 
   > We have not changed anything related to existing behaviour of detach or delete of volumes during the destroy and recover VM. We are trying to fix the issue to keep the same behaviour as before.
   > Disks will still remain attached to VM when VM is destroyed (not expunged), when VM actually gets expunged then the disks will be detached from VM.
   > 
   > I'll quickly explain about the previous behaviour of destroy and recover VM, then what is the regression caused and how is this PR addressing that issue.
   > 
   > **Existing behaviour:**
   > 
   > 1. When destroy VM without expunge and when no data disk is selected for deletion
   >    
   >    * VM is stopped and data disks will still shown as attached to original VM
   >    * When tried to recover the VM, the VM is made ready along with its data disks as before.
   > 2. When destroy VM without expunge and when one or more data disks are selected for deletion
   >    
   >    * VM is stopped and data disks which are selected for deletion will be deleted and other data disks will still remain attached
   >    * When tried to recover the VM, the VM is made ready along with remaining data disks which are not deleted
   > 3. When destroy VM with expunge and when no data disk is selected for deletion
   >    
   >    * All data disks are detached from VM
   >    * VM will be destroyed and expunged completely
   > 4. When destroy VM with expunge and few data disks are selected for deletion
   >    
   >    * Data disks marked for deletion are deleted and the remaining disks are detached from VM
   >    * VM will be destroyed and expunged completely.
   > 
   > **What is the issue and what caused the regression:**
   > The issue is when VM is tried to recover, VM comes up only with root disk. Data disks are detached from the VM during destroy (which is not supposed to be as per the expected behaviour)
   > 
   > During the new changes of VMware vSphere from PR 4307, we had to reconcile the disks location into CloudStack based on the actual location in storage. (It can happen that disks can be moved across datastores in a datastore cluster, this has to be reconciled in CloudStack during detach of Volumes from CloudStack).
   > To address this, a clean detach of volumes is performed from service layers (previously it used to be from VMware resource layer).
   > 
   > This detach has to be done during VM expunge but I did it during VM destroy which caused the issue now.
   > 
   > **Fix:**
   > so now we are detaching of the volumes logic during VM expunge operation which is original behaviour.
   
   @rhtyd, I've addressed the comments here, this PR fixes an issue which broke the existing behaviour of destroy and recover VM. So this PR needs to be merged. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-736339361


   @PaulAngus, @borisstoyanov  (cc @weizhouapache @rhtyd @harikrishna-patnala )
   By @vladimirpetrov 's results I think we should release without this patch and with a note in the release notes about the changed/fixed behaviour. Thoughts?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-734485898


   <b>Trillian test result (tid-3256)</b>
   Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 31053 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4493-t3256-xenserver-71.zip
   Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_scale_vm | `Failure` | 9.45 | test_scale_vm.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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-734494558


   <b>Trillian test result (tid-3257)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33085 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4493-t3257-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 86 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#discussion_r529223093



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2946,13 +2946,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
 
         stopVirtualMachine(vmId, VmDestroyForcestop.value());
 

Review comment:
       Yes @sureshanaparti 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd merged pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732118797


   @harikrishna-patnala a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816520958


   I think trying to attach disk during discovery is a bug. It can give serious errors if the disks were attached to another VM before recovery. We should document that this behaviour had unexpected side effects and at best make an optional attempt to find the disks and attach them if explicitly requested by the user.
   
   The code I see here is only doing the detaching (unconditionally and independent of hypervisor), which 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816583851


   I've created an improvement ticket for 4.16 here https://github.com/apache/cloudstack/issues/4902


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732476624


   <b>Trillian test result (tid-3236)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37907 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4493-t3236-vmware-67u3.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Smoke tests completed. 85 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 782.93 | test_kubernetes_clusters.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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816518380


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 369


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-783095522


   If we agree upon treating the volumes as first class citizens during the VM destroy and making them independent of VM after it is destroyed then we need to fix this for other hypervisor types also.
   
   In this PR we are trying to bring back the old behaviour of keeping the disks attached to the destroyed VM.
   
   In my opinion, this PR is still required because without this PR the behaviour(what we need now) of detaching disks from VM upon destroy is applicable only for VMware. So I would say, lets revert back to old behaviour and fix the volume detachment process as we need for all hypervisor types, test it completely, document the new behaviour as a new improvement ticket.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] vladimirpetrov commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
vladimirpetrov commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-736361329


   I think we should include @andrijapanicsb in the discussion since he commented on the original issue.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732090873


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-816635690


   Thanks @harikrishna-patnala @DaanHoogland @sureshanaparti for the comments and raising that as an issue - let's address the corrective behaviour as a separate task. Do we need a round of testing, cc @vladimirpetrov @borisstoyanov ? I see Vladi's testing above, not sure if that is enough.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-813309221


   @harikrishna-patnala can you review comments from various reviewers and address the changes based on formed consensus


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-781404995


   I'm with @PaulAngus on this


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-781319551


   ping thoughts on what should be the expected behaviour - let's discuss and build concensus? @DaanHoogland @weizhouapache @harikrishna-patnala @sureshanaparti @borisstoyanov @PaulAngus 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#discussion_r529210986



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2946,13 +2946,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
 
         stopVirtualMachine(vmId, VmDestroyForcestop.value());
 

Review comment:
       @harikrishna-patnala Checked the changes done in the PR https://github.com/apache/cloudstack/pull/4307, which is breaking the old behavior (On destroy VM, detach the data disks which are marked to be deleted, and keep other disks attached). and these changes are reverted here to bring back the old behavior. Am I correct?
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] wido commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
wido commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-781977116


   I agree. Only the root disk is a part of the VM. Data disks are not.
   
   I think we even had a PR around which allowed to attach a data disk to multiple VMs at once. Some applications require this like Oracle DB, MS Exchange, etc.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-747878238


   @DaanHoogland this is marked critical, are we considering this in 4.15?
   @weizhouapache @wido @kiwiflyer @GabrielBrascher what do you think about the behaviour and the fix, should we keep old behaviour?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-732112487


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2425


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4493: Recover VM not able to attach the data disks which were attached before destroy

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4493:
URL: https://github.com/apache/cloudstack/pull/4493#issuecomment-735703662


   Related global settings that would allow users to see and recover a deleted but not expunged VM:
   ```
   allow.user.expunge.recover.vm | Determines whether users can expunge or recover their vm | Advanced | true |  
   allow.user.expunge.recover.volume | Determines whether users can expunge or recover their volume | Advanced | true
   ```


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org