Notify listeners when a host has been added to a cluster, is about to… #816
Conversation
|
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 |
| @@ -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()); | |||
There was a problem hiding this comment.
Why the commented out code?
There was a problem hiding this comment.
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.
|
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.) |
| @@ -124,6 +132,22 @@ | |||
| private final String _clusterAdminPassword; | |||
|
|
|||
| public SolidFireConnection(String managementVip, int managementPort, String clusterAdminUsername, String clusterAdminPassword) { | |||
There was a problem hiding this comment.
Shouldn't a IP be passed as a InetAddress inside the code?
There was a problem hiding this comment.
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.
|
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! |
|
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. 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. Fact is, that if we continue to add code without the complementary unit-tests, our test coverage drops. Is that something we want? |
|
cloudstack-pull-rats #591 ABORTED |
|
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! |
|
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. |
|
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! |
|
cloudstack-pull-analysis #527 ABORTED |
mike-tutkowski
force-pushed
the
addremovehosts2
branch
from
088eb45
to
c1ab439
Compare
|
Just an FYI that I went ahead and updated the code to remove the commented-out lines. |
|
cloudstack-pull-analysis #540 ABORTED |
|
cloudstack-pull-rats #605 ABORTED |
mike-tutkowski
force-pushed
the
addremovehosts2
branch
from
c1ab439
to
bca5bb3
Compare
|
cloudstack-pull-rats #614 SUCCESS |
|
cloudstack-pull-analysis #552 ABORTED |
mike-tutkowski
force-pushed
the
addremovehosts2
branch
from
bca5bb3
to
bf67ed8
Compare
mike-tutkowski
force-pushed
the
addremovehosts2
branch
2 times, most recently
from
5b42bdc
to
b879605
Compare
CI RESULTSSummary of the problem(s): Associated Uploads
Uploads will be available until Comment created by |
|
The one issue showing in the CI run is an issue with my environment. I think this one is ready to merge... |
|
Can you re-push to try to get Jenkins green. Thx... |
mike-tutkowski
force-pushed
the
addremovehosts2
branch
from
b879605
to
73b12fb
Compare
|
@swill Done. Hopefully it will work for Jenkins this time. |
|
@mike-tutkowski unfortunately it failed again. :( |
|
@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. |
|
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. |
…ster, is about to be removed from a cluster, or has been removed from a cluster
mike-tutkowski
force-pushed
the
addremovehosts2
branch
from
73b12fb
to
dad9e5d
Compare
|
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. |
|
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. |
|
@swill OK, looks like are OK here. |
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. * pr/816: CLOUDSTACK-8813: 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 Signed-off-by: Will Stevens <[email protected]>
|
@koushik-das @swill @mike-tutkowski This has broken Hyper-V support. And this is happening because of variable renaming in command. Should we even allow renaming of variable in the first place? Specifically which are used by other components. |
|
Hi @anshul1886 Can you be more specific about the class that is given you problems and the variables? Thanks! |
|
@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. |
|
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 <[email protected]mailto:[email protected]> wrote: @mike-tutkowskihttps://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. — |
|
@mike-tutkowski I believe class name ending in Command signifies that they are meant for agent and should be treated like API params. |
|
I wonder how many people are actually aware of that. :-) Sent from my iPhone On May 19, 2016, at 9:04 PM, Anshul Gangwar <[email protected]mailto:[email protected]> wrote: @mike-tutkowskihttps://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. — |
|
@mike-tutkowski then we can raise this in dev list. |
|
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 [email protected] @mike-tutkowskihttps://github.com/mike-tutkowski then we can raise this in dev list. — |
|
"Tutkowski, Mike" on [email protected] replies: |
|
@mike-tutkowski No, I have not created ticket. You can create one. |
|
?OK, I can create one and open a PR up against it reverting those variable names. From: Anshul Gangwar [email protected] @mike-tutkowskihttps://github.com/mike-tutkowski No, I have not created ticket. You can create one. — |
… 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.