You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nvazquez <gi...@git.apache.org> on 2016/08/24 12:36:57 UTC
[GitHub] cloudstack pull request #1660: CLOUDSTACK-9470: [BLOCKER] Fix for test_netwo...
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1660
CLOUDSTACK-9470: [BLOCKER] Fix for test_network_acl, SshHelper
## Problem
In a Vmware environment, running `test_network_acl` we examined why sometimes it failed and sometimes it passed. It was discovered that issue was on `VmwareResource`:
```java
private int findRouterEthDeviceIndex(String domrName, String routerIp, String mac) throws Exception {
VmwareManager mgr = getServiceContext().getStockObject(VmwareManager.CONTEXT_STOCK_NAME);
s_logger.info("findRouterEthDeviceIndex. mac: " + mac);
// TODO : this is a temporary very inefficient solution, will refactor it later
Pair<Boolean, String> result = SshHelper.sshExecute(routerIp, DefaultDomRSshPort, "root", mgr.getSystemVMKeyFile(), null, "ls /proc/sys/net/ipv4/conf");
...
}
```
Command sent to router returned a pair which its first parameter was always true but second parameter sometimes it was null and sometimes the correct output for given command.
## Solution
Examining `SshHelper` we discovered that cases in which second parameter in result was null, didn't consume stdout properly
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nvazquez/cloudstack sshhelperissue
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1660.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 #1660
----
commit ade0a795e51bdceb65311d9e8bcc81853335ffe5
Author: nvazquez <ni...@gmail.com>
Date: 2016-08-24T12:32:55Z
CLOUDSTACK-9470: Fix for SshHelper
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell that is great; probably I have just read half of an email thread.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@nvazquez could you please amend your commit message to explain why/how this fix addresses the defect?
Also, verification of this PR is to run ``test_network_acl`` against VMware, correct?
@borisstoyanov do you have time to run tests 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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Great! Thanks guys!
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76517786
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -195,6 +195,15 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
}
String result = sbResult.toString();
+ if (result == null || (result != null && result.isEmpty())){
+ int stdOutAvailable = stdout.available();
+ while (stdOutAvailable != 0){
+ currentReadBytes = stdout.read(buffer);
+ sbResult.append(new String(buffer, 0, currentReadBytes));
+ stdOutAvailable = stdout.available();
+ }
+ result = sbResult.toString();
--- End diff --
Great, didn't know about those utils, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Thanks @rhtyd!
I rebased branch '4.9' and pushed some changes based on your last comment. I had to change visibility of `com.cloud.utils.stringUtils.UTF8` from private to public.
I also included catch for IOException, I think that case should return false as the first parameter, do you agree?
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76476168
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -195,6 +195,15 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
}
String result = sbResult.toString();
+ if (result == null || (result != null && result.isEmpty())){
--- End diff --
Consider using ``StringUtils.isBlank`` which will not only make the code more expressive, but also covers the case where the string is composed only of whitespace.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell With this PR test_network_acl is successfull
[root@ussarlabcsmgt41 Aug_23_2016_16_49_32_8HYYKG]# tail results.txt
Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : SUCCESS ===
ok
Ran 1 test in 211.968s
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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Fix for test_network_acl,...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
LGTM for the test.
Exception in network_acl_test **before the fix**
[root@ussarlabcsmgt41 cloudstack]# cat /tmp//MarvinLogs//NB313W/results.txt
Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : EXCEPTION ===
ERROR
======================================================================
ERROR: Test network ACL lists and items in VPC
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/t_vazqnadmin/cloudstack/test/integration/smoke/exclude/test_network_acl.py", line 81, in test_network_acl
domainid=self.domain.id
File "/usr/local/lib/python2.7/site-packages/marvin/lib/base.py", line 4311, in create
return VPC(apiclient.createVPC(cmd).__dict__)
File "/usr/local/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 2210, in createVPC
response = self.connection.marvinRequest(command, response_type=response, method=method)
File "/usr/local/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest
raise e
Exception: Job failed: {jobprocstatus : 0, created : u'2016-08-24T05:52:23-0700', jobresult : {errorcode : 530, errortext : u'Failed to create VPC'}, cmd : u'org.apache.cloudstack.api.command.admin.vpc.CreateVPCCmdByAdmin', userid : u'd557133a-3fcc-11e5-9c45-005056ad45b7', jobstatus : 2, jobid : u'bc8fb5de-94de-43bf-befd-a2b450ab571c', jobresultcode : 530, jobresulttype : u'object', jobinstancetype : u'None', accountid : u'd556f1dc-3fcc-11e5-9c45-005056ad45b7'}
-------------------- >> begin captured stdout << ---------------------
=== TestName: test_network_acl | Status : EXCEPTION ===
--------------------- >> end captured stdout << ----------------------
..... (NOT INCLUDED)
-------------------- >> begin captured logging << --------------------
--------------------- >> end captured logging << ---------------------
----------------------------------------------------------------------
Ran 1 test in 115.185s
FAILED (errors=1)
**After fix is deployed:**
[root@ussarlabcsmgt41 Aug_23_2016_16_49_32_8HYYKG]# tail results.txt
Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 211.968s
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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Thanks, this looks interesting. I'll try to get back to you 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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76532860
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -196,6 +198,9 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
String result = sbResult.toString();
+ if (StringUtils.isBlank(result))
+ result = IOUtils.toString(stdout);
--- End diff --
Done, thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@rhtyd @jburwell Just re-ran Marvin tests. All passes including test_network_acl which fails without this fix.
[root@ussarlabcsmgt41 ~]# cat /tmp//MarvinLogs/test_volumes_939G6N/results.txt |grep -v '^ok'
test DeployVM in anti-affinity groups for project ... === TestName: test_DeployVmAntiAffinityGroup_in_project | Status : SUCCESS ===
test DeployVM in anti-affinity groups ... === TestName: test_DeployVmAntiAffinityGroup | Status : SUCCESS ===
Test Deploy Virtual Machine ... SKIP: Skipping test because suitable hypervisor/host not present
Test Deploy Virtual Machine from ISO ... === TestName: test_deploy_vm_from_iso | Status : SUCCESS ===
Test deploy virtual machine with root resize ... === TestName: test_00_deploy_vm_root_resize | Status : SUCCESS ===
Test proper failure to deploy virtual machine with rootdisksize of 0 ... === TestName: test_01_deploy_vm_root_resize | Status : SUCCESS ===
Test proper failure to deploy virtual machine with rootdisksize less than template size ... === TestName: test_02_deploy_vm_root_resize | Status : SUCCESS ===
Test to deploy vm with a first fit offering ... === TestName: test_deployvm_firstfit | Status : SUCCESS ===
Test deploy VMs using user concentrated planner ... === TestName: test_deployvm_userconcentrated | Status : SUCCESS ===
Test deploy VMs using user dispersion planner ... === TestName: test_deployvm_userdispersing | Status : SUCCESS ===
Test userdata as GET, size > 2k ... === TestName: test_deployvm_userdata | Status : SUCCESS ===
Test userdata as POST, size > 2k ... === TestName: test_deployvm_userdata_post | Status : SUCCESS ===
Test to create disk offering ... === TestName: test_01_create_disk_offering | Status : SUCCESS ===
Test to create a sparse type disk offering ... === TestName: test_02_create_sparse_type_disk_offering | Status : SUCCESS ===
Test to create a sparse type disk offering ... === TestName: test_04_create_fat_type_disk_offering | Status : SUCCESS ===
Test to update existing disk offering ... === TestName: test_02_edit_disk_offering | Status : SUCCESS ===
Test to delete disk offering ... === TestName: test_03_delete_disk_offering | Status : SUCCESS ===
Test to ensure 4 default roles cannot be deleted ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test to check role, role permissions and account life cycles ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test for role-rule enforcement in case of multiple mgmt servers ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Test to ensure role in use cannot be deleted ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests normal lifecycle operations for roles ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests that default four roles exist ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests role update when role is in use by an account ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests concurrent order updation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests creation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests deletion of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests listing of default role's permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
Tests order updation of role permission ... SKIP: Dynamic Role-Based API checker not enabled, skipping test
test update configuration setting at zone level scope ... === TestName: test_UpdateConfigParamWithScope | Status : SUCCESS ===
Test guest vlan range dedication ... === TestName: test_dedicateGuestVlanRange | Status : SUCCESS ===
Test create public & private ISO ... === TestName: test_01_create_iso | Status : SUCCESS ===
Test Edit ISO ... === TestName: test_02_edit_iso | Status : SUCCESS ===
Test delete ISO ... === TestName: test_03_delete_iso | Status : SUCCESS ===
Test for extract ISO ... === TestName: test_04_extract_Iso | Status : SUCCESS ===
Update & Test for ISO permissions ... === TestName: test_05_iso_permissions | Status : SUCCESS ===
Test for copy ISO from one zone to another ... SKIP: Not enough zones available to perform copy template
Test delete ISO ... === TestName: test_07_list_default_iso | Status : SUCCESS ===
Test listing Volumes using 'ids' parameter ... === TestName: test_01_list_volumes | Status : SUCCESS ===
Test listing Templates using 'ids' parameter ... === TestName: test_02_list_templates | Status : SUCCESS ===
Test listing Snapshots using 'ids' parameter ... === TestName: test_03_list_snapshots | Status : SUCCESS ===
Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
Tests that SAML users are not allowed CloudStack local log in ... === TestName: login_test_saml_user | Status : SUCCESS ===
test_nic_secondaryip_add_remove (integration.smoke.test_multipleips_per_nic.TestDeployVM) ... === TestName: test_nic_secondaryip_add_remove | Status : SUCCESS ===
**## Test network ACL lists and items in VPC ... === TestName: test_network_acl | Status : SUCCESS ===**
# 1. Register a template for VMware with nicAdapter vmxnet3 ... SKIP: VCenter API Integration Remaining
Test to add and update added nic to a virtual machine ... === TestName: test_01_nic | Status : SUCCESS ===
Test to update a physical network and extend its vlan ... === TestName: test_extendPhysicalNetworkVlan | Status : SUCCESS ===
test update configuration setting at storage scope ... === TestName: test_UpdateStorageOverProvisioningFactor | Status : SUCCESS ===
test_create_pvlan_network (integration.smoke.test_pvlan.TestPVLAN) ... === TestName: test_create_pvlan_network | Status : SUCCESS ===
Test for create region ... === TestName: test_createRegion | Status : SUCCESS ===
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
Test volume detail ... === TestName: test_01_updatevolumedetail | Status : SUCCESS ===
Test scale virtual machine ... SKIP: Skipping scale VM operation because VMware tools are not installed on the VM
Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
Test to change service to a small capacity ... === TestName: test_04_change_offering_small | Status : SUCCESS ===
Test List secondary storage VMs ... === TestName: test_01_list_sec_storage_vm | Status : SUCCESS ===
Test List console proxy VMs ... === TestName: test_02_list_cpvm_vm | Status : SUCCESS ===
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
Tests allowed APIs for common account types ... === TestName: test_static_role_account_acls | Status : SUCCESS ===
Test create public & private template ... === TestName: test_01_create_template | Status : SUCCESS ===
Test when createTemplate is used to create templates having the same name all of them get ... === TestName: test_CreateTemplateWithDuplicateName | Status : SUCCESS ===
Test Edit template ... === TestName: test_02_edit_template | Status : SUCCESS ===
Test delete template ... === TestName: test_03_delete_template | Status : SUCCESS ===
Test for extract template ... === TestName: test_04_extract_template | Status : SUCCESS ===
Update & Test for template permissions ... === TestName: test_05_template_permissions | Status : SUCCESS ===
Test for copy template from one zone to another ... SKIP: Not enough zones available to perform copy template
Test only public templates are visible to normal user ... === TestName: test_07_list_public_templates | Status : SUCCESS ===
Test System templates are not visible to normal user ... === TestName: test_08_list_system_templates | Status : SUCCESS ===
Check events in usage_events table when VM creation fails ... === TestName: test_01_positive_tests_usage | Status : SUCCESS ===
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
Tests for basic zone virtual router ... === TestName: test_basicZoneVirtualRouter | Status : SUCCESS ===
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
Test for attach and detach ISO to virtual machine ... === TestName: test_10_attachAndDetach_iso | Status : SUCCESS ===
Test Volume creation for all Disk Offerings (incl. custom) ... === TestName: test_01_create_volume | Status : SUCCESS ===
Attach a created Volume to a Running VM ... === TestName: test_02_attach_volume | Status : SUCCESS ===
Download a Volume attached to a VM ... === TestName: test_03_download_attached_volume | Status : SUCCESS ===
Delete a Volume attached to a VM ... === TestName: test_04_delete_attached_volume | Status : SUCCESS ===
Detach a Volume attached to a VM ... === TestName: test_05_detach_volume | Status : SUCCESS ===
Download a Volume unattached to an VM ... === TestName: test_06_download_detached_volume | Status : SUCCESS ===
Test resize (negative) non-existent volume ... SKIP: Resize Volume is unsupported on VmWare and Hyper-V
Test resize a volume ... SKIP: Resize Volume is unsupported on VmWare and Hyper-V
Delete a Volume unattached to an VM ... === TestName: test_09_delete_detached_volume | Status : SUCCESS ===
----------------------------------------------------------------------
Ran 104 tests in 9303.703s
OK (SKIP=21)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@nvazquez The Travis build has failed. Can you investigate? Likely just needs to be re-kicked.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
I looked at the class, while it can have further refactoring improvements, fwiw LGTM. /cc @jburwell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@nvazquez the Jenkins build failed. Could you please investigate and fix any issues?
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r77285299
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -196,6 +201,16 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
String result = sbResult.toString();
+ if (StringUtils.isBlank(result)) {
+ try {
+ result = IOUtils.toString(stdout, UTF8);
--- End diff --
Rather than use a homegrown string constant, pass ``StandardCharsets.UTF_8`` instead. An additional benefit is that it is a ``Charset`` instance which provides greater type safety.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Thanks @rhtyd @jburwell for reviewing!
@jburwell sure, now PR is targeted to 4.9 branch. Jenkins hadn't started properly last time and failed, with last refactor it started properly. I also changed commit message, I think it explains the fix, but if not please let me know and I'll change 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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76517779
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -195,6 +195,15 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
}
String result = sbResult.toString();
+ if (result == null || (result != null && result.isEmpty())){
--- End diff --
Done, 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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76731318
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -196,6 +198,10 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
String result = sbResult.toString();
+ if (StringUtils.isBlank(result)) {
+ result = IOUtils.toString(stdout);
--- End diff --
Add the default utf8 encoding, what if this throws an exception (IOException)?
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76530524
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -196,6 +198,9 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
String result = sbResult.toString();
+ if (StringUtils.isBlank(result))
+ result = IOUtils.toString(stdout);
--- End diff --
Per our coding standards, all conditionals must wrapped in curly braces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell Commit message amended. Is it ok like this? Yes, it should be ran against VMware.
We also made some individual tests for `SshHelper.sshExecute` by adding a test API method which received an host ip and a command to execute on that host, and passed them to `SshHelper.sshExecute`.
@serg38 posted results of this tests on [this comment](https://github.com/apache/cloudstack/pull/1660#issuecomment-242061367)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Fix for test_network_acl,...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Also we wrote a tester for SshHelper.sshExecute method. It was returning output of SSHexecute command as it gets received by sshExecute. Here they are 100 sequential runs (success if result is 100) :
Before the fix:
[root@ussarlabcsmgt41 t_vazqnadmin]# for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=echo%20success" 2>/dev/null|head -1|grep success;done|wc -l
## **58**
[root@ussarlabcsmgt41 t_vazqnadmin]# for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=pwd" 2>/dev/null|head -1|grep '/root';done|wc -l
## **49**
[root@ussarlabcsmgt41 t_vazqnadmin]# for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=ls%20/proc/sys/net/ipv4/conf" 2>/dev/null|grep eth;done|wc -l
## **0**
After the fix:
[root@ussarlabcsmgt41 t_vazqnadmin]# for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=echo%20success" 2>/dev/null|head -1|grep success;done|wc -l
## **100**
[levitssadmin@ussclpdadubh001 ~]$ for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=pwd" 2>/dev/null|head -1|grep '/root';done|wc -l
## **100**
[root@ussarlabcsmgt41 t_vazqnadmin]# for i in `seq 1 100`;do curl "http://10.144.31.41:8096/client/api?command=testNicoSSHCommand&hostip=10.140.32.220&sshcommand=ls%20/proc/sys/net/ipv4/conf" 2>/dev/null|grep eth0;done|wc -l
## **100**
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@rafaelweingartner . Thanks. I believe per current process any committer can merge PRs as per below. We have so many PRs in merge ready state....
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1642
@mike-tukowski any committer may merge a PR with at least one code review LGTM, at least one test LGTM, and no -1s. For this PR, I don't see a test LGTM with test results. Until it has at least one test LGTM, it is *not* 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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Minor improvements suggested, 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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@serg38 I will be going through other PRs tomorrow that are due for 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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell now both tests passed!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
LGTM thanks @nvazquez for the change. Can you help with testing, unfortunately I don't have the resources (some infra issues) to run tests across vmware and xenserver.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@rhtyd @jburwell @swill @koushik-das @rafaelweingartner @wido This PR has enough of everything. Can one of the committers merge 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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r76477326
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -195,6 +195,15 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
}
String result = sbResult.toString();
+ if (result == null || (result != null && result.isEmpty())){
+ int stdOutAvailable = stdout.available();
+ while (stdOutAvailable != 0){
+ currentReadBytes = stdout.read(buffer);
+ sbResult.append(new String(buffer, 0, currentReadBytes));
+ stdOutAvailable = stdout.available();
+ }
+ result = sbResult.toString();
--- End diff --
Consider replacing lines 199-205 with the following:
```
result = IOUtils.toString(buffer, Charset.defaultCharset());
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Hi @rhtyd, it would be nice to include it in 4.9. I tried rebasing branch 4.9 but it tries to update poms to 4.10 version, what do you suggest me to do?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@rafaelweingartner @serg38 is correct. Any committer may merge a PR so long as the following conditions are met:
* At least 1 code review LGTM
* At least 1 test LGTM
* No -1s
I am merging this PR now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell Thanks a lot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
RMs please help review /cc @jburwell @karuturi
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@rhtyd @jburwell Can this be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@serg38 I do not know how it is working right now this process; but, I recall seeing an email saying that only RMs would be allowed to merge now. I might be mistaken, I had lost track of that thread, but just in case that is why I am not going forward right now and merging this PR. The same applies for your other PRs. Let\u2019s see what others have to add on this 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 issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1660
Done, thanks @rhtyd for your help, it was really useful!
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1660
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@nvazquez @serg38 does this also affect 4.9, if so please edit the base branch to 4.9. We should have this in 4.9/lts release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@jburwell I don't have a VMware setup, maybe when we got Trillian back I could have a look.
---
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 #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper a...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1660#discussion_r77363519
--- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java ---
@@ -196,6 +201,16 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
String result = sbResult.toString();
+ if (StringUtils.isBlank(result)) {
+ try {
+ result = IOUtils.toString(stdout, UTF8);
--- End diff --
Done, thanks @jburwell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1660: CLOUDSTACK-9470: [BLOCKER] Bug in SshHelper affectin...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1660
@nvazquez okay, there are many ways to do this. Here's how you can do it fairly easily without merge conflicts:
git checkout <your PR branch>
git format-patch -1 # this will create a .patch file of your commit
git fetch origin # note origin points to asf or apache/cloudstack github remote
git reset --hard origin/4.9 # this will move your branch's HEAD to origin/4.9 i.e. your branch is now exactly origin/4.9
git am <patch file you created above>
git push <your own remote you used to send this PR> sshhelperissue -fv # this will force push and update your remote branch and the commit on this PR
Hope this 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.
---