You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by anshul1886 <gi...@git.apache.org> on 2015/08/10 11:36:17 UTC
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/672
Allow VM snapshots and volume snapshots to exist together
This branch fixes issues involved in allowing VM snapshots and volume snapshots to exist together.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 vmsnapshots
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/672.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #672
----
commit 10295e99a6b03350a132d66444a5c26b2f0fa60a
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-07-24T09:15:20Z
Allow VM and volume snapshots to exist together
commit eed632026028385325a638cfd90ce65fc8ec3543
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-07-27T05:30:49Z
Reverting VM to disk only snapshot in Xenserver corrupts VM
commit c91f100b64e17b042dc1596e162f36e344389bcb
Author: Anshul Gangwar <an...@citrix.com>
Date: 2015-07-27T05:38:45Z
Stale NFS secondary storage on XS leads to volume creation failure from snapshot
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51385879
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
"wasting time, finding out how it works"? That is definitely a contradiction for someone trying to enhance the code. relying on javadoc is a no go fomr that perspective. Reading javadoc if you want to enhance a system like this would be were the time is wasted. The exact working as it can only be seen in the code is what it is about. Therefore if there is a call for comments of any kind the code must be revisited. If we are writing public extension points for others to base their work on our binaries I would consider javadoc as an answer but still I would want it to be gnerated from the code. the code is the truth and nothing but the code. sorry to rant on this, I have seen to much outdated comments in cloudstack.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51362827
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
@anshul1886 @DaanHoogland @rodrigo93 As an ideia. Maybe it could have a name like "removeSrIfNotNull" or "removeSrIfVdiOperationAndSrNotNull".
As an example, a documetation could look like:
`/** Removes the SR if the SR is not null and exists a VDI operation for this SR*/`
Do you think that one of this options (or anoter in your mind) could improve this code? :-)
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by rodrigo93 <gi...@git.apache.org>.
Github user rodrigo93 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51352994
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
Hi @anshul1886 , since you are adding a new method, consider making a JavaDoc for it.
It makes things more practicable and readable.
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
@rhtyd @karuturi @syed Raised #1941 against 4.9.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #672: Allow VM snapshots and volume snapshots to exi...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 closed the pull request at:
https://github.com/apache/cloudstack/pull/672
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/672#issuecomment-216186265
@anshul1886 please rebase against latest master and share status of your PR; also please update on outstanding comments from other reviewers
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/672#issuecomment-131698325
@anshul1886 Thanks for the info. If this is the second part, is the first part already merged? If so, what was the PR nr?
What would be a good way to test/verify this?
All: This confluence doc (linked in jira) has a detailed description: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=60624966
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51362146
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
NP @GabrielBrascher My point is not with the protection level but with comment being a last resort that should only decided upon to add if all else fails. Thought last resort is further away the higher the protection level.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51362259
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
@anshul1886 Could this method be void? Seems that the returned String is not used (see the line 744 of the "com.cloud.hypervisor.xenserver.resource.Xenserver625StorageProcessor" class).
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/672#issuecomment-131674635
@remibergsma Updated the description with bug link. These commits covers only second part i.e. allowing volume and VM snapshots to exist together.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
@karuturi @rhtyd @syed PR #1829 is subset of this PR i.e. that contains the same fix as one commit of this PR. How to proceed with that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/672
@anshul1886 yes given you've created a new PR, you may close this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by rodrigo93 <gi...@git.apache.org>.
Github user rodrigo93 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51367120
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
@DaanHoogland, sorry to not share your opinion, but, since ACS is open-source I think that "JavaDocing" could be very handy and turns things more organized when needed.
Those methods like "geters" and "seters" may not received a javadoc because they are self-explanatory. But since others people will read your code in the future, it may worth wasting some time JavaDocing the method. That will take you just a few minutes to do it and it is better than letting other people wasting much more time to find out what that code do or how it works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/672#issuecomment-131736170
@remibergsma First part of snapshot improvements is not yet implemented.
To test verify following operations
VM snapshots and volume snapshots created in any order should work as expected
VM and volume snapshots related operation should be performed with at least 3-4 levels of both type of snapshots.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51358645
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
Please don't make comments, even javadoc when not needed. this is a protected method!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
@rhtyd @karuturi Should I close this PR then?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
@jburwell those are fixing different bugs but all all needed to enable this that's why they are kept as separate tickets.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #672: Allow VM snapshots and volume snapshots to exi...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r73613353
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
--- End diff --
Please collapse these two catch blocks with a multi-catch to reduce duplication. Also, why is the issue being logged to ``WARN`` and not ``ERROR``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51361803
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
@GabrielBrascher Pointing to oracle is not a good support for your point.
You give a good example of why a well named method should still be documented. It's name does not make clear how it works. This should be addressed in a different way in my opinion: Can this be renamed to better depict what it does and only if all else fails should the user have to RTFM. @rodrigo93 starts with asking for javadoc. Such questions have been asked a lot lately and mostly unjust (not just by @rodrigo93 ) In my opinion this is not the way to improve our codebase. The question could we rename the method (or other code element) to something more descriptive had not been asked.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #672: Allow VM snapshots and volume snapshots to exi...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r73613647
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
I agree with @GabrielBrascher -- there appears to be no value added by returning a ``String``. Why not change the return type to ``void``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #672: Allow VM snapshots and volume snapshots to exi...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r73613017
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -1402,6 +1402,12 @@ public VM createWorkingVM(final Connection conn, final String vmName, final Stri
vbdr.userdevice = Long.toString(volumeTO.getDeviceId());
vbdr.mode = Types.VbdMode.RW;
vbdr.type = Types.VbdType.DISK;
+ Long deviceId = volumeTO.getDeviceId();
+ if (deviceId != null) {
+ if (!isDeviceUsed(conn, vm, deviceId) || deviceId.longValue() > 3) {
--- End diff --
Calling ``longValue`` is unnecessary due to autoboxing. Also, ``3`` seems like a magic value. What is its significance?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/672
Thanks @anshul1886 kindly also squash your changes and fix the commit message/jira id etc
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/672
@anshul1886 I would suggest, if your PR already has @syed 's fix along with other useful fixes -- please rebase against 4.9, fix the conflicts and we can help review/test your PR if you've time/bandwidth? Alternatively, we can merge @syed 's PR first since it's a rather small change and already has review/tests, and we can merge/test your PR once that is ready for review/test/merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51360756
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
@DaanHoogland With all due respect, I don't see why a protected method can't be documented.
In my point of view, private methods don't have to be documented (but may benefit from it), but protected methods might be treated as public (in terms of documenting). Even Oracle gives an example of a protected constructor with javadoc http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#defaultconstructors. In fact, at Xenserver625StorageProcessor class, this method is seen as "public".
The question here should be: Is this method self-explanatory?
I think that with respect to its act Yes. Althought it could be documented clarifying when this method skips and when it removes the SR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/672
@anshul1886 can you retarget this PR for 4.9? Also rebase against 4.9, fix conflicts thanks. /cc @karuturi @syed @koushik-das
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
Resolving conflicts and rebasing the PR against 4.9
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51486290
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
Don't worry about being rude with me. worry about building the best system we can ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by rodrigo93 <gi...@git.apache.org>.
Github user rodrigo93 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51428218
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
+ continue;
+ }
+ return null;
+ }
+ removeSR(conn, sr);
+ return null;
+ } catch (XenAPIException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.toString()), e);
+ } catch (XmlRpcException e) {
+ s_logger.warn(logX(sr, "Unable to get current opertions " + e.getMessage()), e);
+ }
+ String msg = "Remove SR failed";
+ s_logger.warn(msg);
+ return msg;
--- End diff --
@DaanHoogland I might have choose the wrongs words, but summarizing my idea, what I wanted to say is that I thought it would be good to have a well documented system of this size. It might take a few minutes to do so when someone writes the code, but it would turn the system "more standardized".
Sorry if I sounded rude before, but I was just trying to help.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/672
@anshul1886 could please squash the three commits and add the ticket reference to the commit message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/672#issuecomment-131231785
@anshul1886 Thanks! Can you add a Jira issue and provide a bit more details?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51359410
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
+ if (sr == null) {
+ return null;
+ }
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug(logX(sr, "Removing SR"));
+ }
+ try {
+ Set<VDI> vdis = sr.getVDIs(conn);
+ for (VDI vdi : vdis) {
+ Map<java.lang.String, Types.VdiOperations> currentOperation = vdi.getCurrentOperations(conn);
+ if (currentOperation == null || currentOperation.size() == 0) {
--- End diff --
@anshul1886 Could you please use MapUtils.isEmpty()?
It is a null-safe check if the specified map is empty (https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/MapUtils.html#isEmpty%28java.util.Map%29).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Allow VM snapshots and volume snapshots t...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/672#discussion_r51362068
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -4251,6 +4257,34 @@ public void rebootVM(final Connection conn, final VM vm, final String vmName) th
}
}
+ protected String skipOrRemoveSR(Connection conn, SR sr) {
--- End diff --
@DaanHoogland I thought that your point was with the fact of being a protected method (not with the fact of being self-explanatory).
Sorry, but when I said, "Is this method self-explanatory?" I meant about the name of it, which I think is self-explanatory (says exactly what it does). Although a documentation could (not should) be applied.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #672: Allow VM snapshots and volume snapshots to exist toge...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the issue:
https://github.com/apache/cloudstack/pull/672
### ACS CI BVT Run
**Sumarry:**
Build Number 133
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---