You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cloudstack.apache.org by "Mike Tutkowski (JIRA)" <ji...@apache.org> on 2014/01/07 20:49:53 UTC
[jira] [Resolved] (CLOUDSTACK-5823) Taking a VMware snapshot
doesn't work
[ https://issues.apache.org/jira/browse/CLOUDSTACK-5823?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Mike Tutkowski resolved CLOUDSTACK-5823.
----------------------------------------
Resolution: Fixed
I checked in ecd4a9c6424be73b24a44c73f04c67c47e1611e8.
Creating, deleting, and reverting VMware snapshots appears to work now.
> Taking a VMware snapshot doesn't work
> -------------------------------------
>
> Key: CLOUDSTACK-5823
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-5823
> Project: CloudStack
> Issue Type: Bug
> Security Level: Public(Anyone can view this level - this is the default.)
> Components: VMware
> Affects Versions: 4.3.0
> Environment: ESXi 5.1
> Reporter: Mike Tutkowski
> Assignee: Mike Tutkowski
> Fix For: 4.3.0
>
>
> Per a discussion on the mailing list:
> VMware snapshot question
> Mike Tutkowski <mi...@solidfire.com>
> 4:10 PM (18 hours ago)
> to dev
> 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 <mi...@solidfire.com>
> 4:35 PM (17 hours ago)
> to dev
> 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?
> Mike Tutkowski <mi...@solidfire.com>
> 6:33 PM (15 hours ago)
> to Kelven, dev
> 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).
> 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!
> Kelven Yang 10:02 PM (12 hours ago)
> Yes, your guess is correct, the intent to update with a new path is to reflec...
> Mike Tutkowski <mi...@solidfire.com>
> 10:13 PM (12 hours ago)
> to dev, Kelven
> 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!
> --
> Mike Tutkowski
> Senior CloudStack Developer, SolidFire Inc.
> e: mike.tutkowski@solidfire.com
> o: 303.746.7302
> Advancing the way the world uses the cloud™
> Mike Tutkowski <mi...@solidfire.com>
> 10:43 PM (11 hours ago)
> to dev, Kelven
> 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!!
> Mike Tutkowski <mi...@solidfire.com>
> 10:47 PM (11 hours ago)
> to dev, Kelven
> 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?
> Mike Tutkowski <mi...@solidfire.com>
> 12:19 AM (10 hours ago)
> to dev, Kelven
> 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!
> Mike Tutkowski <mi...@solidfire.com>
> 12:20 AM (10 hours ago)
> to dev, Kelven
> 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);
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)