Taking fast and efficient volume snapshots with XenServer (and your storage provider) by mike-tutkowski · Pull Request #1403 · apache/cloudstack

mike-tutkowski · 2016-02-06T17:21:08Z

A XenServer storage repository (SR) and virtual disk image (VDI) each have UUIDs that are immutable.

This poses a problem for SAN snapshots, if you intend on mounting the underlying snapshot SR alongside the source SR (duplicate UUIDs).

VMware has a solution for this called re-signaturing (so, in other words, the snapshot UUIDs can be changed).

This PR only deals with the CloudStack side of things, but it works in concert with a new XenServer storage manager created by CloudOps (this storage manager enables re-signaturing of XenServer SR and VDI UUIDs).

I have written Marvin integration tests to go along with this, but cannot yet check those into the CloudStack repo as they rely on SolidFire hardware.

If anyone would like to see these integration tests, please let me know.

JIRA ticket: https://issues.apache.org/jira/browse/CLOUDSTACK-9281

Here's a video I made that shows this feature in action:

https://www.youtube.com/watch?v=YQ3pBeL-WaA&list=PLqOXKM0Bt13DFnQnwUx8ZtJzoyDV0Uuye&index=13

mike-tutkowski · 2016-02-06T20:11:20Z

Here's a copy of my Marvin integration tests (I added a .txt file type so that I could upload the file as it's not permitted to upload a file of type .py):

TestSnapshots.py.txt

Here are the most recent test results:

results.txt

GabrielBrascher · 2016-02-06T22:41:29Z

