You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/07/04 16:08:57 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8607 - As an Operator I want t...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/559
CLOUDSTACK-8607 - As an Operator I want to be able to change the host password on the host itself via the updateHostPassword API
The changes in this PR will cover the following:
- Make sure the new password replaces the old one in the queue
- Updated the patch files for XenServer
- Updated the script path on LibvirtComputing class
- Adding update_host_passwd to VRScripts
- Add implementation to CitrixUpdateHostPasswordCommandWrapper
- Improve testUpdateHostPasswordCommand() unit test on CitrixRequestWrapperTest
- Adding update_host_passwd.sh script
- Adding the host IP address as an instance variable on UpdateHostPasswordCommand
- Improving the Unit Test (LibvirtComputingResourceTest) to get it covering the new code
- Make sure doUpdateHostPassword() doesn't get called if flag is set to false
- Do not update XenServer hosts if the cluster ID is not informed
Test Environment:
XenServer cluster: 2 hosts
1 KVM host
Management Server running on CentOS 7.1
Shared Storage
Cloudmonkey
:: Update Single Xen Host ::
update hostpassword hostid=ce516ca6-9294-46c8-995c-406672aecf3f username=root password=admin update_passwd_on_host=true
Result: Error 431: Single host update is not supported by XenServer hypervisors. Please try again informing the Cluster ID.
:: Update Xen Cluster ::
update hostpassword clusterid=479cd3f1-2f94-4ae9-b1bb-08be1da1b460 username=root update_passwd_on_host=true password=admin
Result: success = true
Successfully SSH into both hosts with new password
:: Update Single KVM Host ::
update hostpassword hostid=5683cf21-6cb0-4ed0-8498-f77411e89e9a username=root update_passwd_on_host=true password=admin
Result: success = true
Successfully SSH into KVM host with new password
I also reverted the passwords of the hosts to the original, all worked fine.
The "update_passwd_on_host" parameter is not required and is defaulted to "false", which means if operators want to proceed as before, updating only the database and the hosts manually that will be fine.
In case we face problems with the DB update, nothing will change - not even the hosts. In case we are updating a host and it fails due to connection problem, for example, the DB changes will not be rolled back. That's not okay, but I want to fix the consistence in a separate issue, since it would be out of scope for our sprint - which finished next week.
@remibergsma: could you have a look at that before you go on holidays? :)
@DaanHoogland, @karuturi and @bhaisaab, your comments are also appreciated
Thanks in advance.
Cheers,
Wilder
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/schubergphilis/cloudstack improvement/CLOUDSTACK-8607
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/559.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 #559
----
commit a74971df06e04ee2df993a9876bb274d58c9662d
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-02T09:12:08Z
CLOUDSTACK-8607 - Adding shouldUpdateHost flag
- Make sure doUpdateHostPassword() doesn't get called if flag is set to false
- Do not update XenServer hosts if the cluster ID is not informed
commit 47c7a1083fc4be9e74cb3fd73fce069c5e85c1cc
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-02T12:54:51Z
CLOUDSTACK-8607 - Adding update_host_passwd.sh script
- Modifying the LibvirtUpdateHostPasswordCommandWrapper in order to execute the script on the host
- Adding the script path to LibvirtComputingResource
- Adding the host IP address as an instance variable on UpdateHostPasswordCommand
- Improving the Unit Test (LibvirtComputingResourceTest) to get it covering the new code
commit 0dd02ce04341b8026f49fd84be01bfcb7f18bf3d
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-03T08:24:44Z
CLOUDSTACK-8607 - Adding support to update host passwd on XenServer hypervisors
- Adding update_host_passwd to VRScripts
- Add accessor method to host password on CitrixResourceBase
- Add implementation to CitrixUpdateHostPasswordCommandWrapper
- Improve testUpdateHostPasswordCommand() unit test on CitrixRequestWrapperTest
- Add line to patch files on xenserver directory
Concerning the LibVirt change:
- I forgot to assing the return of the getDefaultHypervisorScriptsDir() method to the hypervisorScriptsDir variable
commit 6c92ccf8d1ef5b824d6e36ea727bbebf05a95001
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-03T09:43:56Z
CLOUDSTACK-8607 - Refactoring attribute name
- Refactoring attribute name from shouldUpdateHost to updatePasswdOnHost
- Fixing ApiConstants class because it had an error in the constant name
commit efa34361df72e351cdc1f719a5dba705eab771ef
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-03T12:29:57Z
CLOUDSTACK-8607 - Changed update script to return exit code based on the result
- Changed location of the update_host_passwd script
- Updated the patch files for XenServer
- Updated the script path on LibvirtComputing class
- Removed the hostIP from the LibvirtUpdateHostPasswordCommandWrapper execute() method
commit 86297e70be8cd587ab07ba3109042ea782772624
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-04T09:33:52Z
CLOUDSTACK-8607 - Make sure the new password replaces the old one in the queue
- Added log info to show details of the operation
- Renamed the addPwdToQueue to replaceOldPasswdInQueue
----
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/559#issuecomment-118798645
@wilderrodrigues Thanks for the detailed description in the pull request. It really helps :)
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/559#discussion_r33922730
--- Diff: plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java ---
@@ -1320,14 +1322,23 @@ public void testOvsDestroyTunnelCommandFailed() {
@Test
public void testUpdateHostPasswordCommand() {
- final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123");
+ final ExecutionResult executionResult = Mockito.mock(ExecutionResult.class);
+
+ final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123", "127.0.0.1");
+
+ final StringBuffer buff = new StringBuffer();
+ buff.append(updatePwd.getUsername());
+ buff.append(' ');
+ buff.append(updatePwd.getNewPassword());
+
+ when(citrixResourceBase.executeInVR(updatePwd.getHostIp(), VRScripts.UPDATE_HOST_PASSWD, buff.toString())).thenReturn(executionResult);
--- End diff --
Hi @karuturi
The tests is not checking that we can SSH into the host itself, but just covering the flow we have with the new wrappers. That's why we have the Mock of ExecutionResult there.
The unit tests is covering the call CitrixRequestWrapper.execute() when a UpdateHostPasswordCommand is given, not the SSH connection itself. The latter should be done separately.
Thanks for the reivew.
Cheers,
Wilder
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/559#discussion_r33921332
--- Diff: plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java ---
@@ -1320,14 +1322,23 @@ public void testOvsDestroyTunnelCommandFailed() {
@Test
public void testUpdateHostPasswordCommand() {
- final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123");
+ final ExecutionResult executionResult = Mockito.mock(ExecutionResult.class);
+
+ final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123", "127.0.0.1");
+
+ final StringBuffer buff = new StringBuffer();
+ buff.append(updatePwd.getUsername());
+ buff.append(' ');
+ buff.append(updatePwd.getNewPassword());
+
+ when(citrixResourceBase.executeInVR(updatePwd.getHostIp(), VRScripts.UPDATE_HOST_PASSWD, buff.toString())).thenReturn(executionResult);
--- End diff --
Why do we need this mock here?
The test always succeeds as the SshHelper.execute fails with "java.io.IOException: There was a problem while connecting to 127.0.0.1:22"
If I add the below line to test, it always fails
verify(citrixResourceBase, Mockito.atLeastOnce()).executeInVR(Mockito.anyString(),Mockito.anyString(), Mockito.anyString());`
Wanted but not invoked:
`citrixResourceBase.executeInVR(
<any>,
<any>,
<any>
);`
-> at `com.cloud.hypervisor.xenserver.resource.wrapper.xenbase.CitrixRequestWrapperTest.testUpdateHostPasswordCommand(CitrixRequestWrapperTest.java:1341)`
However, there were other interactions with this mock:
-> at `com.cloud.hypervisor.xenserver.resource.wrapper.xenbase.CitrixUpdateHostPasswordCommandWrapper.execute(CitrixUpdateHostPasswordCommandWrapper.java:57)`
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/559#discussion_r33923859
--- Diff: plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java ---
@@ -1320,14 +1322,23 @@ public void testOvsDestroyTunnelCommandFailed() {
@Test
public void testUpdateHostPasswordCommand() {
- final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123");
+ final ExecutionResult executionResult = Mockito.mock(ExecutionResult.class);
+
+ final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123", "127.0.0.1");
+
+ final StringBuffer buff = new StringBuffer();
+ buff.append(updatePwd.getUsername());
+ buff.append(' ');
+ buff.append(updatePwd.getNewPassword());
+
+ when(citrixResourceBase.executeInVR(updatePwd.getHostIp(), VRScripts.UPDATE_HOST_PASSWD, buff.toString())).thenReturn(executionResult);
--- End diff --
Agreed on the intention of the test. I think the below interaction for executeInVR is not needed (as it is never called)
`when(citrixResourceBase.executeInVR(updatePwd.getHostIp(), VRScripts.UPDATE_HOST_PASSWD, buff.toString())).thenReturn(executionResult);`
Instead if we could mock `SshHelper.sshExecute()` and test both for success and failure of the sshExecute, that would cover all the branches.
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/559#issuecomment-118848092
Hi @DaanHoogland and @karuturi
I did some refactoring and added more tests in order to cover both success/failure cases.
Thanks for the feedback.
Cheers,
Wilder
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/559#issuecomment-118868287
LGTM
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/559#discussion_r33925325
--- Diff: plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java ---
@@ -1320,14 +1322,23 @@ public void testOvsDestroyTunnelCommandFailed() {
@Test
public void testUpdateHostPasswordCommand() {
- final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123");
+ final ExecutionResult executionResult = Mockito.mock(ExecutionResult.class);
+
+ final UpdateHostPasswordCommand updatePwd = new UpdateHostPasswordCommand("test", "123", "127.0.0.1");
+
+ final StringBuffer buff = new StringBuffer();
+ buff.append(updatePwd.getUsername());
+ buff.append(' ');
+ buff.append(updatePwd.getNewPassword());
+
+ when(citrixResourceBase.executeInVR(updatePwd.getHostIp(), VRScripts.UPDATE_HOST_PASSWD, buff.toString())).thenReturn(executionResult);
--- End diff --
That's what I was discussing with @DaanHoogland here. In order to make it better, we have to extract the Script creation into the existing utilities class and mock it. So we can cover both false/true cases.
But wait, whilst reading both of your comments again, I realised that I was looking into the wrong test file. It might have been result of a initial version of the test. The mock + the when() call should not be there at all.
That's how I will proceed:
1. remove the unused Mock/call
2. extract the SshHelper so we can cover both scenarios
3. apply the similar changes to LibvirtUpdateHostPasswordCommandWrapper
Sorry for the misunderstanding.
Cheers,
Wilder
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/559#issuecomment-118516112
Hi @wilderrodrigues awesome work, thanks! LGTM
(let's wait for Travis now to be sure...)
---
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: CLOUDSTACK-8607 - As an Operator I want t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/559
---
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.
---