I logged a bug for this:
1. CLOUDSTACK-5823<https://issues.apache.org/jira/browse/CLOUDSTACK-5823> I assigned this to myself. I believe I've corrected the issue with taking a VMware snapshot, but I want to look into the delete and revert code, as well. On Tue, Jan 7, 2014 at 12:20 AM, Mike Tutkowski < mike.tutkow...@solidfire.com> wrote: > If it's true that only the last iteration "counts," couldn't we just grab > the last item in this list?: > > List<Pair<String, ManagedObjectReference>> vmdkFiles = vmMo. > getDiskDatastorePathChain(vdisks[i], false); > > > On Tue, Jan 7, 2014 at 12:19 AM, Mike Tutkowski < > mike.tutkow...@solidfire.com> wrote: > >> Hi Kelven, >> >> I've been playing around with some code to fix this VMware-snapshot >> issue. Probably won't have it done until tomorrow, but I wanted to ask you >> about this code: >> >> for (int i = 0; i < vdisks.length; i++) { >> >> List<Pair<String, ManagedObjectReference>> vmdkFiles >> = vmMo.getDiskDatastorePathChain(vdisks[i], false); >> >> >> for (Pair<String, ManagedObjectReference> fileItem : >> vmdkFiles) { >> >> >> Can you tell me why we iterate through all of the VMDK files of a virtual >> disk? It seems like only the last one "counts." Is that correct? Am I to >> assume the last one we iterate over is the most recent snapshot (the >> snapshot we just took)? >> >> Thanks! >> >> >> On Mon, Jan 6, 2014 at 10:47 PM, Mike Tutkowski < >> mike.tutkow...@solidfire.com> wrote: >> >>> Ignore my question about coming up with a baseName. >>> >>> I see now that volumeTO is not available in the first for loop. >>> >>> I do think the key and value we have in the map, though, is incorrect. >>> >>> What do you think? >>> >>> >>> On Mon, Jan 6, 2014 at 10:43 PM, Mike Tutkowski < >>> mike.tutkow...@solidfire.com> wrote: >>> >>>> Hi Kelven, >>>> >>>> To give you an idea visually what I am referring to, please check out >>>> this screen capture: >>>> >>>> http://i.imgur.com/ma3FE9o.png >>>> >>>> The key is "i-2" (part of the folder for the VMDK file). >>>> >>>> The value contains the folder the VMDK file is in. Since the path >>>> column for VMware volumes in the DB doesn't contain the folder the VMDK >>>> file is in, I think this may be incorrect, as well. >>>> >>>> I also noticed that we later try to retrieve from the map using >>>> volumeTO.getPath() (ignore the getPath() method that enclosing >>>> volumeTO.getPath() in the screen shot as this is related to new code...in >>>> the standard case, the value of volumeTO.getPath() is just returned from >>>> the getPath() method). >>>> >>>> In the first line of code visible in the screen capture, why do we go >>>> to the trouble of doing this: >>>> >>>> String baseName = extractSnapshotBaseFileName(vmdkName); >>>> >>>> It seems like this would have worked: >>>> >>>> String baseName = extractSnapshotBaseFileName(volumTO.getPath()); >>>> >>>> Or am I missing something there? >>>> >>>> Thanks!! >>>> >>>> >>>> On Mon, Jan 6, 2014 at 10:13 PM, Mike Tutkowski < >>>> mike.tutkow...@solidfire.com> wrote: >>>> >>>>> Thanks for the info, Kelven. >>>>> >>>>> I believe we have a serious bug then as null is being assigned to >>>>> newPath when a VMware snapshot is being taken (this is in 4.3, by the >>>>> way). >>>>> >>>>> I was trying to fix an issue with VMware snapshots and managed storage >>>>> and happened upon this. >>>>> >>>>> If you have a moment, you might want to set a breakpoint and step >>>>> through and see what I mean first hand. >>>>> >>>>> I'm looking into it, as well. >>>>> >>>>> Thanks! >>>>> >>>>> >>>>> On Mon, Jan 6, 2014 at 10:02 PM, Kelven Yang >>>>> <kelven.y...@citrix.com>wrote: >>>>> >>>>>> >>>>>> >>>>>> On 1/6/14, 5:33 PM, "Mike Tutkowski" <mike.tutkow...@solidfire.com> >>>>>> wrote: >>>>>> >>>>>> >Actually, the more I look at this code, the more I think perhaps >>>>>> VMware >>>>>> >snapshots are broken because the newPath field should probably not be >>>>>> >assigned null after creating a new VMware snapshot >>>>>> >>>>>> >>>>>> >(I'm thinking the intend >>>>>> >is to replace the other path with a new path that refers to the >>>>>> delta file >>>>>> >that was just created). >>>>>> >>>>>> Yes, your guess is correct, the intent to update with a new path is to >>>>>> reflect the name change after a VM snapshot is taken. When VM >>>>>> snapshot is >>>>>> involved, one CloudStack volume may have more than one disks be >>>>>> related >>>>>> with it. So the information in path field only points to the top of >>>>>> the >>>>>> disk chain, and it is not guaranteed that this information is in sync >>>>>> with >>>>>> vCenter since there may exist out-of-band changes from vCenter (by >>>>>> taking >>>>>> VM snapshot in vCenter). >>>>>> >>>>>> To gracefully work with vCenter, for existing disks that are attached >>>>>> to a >>>>>> VM, CloudStack only uses the information stored in path field in >>>>>> volume >>>>>> table as a search basis to connect the record with the real disk >>>>>> chain in >>>>>> the storage, as soon as it has located the connection, it actually >>>>>> uses >>>>>> the most recent information from vCenter datastore to setup the disk >>>>>> device. In addition, CloudStack also updates the full disk-chain >>>>>> information to a field called ³chainInfo². The full chain-info can be >>>>>> used >>>>>> for recovery/copy-out purpose >>>>>> >>>>>> Kelven >>>>>> >>>>>> >>>>>> > >>>>>> >Does anyone know who worked on VMware snapshots? I've love to ask >>>>>> these >>>>>> >questions to him soon as we are approaching the end of 4.3. >>>>>> > >>>>>> >Thanks! >>>>>> > >>>>>> > >>>>>> >On Mon, Jan 6, 2014 at 4:35 PM, Mike Tutkowski >>>>>> ><mike.tutkow...@solidfire.com >>>>>> >> wrote: >>>>>> > >>>>>> >> In short, I believe we can remove mapNewDisk and just assign null >>>>>> to >>>>>> >> newPath. This will keep the existing path for the volume in >>>>>> question (in >>>>>> >> the volumes table) in the same state as it was before we created a >>>>>> >>VMware >>>>>> >> snapshot, which I believe is the intent anyways. >>>>>> >> >>>>>> >> Thoughts on that? >>>>>> >> >>>>>> >> >>>>>> >> On Mon, Jan 6, 2014 at 4:10 PM, Mike Tutkowski < >>>>>> >> mike.tutkow...@solidfire.com> wrote: >>>>>> >> >>>>>> >>> Hi, >>>>>> >>> >>>>>> >>> I was wondering about the following code in >>>>>> VmwareStorageManagerImpl. >>>>>> >>>It >>>>>> >>> is in the CreateVMSnapshotAnswer execute(VmwareHostService >>>>>> hostService, >>>>>> >>> CreateVMSnapshotCommand cmd) method. >>>>>> >>> >>>>>> >>> The part I wonder about is in populating the mapNewDisk map. For >>>>>> disks >>>>>> >>> like the following: >>>>>> >>> >>>>>> >>> i-2-9-VM/fksjfaklsjdgflajs.vmdk, the key for the map ends up >>>>>> being i-2. >>>>>> >>> >>>>>> >>> When we call this: >>>>>> >>> >>>>>> >>> String baseName = extractSnapshotBaseFileName(volumeTO.getPath()); >>>>>> >>> >>>>>> >>> It uses a path such as the following: >>>>>> >>> >>>>>> >>> fksjfaklsjdgflajs >>>>>> >>> >>>>>> >>> There is no i-2-9-VM/ preceding the name, so the key we search on >>>>>> ends >>>>>> >>>up >>>>>> >>> being the following: >>>>>> >>> >>>>>> >>> fksjfaklsjdgflajs >>>>>> >>> >>>>>> >>> This leads to a newPath being equal to null. >>>>>> >>> >>>>>> >>> As it turns out, I believe null is actually correct, but - if >>>>>> that's >>>>>> >>>the >>>>>> >>> case - why do we have all this logic if - in the end - we are just >>>>>> >>>going to >>>>>> >>> assign null to newPath in every case when creating a VM snapshot >>>>>> for >>>>>> >>> VMware? As it turns out, null is later interpreted to mean, "don't >>>>>> >>>replace >>>>>> >>> the path field of this volume in the volumes table," which is, I >>>>>> think, >>>>>> >>> what we want. >>>>>> >>> >>>>>> >>> Thanks! >>>>>> >>> >>>>>> >>> VirtualDisk[] vdisks = vmMo.getAllDiskDevice(); >>>>>> >>> >>>>>> >>> for (int i = 0; i < vdisks.length; i ++){ >>>>>> >>> >>>>>> >>> List<Pair<String, ManagedObjectReference>> >>>>>> >>>vmdkFiles >>>>>> >>> = vmMo.getDiskDatastorePathChain(vdisks[i], false); >>>>>> >>> >>>>>> >>> for(Pair<String, ManagedObjectReference> >>>>>> fileItem : >>>>>> >>> vmdkFiles) { >>>>>> >>> >>>>>> >>> String vmdkName = fileItem.first().split(" >>>>>> >>>")[1]; >>>>>> >>> >>>>>> >>> if (vmdkName.endsWith(".vmdk")){ >>>>>> >>> >>>>>> >>> vmdkName = vmdkName.substring(0, >>>>>> >>> vmdkName.length() - (".vmdk").length()); >>>>>> >>> >>>>>> >>> } >>>>>> >>> >>>>>> >>> String baseName = >>>>>> >>> extractSnapshotBaseFileName(vmdkName); >>>>>> >>> >>>>>> >>> mapNewDisk.put(baseName, vmdkName); >>>>>> >>> >>>>>> >>> } >>>>>> >>> >>>>>> >>> } >>>>>> >>> >>>>>> >>> for (VolumeObjectTO volumeTO : volumeTOs) { >>>>>> >>> >>>>>> >>> String baseName = >>>>>> >>> extractSnapshotBaseFileName(volumeTO.getPath()); >>>>>> >>> >>>>>> >>> String newPath = mapNewDisk.get(baseName); >>>>>> >>> >>>>>> >>> // get volume's chain size for this VM >>>>>> snapshot, >>>>>> >>> exclude current volume vdisk >>>>>> >>> >>>>>> >>> DataStoreTO store = volumeTO.getDataStore(); >>>>>> >>> >>>>>> >>> long size = >>>>>> >>> getVMSnapshotChainSize(context,hyperHost,baseName + "*.vmdk", >>>>>> >>> >>>>>> >>> store.getUuid(), newPath); >>>>>> >>> >>>>>> >>> >>>>>> >>> if(volumeTO.getVolumeType()== >>>>>> Volume.Type.ROOT){ >>>>>> >>> >>>>>> >>> // add memory snapshot size >>>>>> >>> >>>>>> >>> size = size + >>>>>> >>> getVMSnapshotChainSize(context,hyperHost,cmd.getVmName()+"*.vmsn" >>>>>> >>> ,store.getUuid(),null); >>>>>> >>> >>>>>> >>> } >>>>>> >>> >>>>>> >>> >>>>>> >>> volumeTO.setSize(size); >>>>>> >>> >>>>>> >>> volumeTO.setPath(newPath); >>>>>> >>> >>>>>> >>> } >>>>>> >>> >>>>>> >>> -- >>>>>> >>> *Mike Tutkowski* >>>>>> >>> *Senior CloudStack Developer, SolidFire Inc.* >>>>>> >>> e: mike.tutkow...@solidfire.com >>>>>> >>> o: 303.746.7302 >>>>>> >>> Advancing the way the world uses the >>>>>> >>>cloud<http://solidfire.com/solution/overview/?video=play> >>>>>> >>> * * >>>>>> >>> >>>>>> >> >>>>>> >> >>>>>> >> >>>>>> >> -- >>>>>> >> *Mike Tutkowski* >>>>>> >> *Senior CloudStack Developer, SolidFire Inc.* >>>>>> >> e: mike.tutkow...@solidfire.com >>>>>> >> o: 303.746.7302 >>>>>> >> Advancing the way the world uses the >>>>>> >>cloud<http://solidfire.com/solution/overview/?video=play> >>>>>> >> * * >>>>>> >> >>>>>> > >>>>>> > >>>>>> > >>>>>> >-- >>>>>> >*Mike Tutkowski* >>>>>> >*Senior CloudStack Developer, SolidFire Inc.* >>>>>> >e: mike.tutkow...@solidfire.com >>>>>> >o: 303.746.7302 >>>>>> >Advancing the way the world uses the >>>>>> >cloud<http://solidfire.com/solution/overview/?video=play> >>>>>> >* * >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> *Mike Tutkowski* >>>>> *Senior CloudStack Developer, SolidFire Inc.* >>>>> e: mike.tutkow...@solidfire.com >>>>> o: 303.746.7302 >>>>> Advancing the way the world uses the >>>>> cloud<http://solidfire.com/solution/overview/?video=play> >>>>> *™* >>>>> >>>> >>>> >>>> >>>> -- >>>> *Mike Tutkowski* >>>> *Senior CloudStack Developer, SolidFire Inc.* >>>> e: mike.tutkow...@solidfire.com >>>> o: 303.746.7302 >>>> Advancing the way the world uses the >>>> cloud<http://solidfire.com/solution/overview/?video=play> >>>> *™* >>>> >>> >>> >>> >>> -- >>> *Mike Tutkowski* >>> *Senior CloudStack Developer, SolidFire Inc.* >>> e: mike.tutkow...@solidfire.com >>> o: 303.746.7302 >>> Advancing the way the world uses the >>> cloud<http://solidfire.com/solution/overview/?video=play> >>> *™* >>> >> >> >> >> -- >> *Mike Tutkowski* >> *Senior CloudStack Developer, SolidFire Inc.* >> e: mike.tutkow...@solidfire.com >> o: 303.746.7302 >> Advancing the way the world uses the >> cloud<http://solidfire.com/solution/overview/?video=play> >> *™* >> > > > > -- > *Mike Tutkowski* > *Senior CloudStack Developer, SolidFire Inc.* > e: mike.tutkow...@solidfire.com > o: 303.746.7302 > Advancing the way the world uses the > cloud<http://solidfire.com/solution/overview/?video=play> > *™* > -- *Mike Tutkowski* *Senior CloudStack Developer, SolidFire Inc.* e: mike.tutkow...@solidfire.com o: 303.746.7302 Advancing the way the world uses the cloud<http://solidfire.com/solution/overview/?video=play> *™*