-                if (errMsg == null) {
-                    snapshotInfo.processEvent(Event.OperationSuccessed);
+                if (copyCmdAnswer == null || !copyCmdAnswer.getResult()) {
+                    if (copyCmdAnswer != null && copyCmdAnswer.getDetails() != null && !copyCmdAnswer.getDetails().isEmpty()) {


@mike-tutkowski Could you please use StringUtils.isEmpty (https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/StringUtils.html#isEmpty(java.lang.String))?

This is a null safe test if the String copyCmdAnswer.getDetails is empty (you can use this method at lines 260, 363 and 403 -- in the StorageSystemDataMotionStrategy class).

Thanks.

Done - thanks

mike-tutkowski · 2016-02-07T08:46:01Z

Thanks for the comments, Gabriel. I have pushed a new commit with those updates.

rodrigo93 · 2016-02-14T17:01:35Z

+        return Boolean.parseBoolean(property);
+    }
+
    private Void handleCreateTemplateFromSnapshot(SnapshotInfo snapshotInfo, TemplateInfo templateInfo, AsyncCompletionCallback<CopyCommandResult> callback) {


Hi @mike-tutkowski ,
Consider changing "Void" to "void" and removing the "return null;" at the end of this method.
Another suggestion is to create a single method, if possible, for the lines 260 to 266 and 363 to 371, this would avoid some duplication of code.

Thanks. Initially this method was patterned off of an existing set of methods that used the "Void" approach. I wasn't sure if there was a good reason for why they did it that way and erred on the side of caution and just repeated the pattern.

As it turned out, someone later changed those methods from "Void" to "void" and so I ended up doing so in my sandbox (but haven't yet pushed the changes).

I see... Using "Void" instead of "void" is not a good programming practice and should be "avoided" =P.
Another suggestion that I forgot to mention is to squash your commits into one.

I believe our current best practice is to not squash commits (i.e. to preserve the history).

@mike-tutkowski if commits are atomic and not breaking ci, yes, else, no. Please use your own judgement.

syed · 2016-02-15T15:17:04Z

LGTM. I've also been testing this in my dev environment with no issues so far

asfbot · 2016-02-16T19:50:13Z

Mike Tutkowski on dev@cloudstack.apache.org replies:
None of those individual commits should break CI.
eSystemDataMotionStrategy.java
{
ur
e
se

bvbharatk · 2016-03-13T23:06:50Z

ACS CI BVT Run

Sumarry:
Build Number 91
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=14
Skipped=4

The follwing tests have known issues
integration.smoke.test_iso.TestISO.test_07_list_default_iso
integration.smoke.test_privategw_acl.py
integration.smoke.test_vm_snapshots.TestSnapshots.test_01_test_vm_volume_snapshot
integration.smoke.test_vpc_vpn.TestVpcSite2SiteVpn.test_vpc_site2site_vpn
integration.smoke.test_iso.TestISO.test_04_extract_Iso

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

integration.smoke.test_vpc_vpn.TestVpcRemoteAccessVpn
- test_vpc_remote_access_vpn
- test_vpc_site2site_vpn
integration.smoke.test_vm_snapshots.TestSnapshots
- test_01_test_vm_volume_snapshot
integration.smoke.test_privategw_acl.TestPrivateGwACL
- test_02_vpc_privategw_static_routes
- test_03_rvpc_privategw_static_routes
integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
- test_dedicateGuestVlanRange
<nose.suite
- ContextSuite context=TestNiciraContoller>:setup
integration.smoke.test_primary_storage.TestPrimaryStorageServices
- test_01_primary_storage_iscsi
<nose.suite
- ContextSuite context=TestCreateVolume>:setup
integration.smoke.test_internal_lb.TestInternalLb
- test02_internallb_haproxy_stats_on_all_interfaces
- test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80
integration.smoke.test_templates.TestCreateTemplate
- test_04_extract_template
integration.smoke.test_iso.TestCreateIso
- test_04_extract_Iso
- test_07_list_default_iso

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

Passed test suits:
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering

mike-tutkowski · 2016-04-08T17:51:35Z

Here are my updated Marvin tests for this feature (with a "txt" extension because GitHub doesn't allow you to upload files with a "py" extension):

TestSnapshots.py.txt
TestVMSnapshots.py.txt
TestVolumes.py.txt

swill · 2016-04-08T17:53:15Z

Can you rebase and give me a squashed commit for this?

Also, regarding the code. I have seen a lot of people cleaning up code to remove the _ in front of the variables. You seem to have done the opposite in this PR and have actually added an _ to variables in some cases. I am not sure how we are handling this, but I suggest we start working toward a single standard as it is confusing for me right now...

I will add this to my queue to run a full CI against it again.

Also, I am looking for two distinct LGTM for every PR, so I would like another LGTM code review. Can someone from the dev list please give me a review? Thx...

mike-tutkowski · 2016-04-08T17:57:12Z

Hi @swill - No problem for me to squash the commits, but are you sure you want to lose the history of the individual commits?

Also, from what I've seen over the years, it seems more common to prefix instance member variables with "_" than to not do so, but I'm not sure if we have a coding guideline around that or not.

Can you clarify, too, if a LGTM has to come from a committer or can it be any dev on the list?

If any dev on the list, we have Syed and I believe Gabriel (although I didn't see a literal LGTM from him, but rather he provided his comments on the code).

Thanks!

swill · 2016-04-08T18:12:49Z

I have not seen a style guide, but I have not looked. I have just seen a bunch of PRs recently and there seems to be a consistent work to remove the _s in the code, so I figured I would mention it. To me, this is not a reason not to merge the code, but I agree it would be good to know what the style should be so I can start enforcing it so we can start working towards consistent styling.

I am not requiring the LGTM code reviews to come from committers. If you are active on on Github and are adding value to the project by reviewing code, I am willing to work with that. Because I am not being strict about who is reviewing the code, I am also enforcing at least 2 people review the code so I get more eyes on ever PR.

As for the squash. It may make sense to have some history, but there are a lot of commits in there that are entirely useless outside the greater context of this PR. To me, the final version of the code merged trumps all. If it is squashed then we can more easily evaluate the changes associated with this commit later. If you think there are some points in the history of this feature that make sense to be called out as important, you can squash into 3 or 4 commits, but 43 commits for a single PR is crazy and will add more confusion than clarity.

mike-tutkowski · 2016-04-08T18:16:06Z

No problem, @swill, I can just squash them all. I was just thinking that the fact that we are merging can preserve the individual history better (in a different branch that makes its way back to the master branch), but I'm fine with just dropping the history and having a single commit.

jburwell · 2016-04-08T18:50:30Z

@mike-tutkowski @swill I don't think a formal guideline has been set, but we have been pushing toward adopting more idiomatic Java naming conventions. The use of _, m_, and s_ prefixes for class members is not idiomatic. Generally, Java instance members are camel case and static members are all uppercase. It would also be nice to adopt the more stand naming of LOGGER or LOG rather than s_logger. If it is not too much trouble, I would encourage you to remove these prefixes.

jburwell · 2016-04-08T18:52:15Z

@mike-tutkowski why can't the Marbin test cases be checked in? While we can't run them, it would be nice to review them to understand exactly how the feature is being tested. We can exclude them by default to avoid running into problems on full runs that don't have SolidFire hardware available.

mike-tutkowski · 2016-04-08T18:54:52Z

The CloudStack community can definitely have those tests, if it wants them.

Right now, however, we don't have any way to enable other people (besides those who own SolidFire equipment) to successfully run those tests. Perhaps SolidFire will have a virtual node at some point that can be handed over to Apache Infra for this purpose.

jburwell · 2016-04-08T18:55:45Z

+    private boolean _executeInSequence = true;
+
+    public ResignatureCommand(final Map<String, String> details) {
+        _details = details;


Copy the details to an ImmutableMap map to avoid any side-effects by the caller making subsequence changes to the map. It will also prevent the same side-effects when returned from getDetails.

mike-tutkowski · 2016-04-08T19:02:13Z

I'm not sure what the standard for "idiomatic" is here. :) I've pretty much usually seen _ (and sometimes m_) used to note a member variable versus a local variable. That is then used in concert with camel case.

Also, for static member variables that are public, I usually have seen all upper case with individual words separated by an underscore (but the variable not prefixed with an underscore).

We should probably develop a community standard.

jburwell · 2016-04-08T19:02:39Z

    }

-    private boolean canHandle(DataStore dataStore) {
+    private boolean canHandle(DataObject dataObject) {


What is dataObject is null?

Would it be possible to add some unit test cases for this method?

I would say that that is an infrastructure fail on the part of CloudStack (passing in null there). We could defend against it, but then we should probably do so everywhere where canHandle is implemented.

@mike-tutkowski while it's unlikely, we should defend against if for nothing more than providing a better error than an NPE. Seems like a good place to use Preconditions.checkArgument with a meaningful error message. It also better pinpoints the origin of the failure.

mike-tutkowski · 2016-05-12T20:03:52Z

@DaanHoogland I think I addressed all your concerns. I plan to rebuild this locally, then push to GitHub in a bit.

mike-tutkowski · 2016-05-12T20:21:16Z

@DaanHoogland I had to put the "Void" return types back. It is used for AOP and won't compile with "void" for those two methods.

jburwell · 2016-05-12T22:34:01Z

+        }
+
+        return true;
+    }


The DAO calls in the for loop are concerning from a performance perspective (both for the speed of this call and the load placed on the database). Ideally, it would be calculated in a single join. Would it be possible to craft a query across the details for all hosts in a cluster where the detail key = "supportsResign" and the value = "true"?

Isn't there a limit of like 16 - 32 hosts in a cluster (therefore only 16 - 32 DB calls in the for loop)?

See what you think...I just added a method on the HostDetailsDao to return a Map where the key is the ID of the host and the value is true or false (you pass in the "name" field you are interested in).

This way we just do one query and then look up the results in the map each time through the "for" loop.

If we want to do a join of the host, host_details, and cluster tables (which, I believe, would be necessary to reduce the results coming back from the DB), perhaps you know of somewhere in our codebase where we do some similar action? Thanks

Actually, I think we just need to join the host and the host_details tables (because cluster ID should be in the host table).

We'd want all of the hosts with a particular cluster ID selected from the host table and then we'd want to join on host ID in the host_details table and further refine the returned hosts by those that have a "supportsResign" value in the name field.

Do you happen to know of any code of ours that allows you to SQL filter the returned values based on values that exist in not just one, but two tables? From what I've seen, we seem to be pretty heavily filtering only on data present in one table (even when we join with another table).

If this were just raw SQL, it would be pretty easy, but we've got that Java DB layer that this needs to fit within.

mike-tutkowski · 2016-05-13T20:06:53Z

@jburwell @DaanHoogland I have a solution for quickly looking up if a cluster supports resigning that I think we'll all be happy with.

Upon a host connecting to the management server is when I check to see if the host supports resigning (and update the host_details table).

I added logic to this connection code to not only update the host_details table with info about resigning, but also the cluster_details table about resigning.

A host connecting to the management server should not be a frequent occurrence, so I believe it's OK at this point to run through the list of hosts in the cluster of the connecting host and see if all of those hosts support resigning. If they do, then I update the cluster_details table that the cluster in question supports resigning.

I also changed the logic to not bother to add a row to either the host_details table or the cluster_details table if the "supportsResign" property would be false (only a true value is stored now). We can then save space by understanding that a missing "supportsResign" property for a host or cluster indicates false (I was proceeding under that assumption anyways).

When the time comes to ask the cluster if it supports resigning, it's a simple matter of looking for the "supportsResign" property in the cluster_details table.

jburwell · 2016-05-13T20:16:17Z

@DaanHoogland in my experience, stack traces are a critical piece of information for operationally debugging CloudStack. Unfortunately, our logging lacks the clarity and specify to use the error message alone. I hope our logging improves overtime to omit them from INFO, WARN, and ERROR. However, today, this information is vital to resolving issues.

DaanHoogland · 2016-05-13T21:15:02Z

Then log the warning with just the message and add a debug statement with the stacktrace. logging stacktraces at a level even more strict then INFO does not compute.

swill · 2016-05-17T12:26:16Z

CI RESULTS

Tests Run: 85
  Skipped: 0
   Failed: 0
   Errors: 0
 Duration: 4h 20m 17s

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_17_2016_00_15_14_6DMLAP:

/tmp/MarvinLogs/test_network_CRQ5C2:

/tmp/MarvinLogs/test_vpc_routers_KUJ7NL:

Uploads will be available until 2016-07-17 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-17T12:26:37Z

this one is ready...

GabrielBrascher reviewed Feb 6, 2016
View reviewed changes

rodrigo93 reviewed Feb 14, 2016
View reviewed changes

mike-tutkowski force-pushed the xs-snapshots branch from e2c8be2 to a425e41 Compare February 26, 2016 06:22

asfgit force-pushed the master branch from 7a9bfce to 359d20e Compare February 26, 2016 16:17

mike-tutkowski force-pushed the xs-snapshots branch from 005367b to 61352d6 Compare March 12, 2016 03:29

mike-tutkowski force-pushed the xs-snapshots branch from ecc5e4d to af73300 Compare March 24, 2016 18:56

mike-tutkowski force-pushed the xs-snapshots branch from 0270bcc to b93daca Compare April 6, 2016 17:31

mike-tutkowski force-pushed the xs-snapshots branch from b93daca to 1396cad Compare April 8, 2016 18:34

jburwell reviewed Apr 8, 2016
View reviewed changes

mike-tutkowski force-pushed the xs-snapshots branch from bbeaaf1 to d52ef81 Compare May 12, 2016 20:35

jburwell reviewed May 12, 2016
View reviewed changes

mike-tutkowski force-pushed the xs-snapshots branch from d52ef81 to 45e2bdc Compare May 13, 2016 06:57

Support for backend snapshots with XenServer

2bd035d

mike-tutkowski force-pushed the xs-snapshots branch from 45e2bdc to 2bd035d Compare May 13, 2016 07:02

mike-tutkowski force-pushed the xs-snapshots branch 2 times, most recently from 38f8cdf to aa6f12b Compare May 13, 2016 20:32

mike-tutkowski force-pushed the xs-snapshots branch from aa6f12b to d9e2d62 Compare May 14, 2016 22:26

Faster logic to see if a cluster supports resigning

9d21556

mike-tutkowski force-pushed the xs-snapshots branch from d9e2d62 to 9d21556 Compare May 16, 2016 13:19

asfgit merged commit 9d21556 into apache:master May 20, 2016

mike-tutkowski deleted the xs-snapshots branch May 20, 2016 16:35

mike-tutkowski mentioned this pull request Jan 4, 2017

CLOUDSTACK-9691: Fixed unhandeled excetion in list snapshot command when a primary store is deleted related to it #1847

Merged

Uh oh!

Conversation

mike-tutkowski commented Feb 6, 2016

Uh oh!

mike-tutkowski commented Feb 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented Feb 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syed commented Feb 15, 2016

Uh oh!

asfbot commented Feb 16, 2016

Uh oh!

bvbharatk commented Mar 13, 2016

ACS CI BVT Run

Uh oh!

mike-tutkowski commented Apr 8, 2016

Uh oh!

swill commented Apr 8, 2016

Uh oh!

mike-tutkowski commented Apr 8, 2016

Uh oh!

swill commented Apr 8, 2016

Uh oh!

mike-tutkowski commented Apr 8, 2016

Uh oh!

jburwell commented Apr 8, 2016

Uh oh!

jburwell commented Apr 8, 2016

Uh oh!

mike-tutkowski commented Apr 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented Apr 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburwell Apr 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented May 12, 2016

Uh oh!

mike-tutkowski commented May 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented May 13, 2016

Uh oh!

jburwell commented May 13, 2016

Uh oh!

DaanHoogland commented May 13, 2016

Uh oh!

swill commented May 17, 2016

CI RESULTS

Uh oh!

swill commented May 17, 2016

Uh oh!

jburwell Apr 8, 2016 •

edited

Loading