[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1635


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1635#discussion_r77279256
  
--- Diff: 
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
 ---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse, 
@BeanParam DeploymentPlan plan, E
 boolean stop(String caller) throws ResourceUnavailableException, 
CloudException;
 
 /**
+ * Stop the virtual machine, optionally force
+ *
+ */
+boolean stop(String caller, boolean forced) throws 
ResourceUnavailableException, CloudException;
--- End diff --

@rhtyd since this PR adds new methods, let's not carry it forward.  The 
implementation of ``forceStop`` would end up passing ``true`` for the 
``forced`` flag down the stack, but at least we are not expanding an 
anti-pattern.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1635#discussion_r76569173
  
--- Diff: 
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
 ---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse, 
@BeanParam DeploymentPlan plan, E
 boolean stop(String caller) throws ResourceUnavailableException, 
CloudException;
 
 /**
+ * Stop the virtual machine, optionally force
+ *
+ */
+boolean stop(String caller, boolean forced) throws 
ResourceUnavailableException, CloudException;
--- End diff --

@jburwell the `forced` flag has always existed, the `stopVirtualMachine` 
API accepts this and the query layer too has a method with this signature 
(https://github.com/apache/cloudstack/pull/1635/files#diff-0a1cd9df984252594918eec5acfed08cR3850)
 but it does not honour or pass this flag to the layers below, so effectively 
stopVM API with/without the `forced` flag has the same effect.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1635#discussion_r76550534
  
--- Diff: 
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
 ---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse, 
@BeanParam DeploymentPlan plan, E
 boolean stop(String caller) throws ResourceUnavailableException, 
CloudException;
 
 /**
+ * Stop the virtual machine, optionally force
+ *
+ */
+boolean stop(String caller, boolean forced) throws 
ResourceUnavailableException, CloudException;
--- End diff --

@nathanejohnson I wasn't thinking to change any of the existing code.  I am 
just interested in avoiding incurring any more flag argument technical debt.

In terms of testing, this change is not hypervisor specific.  Therefore, a 
test case that utilizes the simulator might be a more fruitful path.  @rhtyd 
may have some thoughts as to how you could accomplish it.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[nathanejohnson]

Github user nathanejohnson commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1635#discussion_r76547668
  
--- Diff: 
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
 ---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse, 
@BeanParam DeploymentPlan plan, E
 boolean stop(String caller) throws ResourceUnavailableException, 
CloudException;
 
 /**
+ * Stop the virtual machine, optionally force
+ *
+ */
+boolean stop(String caller, boolean forced) throws 
ResourceUnavailableException, CloudException;
--- End diff --

For better or worse, I was just trying to follow the lead of the actual 
method that does the work in question, namely advanceStop, and some of the 
methods called from advanceStop.  I have no issue changing the name to have 
force in it and skip the bool flag (lest Fowler think I'm a bad programmer), 
but I am not thinking that I'd like to change the signature of advanceStop or 
any other the other methods that eventually get called in the chain where there 
is a bool flag in it.  I think the much bigger issue with this bit of code is 
that the refactor that lead to this issue in the first place abstracted the 
call to advanceStop under so many layers that it made it really hard to notice 
that functionality was reduced in the first place, to the point where the 
author nor anyone reviewing caught it.  If I was to place serious energy into 
refactoring, I'd probably start with undoing a lot of that abstraction, but 
that would require me being familiar with a lot more of the codebase than I
  currently am.  Also, more immediately, I want to come up with a good test for 
this, but so far haven't been able to come up with something reliable / 
reproducible under kvm.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1635#discussion_r76544594
  
--- Diff: 
engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
 ---
@@ -115,6 +115,12 @@ String reserve(DeploymentPlanner plannerToUse, 
@BeanParam DeploymentPlan plan, E
 boolean stop(String caller) throws ResourceUnavailableException, 
CloudException;
 
 /**
+ * Stop the virtual machine, optionally force
+ *
+ */
+boolean stop(String caller, boolean forced) throws 
ResourceUnavailableException, CloudException;
--- End diff --

[Flag arguments](http://martinfowler.com/bliki/FlagArgument.html) are a 
nasty code smell/anti-pattern.  Please consider changing method to something 
more descriptive with a flag argument such as ``forceStop(String caller)``.  
The contrasting version of the method already exists on the interface so it 
would be a compatible change.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1635: CLOUDSTACK-9451

[nathanejohnson]

GitHub user nathanejohnson opened a pull request:

https://github.com/apache/cloudstack/pull/1635

CLOUDSTACK-9451

https://issues.apache.org/jira/browse/CLOUDSTACK-9451

Re-doing against 4.8 since this is a bug, not a feature.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/myENA/cloudstack feature/honor_force_stop_vm

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1635.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 #1635


commit d5471b94e7617565836c1460007cbf6afa4c12ec
Author: Nathan Johnson 
Date:   2016-08-11T12:52:05Z

CLOUDSTACK-9451

https://issues.apache.org/jira/browse/CLOUDSTACK-9451

Re-doing against 4.8 since this is a bug, not a feature.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---