You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by mike-tutkowski <gi...@git.apache.org> on 2015/09/14 08:46:51 UTC
[GitHub] cloudstack pull request: Notify listeners when a host has been add...
GitHub user mike-tutkowski opened a pull request:
https://github.com/apache/cloudstack/pull/816
Notify listeners when a host has been added to a cluster, is about to…
… be removed from a cluster, or has been removed from a cluster
This PR addresses the following JIRA ticket:
https://issues.apache.org/jira/browse/CLOUDSTACK-8813
The problem is that there needs to be notifications sent when a host is added to, about to be removed from, and removed from a cluster.
Such notifications can be used for many purposes. For example, it can allow storage plug-ins to update ACLs on their storage systems. Also, it can allow us to clean up IQNs from ESXi hosts that are no longer needed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mike-tutkowski/cloudstack addremovehosts2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/816.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 #816
----
commit 088eb45fcd1618624b72af3c3eba0b19acb77aac
Author: Mike Tutkowski <mi...@solidfire.com>
Date: 2015-08-14T00:44:12Z
Notify listeners when a host has been added to a cluster, is about to be removed from a cluster, or has been removed from a cluster
----
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547042
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -18,40 +18,69 @@
*/
package org.apache.cloudstack.storage.datastore.provider;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
import javax.inject.Inject;
import org.apache.log4j.Logger;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener;
+import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.cloudstack.storage.datastore.util.SolidFireUtil;
import com.cloud.agent.AgentManager;
import com.cloud.agent.api.Answer;
import com.cloud.agent.api.ModifyStoragePoolAnswer;
import com.cloud.agent.api.ModifyStoragePoolCommand;
+import com.cloud.agent.api.ModifyTargetsCommand;
import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.dao.ClusterDao;
import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.StoragePool;
import com.cloud.storage.StoragePoolHostVO;
+import com.cloud.storage.VolumeVO;
import com.cloud.storage.dao.StoragePoolHostDao;
+import com.cloud.storage.dao.VolumeDao;
import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.dao.VMInstanceDao;
public class SolidFireHostListener implements HypervisorHostListener {
private static final Logger s_logger = Logger.getLogger(SolidFireHostListener.class);
- @Inject
- private AgentManager _agentMgr;
- @Inject
- private AlertManager _alertMgr;
- @Inject
- private DataStoreManager _dataStoreMgr;
- @Inject
- private HostDao _hostDao;
- @Inject
- private StoragePoolHostDao storagePoolHostDao;
+ @Inject private AgentManager _agentMgr;
+ @Inject private AlertManager _alertMgr;
+ @Inject private ClusterDao _clusterDao;
+ @Inject private ClusterDetailsDao _clusterDetailsDao;
+ @Inject private DataStoreManager _dataStoreMgr;
+ @Inject private HostDao _hostDao;
+ @Inject private PrimaryDataStoreDao _storagePoolDao;
+ @Inject private StoragePoolDetailsDao _storagePoolDetailsDao;
+ @Inject private StoragePoolHostDao storagePoolHostDao;
+ @Inject private VMInstanceDao _vmDao;
+ @Inject private VolumeDao _volumeDao;
+
+ @Override
+ public boolean hostAdded(long hostId) {
+ HostVO host = _hostDao.findById(hostId);
+
+ SolidFireUtil.hostAddedToOrRemovedFromCluster(hostId, host.getClusterId(), true, SolidFireUtil.PROVIDER_NAME,
+ _clusterDao, _clusterDetailsDao, _storagePoolDao, _storagePoolDetailsDao, _hostDao);
+
+ handleVMware(host, true);
--- End diff --
For now, yes (hypervisor hosts that require that we explicitly remove iSCSI targets in this scenario).
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220295817
I see.
Shouldn't the system be using annotations to make this less brittle.
At present, there's no obvious way to see that changing these variable names will break anything.
At the least, we should have unit tests that look for certain variable names in certain classes and fail if expected ones no longer exist.
On May 18, 2016, at 11:05 PM, Anshul Gangwar <no...@github.com>> wrote:
@mike-tutkowski<https://github.com/mike-tutkowski> First one which I hit is in ModifyStoragePoolCommand.java. But I am sure there will be more. Hyper-V uses json to communicate between management server and Hyper-V agent. So if we are changing variable names it no longer will be able to parse that info.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<https://github.com/apache/cloudstack/pull/816#issuecomment-220228493>
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390690
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
}
private void createDefaultEgressFirewallRule(final List<FirewallRule> rules, final long networkId) {
- String systemRule = null;
+ // String systemRule = null;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable systemRule was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390730
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java ---
@@ -106,7 +106,7 @@ public boolean processAnswers(long agentId, long seq, Answer[] answers) {
}
}
}
- commandNum++;
+ // commandNum++;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable commandNum was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-144619954
So, I've been paging through the diff files to see what might be a good candidate for unit testing.
Unfortunately, I don't really see much.
What I do see (in terms of added/changed code) is the following:
* Removal of a constructor (compiler would catch any issues here).
* Renaming of variables (typically adding a "_" before a member variable).
* Some new setters and getters (technically could do unit testing here to confirm that what we place in the object is what we get out of it, but most of the time we are just directly setting a member variable and retrieving from it, so it's pretty low risk).
* Adding "private" and "protected" where previously we had "public" (the compiler should catch issues here).
* Making a previously private method public and making it accessible via an interface.
* Adding three methods to the HypervisorHostListener interface: hostAdded, hostAboutToBeRemoved, and hostRemoved (but not really having any hooks into the system to be able to "pretend" like I'm adding or removing a host).
* Removing the word "public" from the methods of an interface since all methods in an interface are public in Java.
* Adding three methods to the Listener interface: processHostAdded, processHostAboutToBeRemoved, and processHostRemoved (but not really having any hooks into the system to be able to "pretend" like I'm adding or removing a host). Also, this leads to a need to make a simple, default "do nothing" implementation of these methods for about 30 classes.
* Adding an extra command to allow while a host is in maintenance mode (but not having any way to simulate maintenance mode via unit testing).
* Adding a new command that VmwareResource can respond to. However, the command requires interaction with an ESXi host, so can't really be tested in isolation (i.e. via unit testing).
* Changes to the way DB locking is performed in a couple scenarios with the SolidFire storage plug-ins (requiring an underlying DB).
* Reformatting of text to better conform to ACS formatting guidelines.
* Removal of "dead" code that I came across while working on this ticket.
I'd be happy to entertain ideas on unit testing this PR, but I'm thinking we'd get more "mileage" here out of adding integration tests.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366830
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java ---
@@ -115,7 +115,7 @@
protected String _guestNic;
protected boolean _setupMultipath;
protected String _instance;
- private String xs620snapshothotfix = "Xenserver-Vdi-Copy-HotFix";
+ // private String xs620snapshothotfix = "Xenserver-Vdi-Copy-HotFix";
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216189633
@mike-tutkowski please rebase against latest master and push -f, update on status of your PR
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218228119
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 1
Errors: 0
Duration: 9h 15m 46s
```
**Summary of the problem(s):**
```
FAIL: Test redundant router internals
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_routers_network_ops.py", line 483, in test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false
"Attempt to retrieve google.com index page should be successful once rule is added!"
AssertionError: Attempt to retrieve google.com index page should be successful once rule is added!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_5B8X28/results.txt
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_10_2016_06_46_02_Z1YVGT:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/DeployDataCenter__May_10_2016_06_46_02_Z1YVGT/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/DeployDataCenter__May_10_2016_06_46_02_Z1YVGT/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/DeployDataCenter__May_10_2016_06_46_02_Z1YVGT/runinfo.txt)
**`/tmp/MarvinLogs/test_network_5B8X28:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_network_5B8X28/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_network_5B8X28/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_network_5B8X28/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_01VL92:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_vpc_routers_01VL92/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_vpc_routers_01VL92/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR816/tmp/MarvinLogs/test_vpc_routers_01VL92/runinfo.txt)
Uploads will be available until `2016-07-10 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62511172
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -91,18 +280,5 @@ public boolean hostConnect(long hostId, long storagePoolId) {
assert (answer instanceof ModifyStoragePoolAnswer) : "ModifyStoragePoolAnswer expected ; Pool = " + storagePool.getId() + " Host = " + hostId;
--- End diff --
Off topic but I wanted to know what is the take on asserts? Personally I love asserts but not sure what the community thinks of 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62508121
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -933,7 +937,7 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
*/
--- End diff --
Not directly related to the PR but I've been seening the discussion and I've also seen that you've cleaned up a lot of the code @mike-tutkowski. is it safe to remove this commented block also?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39394113
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java ---
@@ -124,6 +132,22 @@
private final String _clusterAdminPassword;
public SolidFireConnection(String managementVip, int managementPort, String clusterAdminUsername, String clusterAdminPassword) {
--- End diff --
Good catch, Wido. How's about I put that on my todo list (and open a PR after this one)? It would end up changing a lot of lines of code in this PR that might distract from the main intention of this work.
---
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: Notify listeners when a host has been add...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/816
---
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: Notify listeners when a host has been add...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39370166
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java ---
@@ -124,6 +132,22 @@
private final String _clusterAdminPassword;
public SolidFireConnection(String managementVip, int managementPort, String clusterAdminUsername, String clusterAdminPassword) {
--- End diff --
Shouldn't a IP be passed as a InetAddress inside the code?
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366998
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -113,7 +113,7 @@ protected void runInContext() {
private DataStore sserver;
- private boolean uploadActive = true;
+ // private boolean uploadActive = true;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-141960991
Hi @mike-tutkowski
Unfortunately, I don't think I will have the time to help you with unit tests for this PR.
I will again need to direct my sprint work to internal projects.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366792
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -3327,7 +3330,7 @@ private Answer execute(MigrateVolumeCommand cmd) {
// Consolidate VM disks.
// In case of a linked clone VM, if VM's disks are not consolidated,
// further volume operations on the ROOT volume such as volume snapshot etc. will result in DB inconsistencies.
- String apiVersion = HypervisorHostHelper.getVcenterApiVersion(vmMo.getContext());
+ // String apiVersion = HypervisorHostHelper.getVcenterApiVersion(vmMo.getContext());
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216327328
Here is an integration test for this:
[TestAddRemoveHosts.py.txt](https://github.com/apache/cloudstack/files/245702/TestAddRemoveHosts.py.txt)
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140095795
Hi @mike-tutkowski
Thanks for being open about removing the commented-out code. IMHO (and not just mine btw) commented-out code poses a serious risk for maintainability. I'll be happy to dive in deeper on the topic if you would like to. Over beers would be the best setting :)
Regarding the unit tests, I cannot make any time this week since I'm already behind schedule for completing my sprint. But I will for sure reserve time next sprint (starting on Monday), to write some unit tests for the code changes in this PR. We can then discuss how to make the test suite more complete.
---
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: Notify listeners when a host has been add...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220228493
@mike-tutkowski First one which I hit is in ModifyStoragePoolCommand.java. But I am sure there will be more. Hyper-V uses json to communicate between management server and Hyper-V agent. So if we are changing variable names it no longer will be able to parse that info.
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218354565
Can you re-push to try to get Jenkins green. Thx...
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39367006
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -270,7 +274,7 @@ public AgentControlAnswer processControlCommand(long agentId, AgentControlComman
}
public void setUploadInactive(Status reason) {
- uploadActive = false;
+ // uploadActive = false;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390123
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -3327,7 +3330,7 @@ private Answer execute(MigrateVolumeCommand cmd) {
// Consolidate VM disks.
// In case of a linked clone VM, if VM's disks are not consolidated,
// further volume operations on the ROOT volume such as volume snapshot etc. will result in DB inconsistencies.
- String apiVersion = HypervisorHostHelper.getVcenterApiVersion(vmMo.getContext());
+ // String apiVersion = HypervisorHostHelper.getVcenterApiVersion(vmMo.getContext());
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable apiVersion was not in use. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390966
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -270,7 +274,7 @@ public AgentControlAnswer processControlCommand(long agentId, AgentControlComman
}
public void setUploadInactive(Status reason) {
- uploadActive = false;
+ // uploadActive = false;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable uploadActive was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390917
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -39,7 +39,7 @@
import com.cloud.utils.db.DB;
public class LocalStoragePoolListener implements Listener {
- private final static Logger s_logger = Logger.getLogger(LocalStoragePoolListener.class);
+ // private final static Logger s_logger = Logger.getLogger(LocalStoragePoolListener.class);
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable s_logger was not in use. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546243
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
/**
+ * This method is called by AgentManager when a host is about to be removed from a cluster.
+ * @param long the ID of the newly added host
+ */
+ void processHostAboutToBeRemoved(long hostId);
+
+ /**
+ * This method is called by AgentManager when a host is removed from a cluster.
+ * @param long the ID of the newly added host
--- End diff --
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220019059
Hi @anshul1886 Can you be more specific about the class that is given you problems and the variables? 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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216987677
Hi @mike-tutkowski
It has indeed been a long time :)
None the less, it's great that you took the time time write that integration test. I think it's good to have it even if you are maybe the only one that can run it. I mean, you could be the "gate keeper" for the stability and reliability of the SolidFire plugin, and report on the test you run.
I haven't looked in detail at the test, but I'm definitely very happy to see it as part of this PR. One suggestion that I make (and it's entirely up to you to decide to do it or not), is to commit the test as part of the PR (I scanned the list of files, while on my phone, and didn't see it there). A good location for it (one that causes less friction with the BTV runs) could be 'test/integration/plugins'.
Cheers
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547209
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -65,22 +94,182 @@ public boolean hostConnect(long hostId, long storagePoolId) {
storagePoolHostDao.persist(storagePoolHost);
}
- // just want to send the ModifyStoragePoolCommand for KVM
- if (host.getHypervisorType() != HypervisorType.KVM) {
- return true;
+ if (host.getHypervisorType().equals(HypervisorType.XenServer)) {
+ handleXenServer(host.getClusterId(), host.getId(), storagePoolId);
+ }
+ else if (host.getHypervisorType().equals(HypervisorType.KVM)) {
+ handleKVM(hostId, storagePoolId);
+ }
+
+ return true;
+ }
+
+ @Override
+ public boolean hostDisconnected(long hostId, long storagePoolId) {
+ StoragePoolHostVO storagePoolHost = storagePoolHostDao.findByPoolHost(storagePoolId, hostId);
+
+ if (storagePoolHost != null) {
+ storagePoolHostDao.deleteStoragePoolHostDetails(hostId, storagePoolId);
+ }
+
+ return true;
+ }
+
+ @Override
+ public boolean hostAboutToBeRemoved(long hostId) {
+ HostVO host = _hostDao.findById(hostId);
+
+ handleVMware(host, false);
--- End diff --
Unfortunately, it appears to be too late at that point because CloudStack can't talk to the host anymore.
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218465085
I have seen this happen periodically and it does not seem to be consistently telling us the truth. This happens sometimes when only a single line of code changes, so I am not sure what the deal is here. Just re-push and lets see what happens.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62545797
--- Diff: engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/HypervisorHostListener.java ---
@@ -21,7 +21,13 @@
import com.cloud.exception.StorageConflictException;
public interface HypervisorHostListener {
+ boolean hostAdded(long hostId);
+
boolean hostConnect(long hostId, long poolId) throws StorageConflictException;
boolean hostDisconnected(long hostId, long poolId);
+
+ boolean hostAboutToBeRemoved(long hostId);
--- End diff --
This event was added specifically to address hypervisors like VMware that need to be told to remove iSCSI targets.
Those kind of hypervisor hosts need to be notified to do this before the host is removed because CloudStack can no longer talk to those hosts once they're in the Removed state.
---
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: Notify listeners when a host has been add...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218480720
That is the rat test. it is usually right. I didn't see a new file without license though. May one of the to with a license starting with an empty comment line were the problem. Otherwise the job may be the problem in the sense of not cleaning the workspace properly.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220514243
I wonder how many people are actually aware of that. :-)
Sent from my iPhone
On May 19, 2016, at 9:04 PM, Anshul Gangwar <no...@github.com>> wrote:
@mike-tutkowski<https://github.com/mike-tutkowski> I believe class name ending in Command signifies that they are meant for agent and should be treated like API params.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<https://github.com/apache/cloudstack/pull/816#issuecomment-220512436>
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366965
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -39,7 +39,7 @@
import com.cloud.utils.db.DB;
public class LocalStoragePoolListener implements Listener {
- private final static Logger s_logger = Logger.getLogger(LocalStoragePoolListener.class);
+ // private final static Logger s_logger = Logger.getLogger(LocalStoragePoolListener.class);
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220517071
?OK, I can create one and open a PR up against it reverting those variable names.
________________________________
From: Anshul Gangwar <no...@github.com>
Sent: Thursday, May 19, 2016 10:50 PM
To: apache/cloudstack
Cc: Tutkowski, Mike; Mention
Subject: Re: [apache/cloudstack] Notify listeners when a host has been added to a cluster, is about to\u2026 (#816)
@mike-tutkowski<https://github.com/mike-tutkowski> No, I have not created ticket. You can create one.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<https://github.com/apache/cloudstack/pull/816#issuecomment-220516738>
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140189221
Just an FYI that I went ahead and updated the code to remove the commented-out lines.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140077337
Hi again Mike,
Thanks for addressing my questions.
Regarding the commented-out code, I see from your answers that the reason for leaving commented-out code was to make it easy to revert it back in the future.
I also see that you did not follow the same reasoning for other code changes you made in this same PR. And I argue that those changes are also very easy to revert. For that we need only to leverage the version control system. It is easy to track the changes of a given file, and recover it's previous states.
May I recommend that you remove the commented out code, since we still keep the possibility to easily revert those changes in the future?
Regarding the automated testing, I'm very happy to hear that you are working on integration test. I assume from your answer that those integration tests are not committed in the git repository, and that is something I would expect to see happen. Please do correct me if my assumption is wrong.
I trust you when you say that "there is essentially no existing test code" covering the code you changed. I wouldn't go as far as to call that fortunate. I actually think it is a very sad truth.
I think we should unit-test as much as we can, and especially targeting code that has no coverage at all.
I'm sure that given the amount of changes made in this PR there is ample room for covering at least some of it with unit-tests.
Fact is, that if we continue to add code without the complementary unit-tests, our test coverage drops. Is that something we want?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390959
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -113,7 +113,7 @@ protected void runInContext() {
private DataStore sserver;
- private boolean uploadActive = true;
+ // private boolean uploadActive = true;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable uploadActive was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390243
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4420,11 +4450,13 @@ private void fillHostDetailsInfo(VmwareContext serviceContext, Map<String, Strin
}
}
+ /*
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that this method was not in use. Instead of removing the entire method, I just commented it out in case someone later wanted to easily resume making use of 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505386
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
/**
+ * This method is called by AgentManager when a host is about to be removed from a cluster.
+ * @param long the ID of the newly added host
--- End diff --
typo?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546214
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
/**
+ * This method is called by AgentManager when a host is about to be removed from a cluster.
+ * @param long the ID of the newly added host
--- End diff --
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217437287
Thanks for your time on the review, @miguelaferreira!
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218374540
@swill Done. Hopefully it will work for Jenkins this time.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62548252
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -1570,17 +1578,35 @@ private boolean checkCIDR(final HostPodVO pod, final String serverPrivateIP, fin
return true;
}
+ private HostVO isNewHost(StartupCommand[] startupCommands) {
--- End diff --
Changed - 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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217226582
@miguelaferreira I went ahead and added four integration tests to /test/integration/plugins/solidfire.
Please let me know if you're OK with the PR. I believe I have another guy who's going to be the second reviewing taking a look at it once he's back from vacation.
Thanks for your time on this!
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390303
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java ---
@@ -115,7 +115,7 @@
protected String _guestNic;
protected boolean _setupMultipath;
protected String _instance;
- private String xs620snapshothotfix = "Xenserver-Vdi-Copy-HotFix";
+ // private String xs620snapshothotfix = "Xenserver-Vdi-Copy-HotFix";
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable xs620snapshothotfix was not in use. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390513
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1589,8 +1589,8 @@ protected StringBuilder createGuestBootLoadArgs(final NicProfile guestNic, final
protected StringBuilder createRedundantRouterArgs(final NicProfile nic, final DomainRouterVO router) {
final StringBuilder buf = new StringBuilder();
- final long networkId = nic.getNetworkId();
- final NetworkVO network = _networkDao.findById(networkId);
+ // final long networkId = nic.getNetworkId();
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable network was not in use. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
Once I commented it out, Eclipse pointed out that the variable networkId was no longer in use. Same story for it with regards to commenting it out.
---
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: Notify listeners when a host has been add...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220512436
@mike-tutkowski I believe class name ending in Command signifies that they are meant for agent and should be treated like API params.
---
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: Notify listeners when a host has been add...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220514841
@mike-tutkowski then we can raise this in dev list.
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218456885
@mike-tutkowski unfortunately it failed again. :(
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216337441
Here are the results of my automated regression test:
test_add_remove_host_with_solidfire_plugin (TestAddRemoveHosts.TestAddRemoveHosts) ... === TestName: test_add_remove_host_with_solidfire_plugin | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 520.682s
OK
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-139986534
Hi @mike-tutkowski
I've skimmed through the code in this PR and I saw quite a lot of nice modifications. Things like proper formatting, stricter object encapsulation, breaking code down in small methods, just to name a few.
However, I found it quite strange that in a PR where you've changed 58 files (adding 1,410 and removing 214 lines), you did not touch a single test class. (Although you touched 1 class that is used for integration testing - DirectAgentManagerSimpleImpl.)
That seems to fall short from any reasonable amount of automated test coverage for your PR.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140097887
Sure, I'd be happy to get your input on how you see us unit testing this code. In the past, I've mainly restricted my CloudStack testing (automated and manual) to integration tests (in large part because I like to see the code in CloudStack make calls into my storage plug-ins and monitor the behavior it executes on our SAN).
In the meanwhile, I can remove the commented-out code and work on other activities.
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217947984
@syed Thanks for the code review!
@swill I believe we are good to go on this PR.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546959
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -933,7 +937,7 @@ private PlugNicAnswer execute(PlugNicCommand cmd) {
*/
--- End diff --
Interesting...this code does not appear to be commented out in the most recent version of the code on master.
---
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: Notify listeners when a host has been add...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220516738
@mike-tutkowski No, I have not created ticket. You can create 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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366953
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -18,7 +18,7 @@
import javax.inject.Inject;
-import org.apache.log4j.Logger;
+// import org.apache.log4j.Logger;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62513240
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -1570,17 +1578,35 @@ private boolean checkCIDR(final HostPodVO pod, final String serverPrivateIP, fin
return true;
}
+ private HostVO isNewHost(StartupCommand[] startupCommands) {
--- End diff --
Rename to `getNewHost` ? `isNewHost` sounds like it should return a boolean
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217370720
Hi @mike-tutkowski,
I trust you either addressed all my previous remarks, or have a good reason not to.
I've skimmed (very quickly through the diff) and the code looks good, with small bits, didn't see any commented out code, plus you added integration tests: seems like a good PR!
So \U0001f44d
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140071482
Hi Miguel,
Fortunately this PR is such that there is essentially no existing test code that needed to be modified. At a high level, this code simply generates a few new events.
What I am doing is writing integration tests that I plan to run regularly on this (basically performing a lot of the test actions that I did manually in an automated fashion). However, this is part of a larger effort I have underway (where I expect to be automating a lot more than just this code).
Thanks for taking time to review the code!
Mike
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366920
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1981,7 +1981,7 @@ private void createDefaultEgressFirewallRule(final List<FirewallRule> rules, fin
// construct rule when egress policy is true. In true case for VR we default allow rule need to be added
if (!defaultEgressPolicy) {
- systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
+ // systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366926
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java ---
@@ -81,7 +81,7 @@ public boolean isRecurring() {
@Override
public boolean processAnswers(long agentId, long seq, Answer[] answers) {
List<Long> affectedVms = new ArrayList<Long>();
- int commandNum = 0;
+ // int commandNum = 0;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366816
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -4420,11 +4450,13 @@ private void fillHostDetailsInfo(VmwareContext serviceContext, Map<String, Strin
}
}
+ /*
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218228412
The one issue showing in the CI run is an issue with my environment. I think this one is ready to 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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505426
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
/**
+ * This method is called by AgentManager when a host is about to be removed from a cluster.
+ * @param long the ID of the newly added host
+ */
+ void processHostAboutToBeRemoved(long hostId);
+
+ /**
+ * This method is called by AgentManager when a host is removed from a cluster.
+ * @param long the ID of the newly added host
--- End diff --
typo?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218463825
@swill Is this claiming there's a license problem?
I can rebase and push again.
[INFO] Rat check: Summary of files. Unapproved: 1 unknown: 1 generated: 0 approved: 7204 licence.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 10.258 s
[INFO] Finished at: 2016-05-11T06:53:07+00:00
[INFO] Final Memory: 47M/366M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.10:check (default-cli) on project cloudstack: Too many files with unapproved license: 1 See RAT report in: /home/jenkins/jenkins-slave/workspace/cloudstack-pr-analysis/target/rat.txt -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62509667
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -18,40 +18,69 @@
*/
package org.apache.cloudstack.storage.datastore.provider;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
import javax.inject.Inject;
import org.apache.log4j.Logger;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener;
+import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.cloudstack.storage.datastore.util.SolidFireUtil;
import com.cloud.agent.AgentManager;
import com.cloud.agent.api.Answer;
import com.cloud.agent.api.ModifyStoragePoolAnswer;
import com.cloud.agent.api.ModifyStoragePoolCommand;
+import com.cloud.agent.api.ModifyTargetsCommand;
import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.dao.ClusterDao;
import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.StoragePool;
import com.cloud.storage.StoragePoolHostVO;
+import com.cloud.storage.VolumeVO;
import com.cloud.storage.dao.StoragePoolHostDao;
+import com.cloud.storage.dao.VolumeDao;
import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.dao.VMInstanceDao;
public class SolidFireHostListener implements HypervisorHostListener {
private static final Logger s_logger = Logger.getLogger(SolidFireHostListener.class);
- @Inject
- private AgentManager _agentMgr;
- @Inject
- private AlertManager _alertMgr;
- @Inject
- private DataStoreManager _dataStoreMgr;
- @Inject
- private HostDao _hostDao;
- @Inject
- private StoragePoolHostDao storagePoolHostDao;
+ @Inject private AgentManager _agentMgr;
+ @Inject private AlertManager _alertMgr;
+ @Inject private ClusterDao _clusterDao;
+ @Inject private ClusterDetailsDao _clusterDetailsDao;
+ @Inject private DataStoreManager _dataStoreMgr;
+ @Inject private HostDao _hostDao;
+ @Inject private PrimaryDataStoreDao _storagePoolDao;
+ @Inject private StoragePoolDetailsDao _storagePoolDetailsDao;
+ @Inject private StoragePoolHostDao storagePoolHostDao;
+ @Inject private VMInstanceDao _vmDao;
+ @Inject private VolumeDao _volumeDao;
+
+ @Override
+ public boolean hostAdded(long hostId) {
+ HostVO host = _hostDao.findById(hostId);
+
+ SolidFireUtil.hostAddedToOrRemovedFromCluster(hostId, host.getClusterId(), true, SolidFireUtil.PROVIDER_NAME,
+ _clusterDao, _clusterDetailsDao, _storagePoolDao, _storagePoolDetailsDao, _hostDao);
+
+ handleVMware(host, true);
--- End diff --
Is hostAdded only applicable to VMWare?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217018726
hi @miguelaferreira Yeah, after working on this PR last September, I got side tracked with other, higher-priority items and haven't gotten back to it until recently.
I've actually been writing several integration tests on and off over the past months. Are you thinking I can place them in /test/integration/plugins/solidfire? That works for me. Do you know if I will need to disable them somehow to avoid issues with CI? 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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-142081363
I understand.
I'm working on a task from last week and once I complete it I can take a stab at some until tests for this PR.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390601
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
}
private void createDefaultEgressFirewallRule(final List<FirewallRule> rules, final long networkId) {
- String systemRule = null;
+ // String systemRule = null;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable systemRule was not in use. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390851
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -18,7 +18,7 @@
import javax.inject.Inject;
-import org.apache.log4j.Logger;
+// import org.apache.log4j.Logger;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable s_logger was never used. Once I commented it out, Eclipse reminded me that its import statement was un-used. Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this import statement.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390674
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1981,7 +1981,7 @@ private void createDefaultEgressFirewallRule(final List<FirewallRule> rules, fin
// construct rule when egress policy is true. In true case for VR we default allow rule need to be added
if (!defaultEgressPolicy) {
- systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
+ // systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable systemRule was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505096
--- Diff: engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/HypervisorHostListener.java ---
@@ -21,7 +21,13 @@
import com.cloud.exception.StorageConflictException;
public interface HypervisorHostListener {
+ boolean hostAdded(long hostId);
+
boolean hostConnect(long hostId, long poolId) throws StorageConflictException;
boolean hostDisconnected(long hostId, long poolId);
+
+ boolean hostAboutToBeRemoved(long hostId);
--- End diff --
I am assuming this event is generated when moving to `Removing` state. Is there any action that is performed here which cannot be done when the host is in `Removed` state?
---
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.
---
Re: [GitHub] cloudstack pull request: Notify listeners when a host
has been add...
Posted by "Tutkowski, Mike" <Mi...@netapp.com>.
I see that you just sent an e-mail to @dev about this - thanks!
________________________________________
From: mike-tutkowski <gi...@git.apache.org>
Sent: Thursday, May 19, 2016 10:46 PM
To: dev@cloudstack.apache.org
Subject: [GitHub] cloudstack pull request: Notify listeners when a host has been add...
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220516362
Yes, I can send an e-mail out in a bit.
In the short term (at least for 4.9), I can just revert the changed names in those Command classes.
Do you have a ticket I can open the PR against?
Thanks!
________________________________
From: Anshul Gangwar <no...@github.com>
Sent: Thursday, May 19, 2016 10:30 PM
To: apache/cloudstack
Cc: Tutkowski, Mike; Mention
Subject: Re: [apache/cloudstack] Notify listeners when a host has been added to a cluster, is about to… (#816)
@mike-tutkowski<https://github.com/mike-tutkowski> then we can raise this in dev list.
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<https://github.com/apache/cloudstack/pull/816#issuecomment-220514841>
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220516362
Yes, I can send an e-mail out in a bit.
In the short term (at least for 4.9), I can just revert the changed names in those Command classes.
Do you have a ticket I can open the PR against?
Thanks!
________________________________
From: Anshul Gangwar <no...@github.com>
Sent: Thursday, May 19, 2016 10:30 PM
To: apache/cloudstack
Cc: Tutkowski, Mike; Mention
Subject: Re: [apache/cloudstack] Notify listeners when a host has been added to a cluster, is about to\u2026 (#816)
@mike-tutkowski<https://github.com/mike-tutkowski> then we can raise this in dev list.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<https://github.com/apache/cloudstack/pull/816#issuecomment-220514841>
---
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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218489553
I think we are definitely having issues with it not cleaning correctly. I have seen a couple cases where we get the "unable to update index" when it tries to do a git checkout because of problems with cleanup, so that is possible.
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547345
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -91,18 +280,5 @@ public boolean hostConnect(long hostId, long storagePoolId) {
assert (answer instanceof ModifyStoragePoolAnswer) : "ModifyStoragePoolAnswer expected ; Pool = " + storagePool.getId() + " Host = " + hostId;
--- End diff --
Some like they; some don't. I'd say feel free to put asserts in your code. I don't think the community has a formal policy on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218553761
@swill OK, looks like are OK here.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217091053
I think `/test/integration/plugins/solidfire` is a good place to keep the integration tests. I wouldn't disable or skip them because, IMH, it's not hard to select which tests you want to run anyway, and I rather have tests that break if something is not right. I do not some people have different opinions about this.
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217890036
@mike-tutkowski I've reviewed the code and it LGTM. I have a few minor comments that should be fairly simple to adress.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366916
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
}
private void createDefaultEgressFirewallRule(final List<FirewallRule> rules, final long networkId) {
- String systemRule = null;
+ // String systemRule = null;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366912
--- Diff: server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1589,8 +1589,8 @@ protected StringBuilder createGuestBootLoadArgs(final NicProfile guestNic, final
protected StringBuilder createRedundantRouterArgs(final NicProfile nic, final DomainRouterVO router) {
final StringBuilder buf = new StringBuilder();
- final long networkId = nic.getNetworkId();
- final NetworkVO network = _networkDao.findById(networkId);
+ // final long networkId = nic.getNetworkId();
--- End diff --
Why the commented out code?
(next line too)
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-139978868
Just an FYI that I have tested this with the two SolidFire storage plug-ins (where applicable) making use of the following hypervisor types and versions:
XenServer 6.1, 6.2, and 6.5
ESXi 5.1
KVM on Ubuntu 14.04
KVM on Ubuntu 12.04
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390723
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java ---
@@ -81,7 +81,7 @@ public boolean isRecurring() {
@Override
public boolean processAnswers(long agentId, long seq, Answer[] answers) {
List<Long> affectedVms = new ArrayList<Long>();
- int commandNum = 0;
+ // int commandNum = 0;
--- End diff --
While making changes in this file, I noticed that Eclipse was pointing out that the variable commandNum was only written to (never read from). Instead of removing the entire line, I just commented it out in case someone later wanted to easily resume making use of this variable.
---
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: Notify listeners when a host has been add...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366935
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java ---
@@ -106,7 +106,7 @@ public boolean processAnswers(long agentId, long seq, Answer[] answers) {
}
}
}
- commandNum++;
+ // commandNum++;
--- End diff --
Why the commented out code?
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140080679
Hi Miguel,
I can remove those commented-out lines. No problem. Some companies like such lines left in, but others - as you point out - like them removed and point to version control as sufficient documentation.
As you noted, there were some places where I simply didn't see any value in being able to easily reference that info anyways, so I removed those lines. I can just do the same for all of them.
I'm happy to add until tests, but it was not clear to me how to do so for this particular group of logic. Do you have any suggestions as to what specifically (and perhaps how) you'd like unit tested?
Initially I had mainly planned on doing end-to-end integration testing on this code (along with many other pieces of code).
Thanks!
Mike
---
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: Notify listeners when a host has been add...
Posted by mike-tutkowski <gi...@git.apache.org>.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216970070
@miguelaferreira I know it's been a while since you and I were talking about this. I attached a Marvin test for this, but I don't have a way to run it yet within the CloudStack community (it requires SolidFire hardware). I actually have a bunch of these tests. Hopefully when we move to more of a distributed CI environment, I can have these run automatically. Please let me know if we're good to go from your point of view now. 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: Notify listeners when a host has been add...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217892225
This has the required code reviews. @mike-tutkowski have a look at the comments @syed made. I will get this into my CI queue so we can get it moving forward. 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: Notify listeners when a host has been add...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-219949545
@koushik-das @swill @mike-tutkowski This has broken Hyper-V support. And this is happening because of variable renaming in command.
---
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: Notify listeners when a host has been add...
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62509977
--- Diff: plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java ---
@@ -65,22 +94,182 @@ public boolean hostConnect(long hostId, long storagePoolId) {
storagePoolHostDao.persist(storagePoolHost);
}
- // just want to send the ModifyStoragePoolCommand for KVM
- if (host.getHypervisorType() != HypervisorType.KVM) {
- return true;
+ if (host.getHypervisorType().equals(HypervisorType.XenServer)) {
+ handleXenServer(host.getClusterId(), host.getId(), storagePoolId);
+ }
+ else if (host.getHypervisorType().equals(HypervisorType.KVM)) {
+ handleKVM(hostId, storagePoolId);
+ }
+
+ return true;
+ }
+
+ @Override
+ public boolean hostDisconnected(long hostId, long storagePoolId) {
+ StoragePoolHostVO storagePoolHost = storagePoolHostDao.findByPoolHost(storagePoolId, hostId);
+
+ if (storagePoolHost != null) {
+ storagePoolHostDao.deleteStoragePoolHostDetails(hostId, storagePoolId);
+ }
+
+ return true;
+ }
+
+ @Override
+ public boolean hostAboutToBeRemoved(long hostId) {
+ HostVO host = _hostDao.findById(hostId);
+
+ handleVMware(host, false);
--- End diff --
Can this be done in `hostRemoved` ?
---
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.
---