Remote Diagnostics API #13

dhlaluku · 2018-05-24T11:46:41Z

Description

This a new feature for CloudStack to provide functionality for the admin user to execute network-utility commands (ping, traceroute, arping) directly on the system VMs. The initial scope is for a Root Admin using this tools with VR, SSVM and the CPVM.

FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+Remote+Diagnostics+API

Types of changes

New feature (a non-breaking change which adds functionality)

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration test suite with all test that can run on my environment has passed.

DaanHoogland · 2018-05-24T11:48:14Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosisServiceImpl.java

+        command.setAccessDetail(NetworkElementCommand.ROUTER_IP, routerControlHelper.getRouterControlIp(router.getId()));
+        command.setAccessDetail(NetworkElementCommand.ROUTER_NAME, router.getInstanceName());
+
+//        String routerAccessIP = command.getRouterAccessIp();


please remove commented code

DaanHoogland · 2018-05-24T11:50:02Z

core/src/main/java/org/apache/cloudstack/diagnosis/ExecuteDiagnosisCommand.java

@@ -0,0 +1,43 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


double comment ;)

DaanHoogland · 2018-05-24T11:50:21Z

core/src/main/java/org/apache/cloudstack/diagnosis/ExecuteDiagnosisAnswer.java

@@ -0,0 +1,29 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


nested comment

DaanHoogland · 2018-05-24T11:55:15Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnosis/RemoteDiagnosisCmd.java

@@ -0,0 +1,114 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


nested comment

DaanHoogland · 2018-05-24T11:56:19Z

api/src/main/java/org/apache/cloudstack/api/response/RemoteDiagnosisResponse.java

@@ -0,0 +1,53 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


DaanHoogland · 2018-05-24T11:56:38Z

api/src/main/java/org/apache/cloudstack/diangosis/RemoteDiagnosisService.java

@@ -0,0 +1,30 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


DaanHoogland · 2018-05-24T11:58:19Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosisServiceImpl.java

@@ -0,0 +1,115 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


DaanHoogland · 2018-05-24T12:15:29Z

systemvm/debian/opt/cloud/bin/ping_remotely.sh

+# specific language governing permissions and limitations
+# under the License.
+
+$1 $2 -c 5


so is this inteded for more than just 'ping'? passing $1 seems like it is intended but it is senseless given the script name 'ping_remotely.sh'

DaanHoogland · 2018-05-24T12:18:47Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosisServiceImpl.java

+        if (origAnswer instanceof ExecuteDiagnosisAnswer) {
+            answer = (ExecuteDiagnosisAnswer) origAnswer;
+        } else {
+            s_logger.warn("Unable to update router " + router.getHostName() + "status");


c&p? I would expect amessage about remote execution

DaanHoogland

as a user of this feature, ... ;)

DaanHoogland · 2018-06-01T08:13:42Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    //////////////// API parameters /////////////////////
+    /////////////////////////////////////////////////////
+    @Parameter(name = ApiConstants.ID,
+            description = "The ID of the System VM instance",


"... to diagnose" ?

DaanHoogland · 2018-06-01T08:14:42Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    private Long id;
+
+    @Parameter(name = ApiConstants.IP_ADDRESS,
+            description = "Destination IP address to ping",


"Destination address IP to test conection to"

DaanHoogland · 2018-06-01T08:15:21Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+            description = "The type of command to be executed inside the System VM instance, e.g. ping, tracert or arping",
+            required = true,
+            type = CommandType.STRING)
+    private String type;


how are these validated?

Using Preconditions with an enum that throws an IllegalArgument exception.

DaanHoogland · 2018-06-01T08:17:07Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    private String type;
+
+    @Parameter(name = ApiConstants.PARAMS,
+            description = "Additional command line options",


a little word on how these are processed/used?

The aim of this was to allow a user to specify optional arguments like they normally would when using the command from the terminal.

If a user passes an Illegal argument for a specific command type, then script execution will fail and stderr will be propagated back as API response

DaanHoogland · 2018-06-01T08:18:24Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+        authorized = RoleType.Admin)
+public class RemoteDiagnosticsCmd extends BaseCmd {
+
+    public static final String APINAME = "remoteDiganosis";


remoteDiagnosis is a better name IMNSHO, but it must coincide with the actual API name

Changed name to executeDiagnostics following ACS API naming conventions.

DaanHoogland · 2018-06-01T08:24:17Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    @Override
+    public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException,
+            ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
+        Preconditions.checkState(RemoteDiagnosticsService.DiagnosisType.contains(getType()), "%s is " +


as an enum this is not very flexible. consider making it a registry?

Under consideration.

DaanHoogland · 2018-06-01T08:26:30Z

api/src/main/java/org/apache/cloudstack/api/response/RemoteDiagnosticsResponse.java

+    @Param(description = "Script execution result")
+    private String details;
+
+    @SerializedName(ApiConstants.RESULT)


I thought we had discussed return value/exit code instead of success status?

I have included the exit code to be returned by the python script.

DaanHoogland · 2018-06-01T08:28:29Z

api/src/main/java/org/apache/cloudstack/api/response/RemoteDiagnosticsResponse.java

+    private Boolean success;
+
+    @SerializedName("command")
+    @Param(description = "Command passed to script")


this description doesn't tell me what I expect to read here. Is it the exact RemoteDiagnosticsCmd.type? or the cmd line as eventually executed remotely, including the standard and extra parameters, or even path?

I put this the for debug purposes, I will remove it in the final product. But this is the command line is eventually executed remotely.

E.g. command = ping 8.8.8.8 -c 5

ok, that might be usefull for an administrator in more elaborate cases as well

Removed this field in the API response.

DaanHoogland · 2018-06-01T08:29:08Z

api/src/main/java/org/apache/cloudstack/api/response/RemoteDiagnosticsResponse.java

+@EntityReference(value = VirtualMachine.class)
+public class RemoteDiagnosticsResponse extends BaseResponse {
+    @SerializedName(ApiConstants.DETAILS)
+    @Param(description = "Script execution result")


this is stdout on remote, right?

Correct. It represents both stdout and stderr.

DaanHoogland

neat code

DaanHoogland · 2018-06-01T08:42:46Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

@@ -0,0 +1,136 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


still nested comment

DaanHoogland · 2018-06-01T08:43:04Z

api/src/main/java/org/apache/cloudstack/api/response/RemoteDiagnosticsResponse.java

@@ -0,0 +1,66 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


nested comment

DaanHoogland · 2018-06-01T08:43:17Z

api/src/main/java/org/apache/cloudstack/diangosis/RemoteDiagnosticsService.java

@@ -0,0 +1,45 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


nested comment

DaanHoogland · 2018-06-01T08:44:04Z

api/src/main/java/org/apache/cloudstack/diangosis/RemoteDiagnosticsService.java

+public interface RemoteDiagnosticsService {
+    RemoteDiagnosticsResponse executeDiagnosisToolInSystemVm(RemoteDiagnosticsCmd cmd) throws AgentUnavailableException, InvalidParameterValueException;
+
+    enum DiagnosisType {


I would like to see this as a registry, for instance by means of a comma separated configuration value

Under consideration

DaanHoogland · 2018-06-01T08:44:45Z

api/src/main/java/org/apache/cloudstack/diangosis/RemoteDiagnosticsService.java

+
+        public static boolean contains(String cmd){
+            try{
+                valueOf(cmd);


might we go for ignore case?

Refactored enum

DaanHoogland · 2018-06-01T08:46:55Z

core/src/main/java/com/cloud/agent/resource/virtualnetwork/VRScripts.java


    public static final String VR_CFG = "vr_cfg.sh";

+    // New script for use by remoteDiagnosis API


Are these both to be used? or is this part of a transition?

Part of transition, final product will make use of a python based script

DaanHoogland · 2018-06-01T09:02:04Z

core/src/main/java/org/apache/cloudstack/diagnosis/ExecuteDiagnosisAnswer.java

+
+import com.cloud.agent.api.Answer;
+
+public class ExecuteDiagnosisAnswer extends Answer {


so this one stuck to the more beautiful name.

DaanHoogland · 2018-06-01T09:20:33Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosticsServiceImpl.java

@@ -0,0 +1,121 @@
+/*
+ * // Licensed to the Apache Software Foundation (ASF) under one


nested comment

DaanHoogland · 2018-06-01T09:33:38Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosticsServiceImpl.java

+
+    private static String setupRemoteCommand(String diagnosisType, String destinationIpAddress, String optionalArguments){
+        if (optionalArguments != null){
+            return String.format("%s %s", diagnosisType, destinationIpAddress+" "+optionalArguments);


so the optional arguments are always last. for some commands that will exclude options. thought the trend is to allow for any argument on any place in teh command line. This is possible place for augmentation.

I am working on refactoring this entire method during migration to python based script. I aim at parsing all arguments passed for each command type.

DaanHoogland · 2018-06-01T09:35:20Z

server/src/main/java/org/apache/cloudstack/diagnosis/RemoteDiagnosticsServiceImpl.java

+        if (optionalArguments != null){
+            return String.format("%s %s", diagnosisType, destinationIpAddress+" "+optionalArguments);
+        }
+        return String.format("%s %s", diagnosisType, destinationIpAddress);


second occurence of the same string, extract to a constant? for instance named COMMAND_LINE_TEMPLATE.

…rameters

rohityadavcloud

Please address outstanding issues, let's syncup to discuss some of the items.

rohityadavcloud · 2018-06-06T07:25:56Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+import javax.inject.Inject;
+
+@APICommand(name = RemoteDiagnosticsCmd.APINAME,
+        since = "4.11",


Change since to 4.12.0.0.

rohityadavcloud · 2018-06-06T07:26:39Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+        authorized = {RoleType.Admin})
+public class RemoteDiagnosticsCmd extends BaseCmd {
+    private static final Logger s_logger = Logger.getLogger(RemoteDiagnosticsCmd.class);
+    public static final String APINAME = "remoteDiganostics";


Fix typo in APINAME to remoteDiagnostics, or consider above ^^

Amended API command name to executeDiagnostics following CS naming convention.

rohityadavcloud · 2018-06-06T07:27:12Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    /////////////////////////////////////////////////////
+    //////////////// API parameters /////////////////////
+    /////////////////////////////////////////////////////
+    @Parameter(name = ApiConstants.ID, type = CommandType.UUID, required = true, entityType = RemoteDiagnosticsResponse.class,


The use of RemoteDiagnosticsResponse could be misleading, let's discuss this issue over syncup.

This could be UserVmResponse (sort of a hack, as the annotation leads to all the user/system vms), or perhaps SystemVmResponse. You'll need to check if this works.

UserVmResponse shows guest VM instances only. SystemVmResponse or DomainRouterResponse do not show anything, will revisit and investigate further.

rohityadavcloud · 2018-06-06T07:27:43Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    private Long id;
+
+    @Parameter(name = ApiConstants.IP_ADDRESS, type = CommandType.STRING, required = true,
+            description = "Destination IP address to test connection to")


Change description to say - IP/address i.e. the address could be an IP or (domain) address.

rohityadavcloud · 2018-06-06T07:28:24Z

api/src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RemoteDiagnosticsCmd.java

+    private String address;
+
+    @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, required = true,
+            description = "The type of diagnostics command to be executed on the System VM instance, e.g. ping, tracert or arping")


In the description, instead of giving example, state the current list of comma-separated enums that can be used.

rohityadavcloud · 2018-06-06T08:05:39Z

test/integration/smoke/test_systemvm_diagnostics.py

+
+from nose.plugins.attrib import attr
+
+class TestRemoteDiagnosticsInSystemVMs(cloudstackTestCase):


I'll review this next time in detail after rest of the code reviews have been addressed.

Ready for review

rohityadavcloud · 2018-06-06T08:06:09Z

test/integration/smoke/test_systemvm_diagnostics.py

+            domainid=cls.account.domainid,
+            serviceofferingid=cls.service_offering.id
+        )
+        cls.cleanup = [


You also want to cleanup the vm that was created.

rohityadavcloud · 2018-06-06T08:06:27Z

test/integration/smoke/test_systemvm_diagnostics.py

+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.hypervisor = self.testClient.getHypervisorInfo()
+        return


No need to return after end of the method. Remove it from here and rest of the methods.

rohityadavcloud · 2018-06-06T08:07:13Z

test/integration/smoke/test_systemvm_diagnostics.py

+        self.hypervisor = self.testClient.getHypervisorInfo()
+        return
+
+    def tearDown(self):


You probably want to remove self.cleanup. Also add newline after pass. Run your python file via pep8 or pylint etc.

rohityadavcloud · 2018-06-06T08:08:32Z

tools/apidoc/gen_toc.py

    'cloudian': 'Cloudian',
-    'Sioc' : 'Sioc'
+    'Sioc' : 'Sioc',
+    'remoteDiganostics': 'Virtual Machine'


Rename as 'Diagnostics': 'Diagnostics'. When you build cloudstack, go to tools/apidocs/target and open the generated html to see how APIs are grouped in sections.

rohityadavcloud · 2018-06-06T09:34:57Z

server/src/main/java/org/apache/cloudstack/diagnostics/RemoteDiagnosticsServiceImpl.java

+    }
+
+    @Override
+    public String getConfigComponentName() {


Remove this as well.

rohityadavcloud · 2018-06-06T09:35:10Z

server/src/test/java/org/apache/cloudstack/diagnostics/RemoteDiagnosticsServiceImplTest.java

+// under the License.
+//
+
+


Remove extra new line.

rohityadavcloud · 2018-06-06T09:36:27Z

server/src/test/java/org/apache/cloudstack/diagnostics/RemoteDiagnosticsServiceImplTest.java

+        Mockito.when(vmInstanceDao.findByIdTypes(remoteDiagnosticsCmd.getId(), VirtualMachine.Type.ConsoleProxy,
+                VirtualMachine.Type.DomainRouter, VirtualMachine.Type.SecondaryStorageVm)).thenReturn(null);
+        diagnosticsService.executeDiagnosticsToolInSystemVm(remoteDiagnosticsCmd);
+    }


Mock and check the answer

rohityadavcloud · 2018-06-06T09:41:21Z

systemvm/debian/opt/cloud/bin/diagnose_system_vm.py

+            return cmd
+
+        else:
+            return cmd + " -c 4"


Default options for each type/command.

rohityadavcloud · 2018-06-06T09:44:35Z

test/integration/smoke/test_systemvm_diagnostics.py

@@ -0,0 +1,173 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Simplify/Rename the file to test_diagnostics.py

Renamed file.

DaanHoogland

biggest comment is the that logging of exceptions is usually done on catching and not before throwing. lot's of comments, nothing lethal, though

DaanHoogland · 2018-06-14T14:12:51Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+    public DiagnosticsType getType() {
+        DiagnosticsType diagnosticsType = DiagnosticsType.getCommand(type);
+        if (diagnosticsType == null) {
+            LOGGER.warn("An Invalid diagnostics command type passed: " + type);


I don't think you need to log this here as you are throwing it and it will be caught and logged elsewhere

DaanHoogland · 2018-06-14T14:14:37Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+    public String getOptionalArguments() {
+        final String EMPTY_STRING = "";
+
+        if (optionalArguments == null || optionalArguments.isEmpty()) {


I do this a lot as well, but it's better to use CollectionUtils.isEmpty()

There is not much difference in terms of performance, but adopted since it provides cleaner code

DaanHoogland · 2018-06-14T14:16:38Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+        final Pattern pattern = Pattern.compile(regex);
+        final boolean hasInvalidChar = pattern.matcher(optionalArguments).find();
+        if (!hasInvalidChar) {
+            LOGGER.error("An Invalid character has been passed as an optional parameter");


again, you don't need to log what you throw. In this case I would include the offensive parameter for convenience of the operator.

DaanHoogland · 2018-06-14T14:17:35Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+                response.setStdout(answerMap.get("STDOUT"));
+                response.setStderr(answerMap.get("STDERR"));
+                response.setExitCode(answerMap.get("EXITCODE"));
+                response.setResult(answerMap.get("SUCCESS"));


as discussed: considder not returning success but only exitcode.

Removed success return

DaanHoogland · 2018-06-14T14:18:26Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+                response.setStderr(answerMap.get("STDERR"));
+                response.setExitCode(answerMap.get("EXITCODE"));
+                response.setResult(answerMap.get("SUCCESS"));
+                response.setObjectName("diagnostics");


should the name contain some noun or verb indicating that it contains the resiult of a test/diagnostic command-run?

maybe involve Paul in that discussion

Changed it to diagnostics results

DaanHoogland · 2018-06-14T16:51:58Z

plugins/hypervisors/simulator/src/main/java/com/cloud/agent/manager/MockVmManagerImpl.java

 import com.cloud.utils.db.TransactionLegacy;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.VirtualMachine.PowerState;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;


if only the order of imports has changed you better revert this

DaanHoogland · 2018-06-14T16:52:12Z

plugins/hypervisors/simulator/src/main/java/com/cloud/agent/manager/SimulatorManager.java

 import com.cloud.utils.component.Manager;
 import com.cloud.vm.VirtualMachine.PowerState;

+import java.util.HashMap;


if only the order of imports has changed you better revert this

DaanHoogland · 2018-06-14T16:54:12Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

+        final VMInstanceVO vmInstance = instanceDao.findByIdTypes(vmId, VirtualMachine.Type.ConsoleProxy, VirtualMachine.Type.DomainRouter, VirtualMachine.Type.SecondaryStorageVm);
+
+        if (vmInstance == null) {
+            LOGGER.error("Invalid system vm id provided " + vmId);


both logging and throwing is not needed, also you would use a single string object to use for both ;)

DaanHoogland · 2018-06-14T16:55:34Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

+        final Long hostId = vmInstance.getHostId();
+
+        if (hostId == null) {
+            LOGGER.warn("Unable to find host for virtual machine instance: " + vmInstance.getInstanceName());


if you throw an exception it is at least an error, not a warning.
you can reuse one string for both
is it really needed to bot log and throw?

DaanHoogland · 2018-06-14T17:03:30Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

+                throw new CloudRuntimeException("Failed to execute diagnostics command: " + answer.getDetails());
+            }
+        } catch (OperationTimedoutException e) {
+            LOGGER.warn("Timed Out", e);


a warning should be an error that can be handled. throwing and warning seem kind of contradictory

DaanHoogland · 2018-06-18T14:10:20Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

        final DiagnosticsCommand command = new DiagnosticsCommand(cmdType, cmdAddress, optionalArguments);
        command.setAccessDetail(NetworkElementCommand.ROUTER_NAME, vmInstance.getInstanceName());
-        command.setAccessDetail(NetworkElementCommand.ROUTER_IP, routerControlHelper.getRouterControlIp(vmInstance.getId()));
+        //command.setAccessDetail(NetworkElementCommand.ROUTER_IP, routerControlHelper.getRouterControlIp(vmInstance.getId()));


remove comment in code please, git can get it back for you!

DaanHoogland · 2018-06-18T14:10:45Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

+        final DataCenterVO dcVo = dataCenterDao.findById(vmInstanceVO.getDataCenterId());
+        String controlIP = null;
+
+       // if(vmInstanceVO.getHypervisorType() == Hypervisor.HypervisorType.VMware  && dcVo.getNetworkType() == DataCenter.NetworkType.Basic ){


please remove this comment. it has no value

rohityadavcloud · 2018-06-20T06:20:47Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

+        final DiagnosticsCommand command = new DiagnosticsCommand(cmdType, cmdAddress, optionalArguments, vmManager.getExecuteInSequence(vmInstance.getHypervisorType()));
+
+        if (vmInstance.getType() == VirtualMachine.Type.DomainRouter) {
+            command.setAccessDetail(NetworkElementCommand.ROUTER_IP, routerControlHelper.getRouterControlIp(vmInstance.getId()));


Remove the if-else block and replace by something like this:
networkManager.getSystemVMAccessDetails(vm). To use network manager, try something like:

@Inject private NetworkOrchestrationService networkManager;

Resolved, solution implemented and tested on VMware, KVM and Xenserver

rohityadavcloud · 2018-06-20T08:53:05Z

server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java

-            command.setAccessDetail(NetworkElementCommand.ROUTER_IP, nicVO.getIPv4Address());
-        }
-        command.setAccessDetail(NetworkElementCommand.ROUTER_NAME, vmInstance.getInstanceName());
+//        if (vmInstance.getType() == VirtualMachine.Type.DomainRouter) {


After testing, the commented code can be removed.

rohityadavcloud

Changes requested.

rohityadavcloud · 2018-06-20T10:30:16Z

tools/apidoc/gen_toc.py

    'cloudian': 'Cloudian',
-    'Sioc' : 'Sioc'
+    'Sioc' : 'Sioc',
+    'Diagnostics': 'Diagnostics'


You can put the API under Misc, unless you think more APIs are going to join this section in apidocs.

I anticipate the retrieveDiagnostics API to grouped together with the Remote Diagnostics within the same section viz. Diagnostics in the apidocs.

rohityadavcloud · 2018-06-20T10:31:10Z

test/integration/smoke/test_diagnostics.py

+        )
+
+        # Validate Traceroute command execution with a non-existent/pingable IP address
+        cmd.ipaddress = '999.9999.9999.9999.9999'


try to pass a valid but unreachable IPv4 ip see https://stackoverflow.com/questions/10456044/what-is-a-good-invalid-ip-address-to-use-for-unit-tests
Make changes to other related invalid ips you've used. Ideally, each + and - test should be a separate test case.

Changes made, using IP address 192.0.2.2 for the - test case. Also separated the + and - test cases for the ping command.

rohityadavcloud · 2018-06-20T10:34:28Z

systemvm/debian/opt/cloud/bin/diagnostics.py

+            return_code = 127
+
+        finally:
+            print('%s}' % stdout.strip())


Improve prefix/container for stdout and stderr with something like STDOUT{%value here}, STDERR{%value here}. Don't assume that closing brace } may not be part of stdout or stderr.

Change, using && for parsing

This leads to this: exitcode = EXITCODE: {0}

Will parse the details as {} without the preceding STDERR/STDOUT/EXITCODE.
The key values are already provided by/in the API response class.

rohityadavcloud · 2018-06-20T10:34:41Z

server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java

+    @InjectMocks
+    private DiagnosticsServiceImpl diagnosticsService = new DiagnosticsServiceImpl();
+
+


Remove extra newline.

rohityadavcloud · 2018-06-20T10:34:55Z

server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java

+
+    @Before
+    public void setUp() throws Exception {
+


Remove setUp if you're not doing anything.

rohityadavcloud · 2018-06-20T10:39:04Z

api/src/main/java/org/apache/cloudstack/api/response/ExecuteDiagnosticsResponse.java

+
+@EntityReference(value = VirtualMachine.class)
+public class ExecuteDiagnosticsResponse extends BaseResponse {
+    @SerializedName("STDOUT")


lowercase the api response keys to stdout, stderr, exitcode.

Use api constant class for them, perhaps use the same keys in the class where the details map is created.

rohityadavcloud · 2018-06-20T10:40:10Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+        try {
+            final Map<String, String> answerMap = diagnosticsService.runDiagnosticsCommand(this);
+            if (answerMap != null || !answerMap.isEmpty()) {
+                response.setStdout(answerMap.get("STDOUT"));


Change the .get() to get based on a known constant than a hardcoded string like STDOUT, STDERR and EXITCODE.

rohityadavcloud · 2018-06-20T10:40:38Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+        final String EMPTY_STRING = "";
+
+        if (optionalArguments == null || optionalArguments.isEmpty()) {
+            return EMPTY_STRING;


Return "" is fine, than declare a variable and return that.

rohityadavcloud · 2018-06-20T10:41:13Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+        final String regex = "^[\\w\\-\\s]+$";
+        final Pattern pattern = Pattern.compile(regex);
+        final boolean hasInvalidChar = pattern.matcher(optionalArguments).find();
+        if (!hasInvalidChar) {


Write +/- unit tests for this validation method.

rohityadavcloud · 2018-06-20T10:41:39Z

...src/main/java/org/apache/cloudstack/api/command/admin/diagnostics/ExecuteDiagnosticsCmd.java

+        since = "4.12.0.0")
+public class ExecuteDiagnosticsCmd extends BaseCmd {
+    private static final Logger LOGGER = Logger.getLogger(ExecuteDiagnosticsCmd.class);
+    public static final String APINAME = "executeDiagnostics";


Can you consider renaming this to runDiagnostics. Ignore if already discussed with Paul and others.

At this point, might be a bit late. It would require redoing and testing a lot of things i.e unit/marvin test cases etc.

rohityadavcloud

Please address outstanding issues, then close this PR and send this to apache/cloudstack as a community PR.

dhlaluku

I have resolved all the comments

…d to targetid

* NSX integration - skeletal code * Fix module not loading on startup * add upgrade path and daos \n add nsx controller command * add support for adding and listing nsx provider to a zone * add license * add default VPC offering and update upgrade path * add global setting to enable nsx plugin * add delete nsx controller operation * add nsxresource * add NSX resource , api client, create tier1 gw * update db * update response and add license * Add support to create and delete nsx tier-1 gateway * add license * cleanup and add skeletal code for network creation * add create/delete segment and UI integration * add license * address code smells - part 1 * fix test / build failure * NSX integration - skeletal code * Fix module not loading on startup * add upgrade path and daos \n add nsx controller command * add support for adding and listing nsx provider to a zone * add license * add default VPC offering and update upgrade path * add global setting to enable nsx plugin * add delete nsx controller operation * add nsxresource * add NSX resource , api client, create tier1 gw * update db * update response and add license * Add support to create and delete nsx tier-1 gateway * add license * cleanup and add skeletal code for network creation * add create/delete segment and UI integration * add license * address code smells - part 1 * fix test / build failure * add ui changes + update nsx_provider table transport zones + use NSX broadcast domain for add nics to router * ui: fix password field, and backend changes * add route advertisement * update offering * update offering * add sleep before deletion of vpc / tier g/w for ports to be removed * move creation of segments to design phase * change provider to VPC router for Dhcp & dns service in an nsx offering * Add public nic for NSX * reserve first IP (after g/w) of subnet for router nic - NSX * revert reserving 1st IP in vpc segments * [NSX] Create a DHCP relay and add it to a VPC tier segment (#107) * Create DHCP relay command and execute request * In progress integrate with networking * Create DHCP relay config on the network VR allocation * Revert domain router dao changes * Create DHCP relay con VR nic plug to NSX network * Link DHCP relay config to segment after creation * [NSX] Cleanup DHCP Relay config on segment deletion (#108) * Cleanup DHCP Relay config on segment deletion * update segment & relay name generators and call delete dhcprelay after deletion of segment * address comment * [NSX] Fix DHCP relay config deletion was missing zone name (apache#8068) * [NSX] Refactor API wrapper operations (apache#8059) * [NSX] Refactor API wrapper operations * Big refactor * Address review comment * change network cidr to cidr to prevent NPE * add domain and zone names to the various networks - vpc & tier --------- Co-authored-by: Pearl Dsilva <[email protected]> * Nsx unit tests (apache#8090) * Add tests * add test for NsxGuestNetworkGuru * add unit tests for NsxResource * add unti tests for NsxElement * cleanup * [NSX] Refactor API wrapper operations * update tests * update tests - add nsxProviderServiceImpl test * add unit test - NsxServiceImpl * add license * Big refactor * Address review comment * change network cidr to cidr to prevent NPE * add domain and zone names to the various networks - vpc & tier * fix tests --------- Co-authored-by: nvazquez <[email protected]> * modify NSX resource naming convention (apache#8095) * modify NSX resource naming convention * remove unused imports * add a setup phase between desgin and implementation of a network for intermediary steps * add method to all classes * NSX: Refactor Network & VPC offering (apache#8110) * [NSX] Refactor API wrapper operations * Network offering changes for NSX * fix services and provider combination * address comments: rename param * update nsx_mode parameter --------- Co-authored-by: nvazquez <[email protected]> * fix test * [NSX] Allow NSX isolated networks (apache#8132) * Add network offerings for NSX on isolated networks * Fix offerings creation * In progress NSX isolated network * Fixes * Fix NIC allocation to router * NSX: Add Step for Adding Public traffic network for NSX During zone creation (apache#8126) * NSX: Add Step for Adding Public traffic network for NSX * address comments and cleanup * address comment * remove indent * NSX: Create and Delete static NAT & Port forward rules (apache#8131) * NSX: Create and delete NSX Static Nat rules * fix issues with static nat * add static nat * Support to add and delete Port forward rules * add license * fix adding multiple pf rules * cleanup * fix lint check * fix smoke tests * fix smoke tests * Nsx add lb rule (apache#8161) * NSX: Create and delete NSX Static Nat rules * fix issues with static nat * add static nat * Support to add and delete Port forward rules * add license * fix adding multiple pf rules * cleanup * NSX: Add support to create and delete Load balancer rules * fix deletion of lb rules * add header file and update protocol detail * build failure fix * [NSX] Add SNAT support (apache#8100) * In progress add source NAT * Fix after merge * Fix tests * Fix NPE on isolated network deletion * Reserve source NAT IP when its not passed for NSX VPC * Create source NAT rule on VR NIC allocation * Fix update VPC and remove VPC to update and remove SNAT rule * Fix packaging * Address review comment * Fix build * fix build - unused import * Add defensive checks * Add missing design to NSX public guru --------- Co-authored-by: Pearl Dsilva <[email protected]> * NSX: Fix VR public NIC allocation (apache#8166) * NSX: fix LB member addition and deletion and add defensive checks (apache#8167) * Fix public NIC NPE on broadcast URI * NSX: Router Public nic to get IP from systemVM Ip range (apache#8172) * NSX: Router Public nic to get IP from systemVM Ip range * Fix VR IP address and setSourceNatIp command * NSX: hide systemVM reserved IP range SourceNAT * fix test --------- Co-authored-by: nvazquez <[email protected]> * fix test failure * test failure fix * [NSX] Fix update source NAT IP (apache#8176) * [NSX] Fix update source NAT IP * Fix startup * Fix API result * NSX - add LB route Advertizement (apache#8192) * [NSX] Add ACL types support (apache#8224) * NSX: Create segment group on segment creation * Add unit tests * Remove group for segment before removing segment * Create Distributed Firewall rules * Remove distributed firewall policy on segment deletion * Fix policy rule ID and add more unit tests * Fix DROP action rules and transform tests * Add new ACL rules * Fixes * associate security policies with groups and not to DFW and add deletion of rules * Fix name convention --------- Co-authored-by: Pearl Dsilva <[email protected]> * NSX: Fix creation of VPCs (apache#8320) * Fix ACL rules creation (apache#8323) * [NSX] Fix database views (apache#8325) * NSX: Add CKS Support & Firewall rules for Isolated Networks (apache#8189) * NSX: Add ALL LB IP to the list of route advertisements in tier1 * NSX: Support Source NAT on NSX Isolated networks * NSX: Cks Support * NSX: Create segment group on segment creation * Add unit tests * Remove group for segment before removing segment * Create Distributed Firewall rules * Remove distributed firewall policy on segment deletion * Fix policy rule ID and add more unit tests * Add support for routed NSX Isolated networks \n and non RFC 1918 compliant IPs * Add support for routed NSX Isolated networks \n and non RFC 1918 compliant IPs * Add Firewall rules * build failure - fix unit test * fix npes * Add support to delete firewall rules * update nsx cks offering * add license * update order of ports in PF & FW rules * fix filter for getting transport zones * CKS support changed - MTU updated, etc * add LB for CKS on VPC * address comments * adapt upstream cks logic for vpc * rever mtu hack * update UI changes as per upstream fix * change display test for CKS n/w offerings for isolated and VPC tiers * add extra line for linter * address comment * revert list change --------- Co-authored-by: nvazquez <[email protected]> * fix ui build failure * [NSX] Address SonarCloud Bugs (apache#8341) * [NSX] Address SonarCloud Bugs * Fix NSX API connection issues * NSX: Add unit tests to increase coverage (apache#8355) * NSX: Add unit tests * cleanup unused imports * add more unit tests * add tests for publicnsxnetworkguru * add license * fix build failures * address sonar comment * fix security hotspots * NSX: Add more unit tests (apache#8381) * NSX : Unit tests * remove unused imports * remove unused import causing build failure * fix build failures due to unused imports * fix build failure * fix test assertion * remove unused imports * remove unused import * Nsx UI zone bug (apache#8398) * NSX: Attempt to fix NSX Zone creation bug for public networks * fix zone wizard public traffic issue * add proper filtering of offerings based on VPC nsx mode * clean up console logs * NSX: Fix code smells and reported bugs (apache#8409) * NSX: Fix code smells and reported bugs * fox override issue * remove unused imports * fix test * refactor code to reduce complexity * add lisence * cleanup * fix build failure * fix build failure * address comments * test - add config to ignore certain files from test coverage * test exclusion of classes from test cov * rever pom changes * [NSX] Add more unit tests (apache#8431) * [NSX] Add more unit tests * More tests * Fix build errors * NSX: Prevent creation of L2 and Shared networks for NSX (apache#8463) * NSX: Prevent creation of L2 and Shared networks for NSX * add checks to backend to prevent creation of l2 and shared networks in nsx zones and filter only nsx offerings when creating isolated networks * cleanup * NSX: Fix code smells (apache#8436) * NSX: Fix code smells * Add changes to service creation logic * CKS: Add action to during firewall rule creation (apache#8498) * NSX,UI: Deduplicate network list when creating kubernetes clusters (apache#8513) * NSX: Make LB service selectable in network offering (apache#8512) * NSX: Make LB service selectable in network offering * fix label * address comments * address comments * NSX: Add appropriate error message when icmp type is set to -1 for NSX (apache#8504) * NSX: Add appropriate error message when icmp type is set to -1 for NSX * address comments * update text * fix test * fix test - build failure * fix test - build failure * NSX: Cleanup NSX resources during k8s cluster cleanup (apache#8528) * fix test failure * NSX: Improve segment deletion process (apache#8538) * NSX: Add passive monitor for NSX LB to test whether a server is available (apache#8533) * NSX: Add passive monitor for NSX LB to test whether a server is available * Add active monitors too * fix build failure * NSX: Add check for ICMP code / type for NSX zones (apache#8542) * NSX: Fix Routed Mode for Isolated and VPC networks (apache#8534) * NSX: Fix Routed Mode for Isolated and VPC networks * NSX: Fix Routed mode - add checks for ports added for FW rules * clean up code * fix build failure * NSX: Add retry logic with sleep to delete segments (apache#8554) * NSX: Add retry logic with sleep to delete segments * add logs * NSX: Fix custom ACL check (#2) * NSX: Fix custom ACL check * NSX: Fix custom ACL check * Nsx vpc routed mode (#5) * NSX: Fix VPC routed mode * NSX: VPC route mode * remove unnecessary changes * Nsx: Support internal LB (#4) * NSX: Support internal LB service in NSX * add lb removal logic * Fix UI issue hiding internal LB tab * Refactor method name --------- Co-authored-by: nvazquez <[email protected]> * NSX: Improve NSX resource cleanup process (#3) * Fix unit test * NSX: Add SourceNAT service to the default Routed offering for VPC (#13) * Fix VPC restart with cleanup (#12) * NSX: Fix ACL rule removal on replacement and fix rule order (#11) * NSX: fix smoke test failure for ACLs (#9) * Fix unit tests * Fix NSX plugin pom XML * NSX: Add support to re-order ACL rules (NSX FW rules) (#14) * [WIP] NSX: Add support to re-order ACL rules (NSX FW rules) * fix reordering of acl rules on all networks that it is associated to * clean up and attempt test fix * Fix tests * Remove unused import * tweak reorder logic --------- Co-authored-by: nvazquez <[email protected]> * Fix zone creation issue for internal load balancer * Fix * Fix unit test * fix logger * fix logger * fix logger * NSX: Fix VPC form to ignore source NAT IP when creating VPCs and fix label * Move SQL changes to the newest schema file * NSX: Last Fixes * Fix build --------- Co-authored-by: nvazquez <[email protected]>

dhlaluku requested a review from rohityadavcloud May 24, 2018 14:02

Dingane Hlaluku added 8 commits May 31, 2018 12:21

Beginning to End workflow for Virtual Router

0658e24

Refactoring unused code

bbe380d

Testing agent-manager connection error

836bcb1

Minor Fixes on Command/Answer class

67669db

Initial successful test with VR and Ping command

95e8e57

Modified code to allow for all system VMs and all three ICMP commands

17dc1e1

Fixing typos

8cf67d3

Fixing merge conflicts

c9d4458

dhlaluku force-pushed the remoteDiganosisAPI branch from 6f9cb8b to c9d4458 Compare May 31, 2018 10:41

Dingane Hlaluku added 2 commits May 31, 2018 17:36

Migrating to python based script

259a459

Renaming packages to diagnostics

cd5e623

DaanHoogland reviewed Jun 1, 2018

View reviewed changes

Removing nested comments

bf01218

DaanHoogland reviewed Jun 1, 2018

View reviewed changes

Dingane Hlaluku added 2 commits June 1, 2018 17:58

Working on unit test impl

448fb44

Migrated to python based script, and added validation for optional pa…

1e60bdc

…rameters

dhlaluku requested review from rohityadavcloud and removed request for rohityadavcloud June 5, 2018 14:35

Dingane Hlaluku added 2 commits June 5, 2018 18:14

Removing unsused imports

9b9cb89

Adding Marvin tests

72f7023

rohityadavcloud reviewed Jun 6, 2018

View reviewed changes

Dingane Hlaluku added 2 commits June 6, 2018 14:42

Adding traceroute command to Marvin tests

05483de

Renaming packages to diagnostics and code refactoring

1df8855

dhlaluku force-pushed the remoteDiganosisAPI branch from 39a6f40 to 007eb70 Compare June 13, 2018 17:17

dhlaluku removed request for charles-phiri, ernjvr, houthuis and nvazquez June 13, 2018 19:07

dhlaluku force-pushed the remoteDiganosisAPI branch from 007eb70 to 0c45874 Compare June 13, 2018 19:09

For rebuilding testing packages

289a4ed

DaanHoogland reviewed Jun 14, 2018

View reviewed changes

Dingane Hlaluku added 3 commits June 18, 2018 11:59

Using AgentManager's EasySend

e3f2bef

Testing VMware hypervisor

7645c07

Removing unused import

9801b9f

DaanHoogland reviewed Jun 18, 2018

View reviewed changes

Dingane Hlaluku added 4 commits June 18, 2018 16:44

Fix issue with packaging failures

149f422

Setting executeInSequence for VMware tests

97e560d

For testing VMware fix

50d152f

Fix typos in Answer class

93a3cfb

rohityadavcloud reviewed Jun 20, 2018

View reviewed changes

Testing Fix for system vm access details

5571123

rohityadavcloud reviewed Jun 20, 2018

View reviewed changes

Removed commented code

6a11767

rohityadavcloud requested changes Jun 20, 2018

View reviewed changes

Dingane Hlaluku added 2 commits June 20, 2018 19:35

Adding fix for VMware and ammending unit tests to capture changes

2ddb21b

Removed hard-coded values in Cmd class

11a818b

rohityadavcloud requested changes Jun 22, 2018

View reviewed changes

dhlaluku commented Jun 22, 2018

View reviewed changes

Dingane Hlaluku added 2 commits June 22, 2018 16:50

Changed API name to runDiagnostics

2f5767c

Updated marvin and unit test to use new API name, also changed from i…

80058c2

…d to targetid

dhlaluku closed this Jun 22, 2018

		@@ -0,0 +1,43 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,29 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,114 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,53 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,30 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,115 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,136 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,66 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,45 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one


		public static final String VR_CFG = "vr_cfg.sh";

		// New script for use by remoteDiagnosis API


		import com.cloud.agent.api.Answer;

		public class ExecuteDiagnosisAnswer extends Answer {

		@@ -0,0 +1,121 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one


		from nose.plugins.attrib import attr

		class TestRemoteDiagnosticsInSystemVMs(cloudstackTestCase):

		@@ -0,0 +1,173 @@
		# Licensed to the Apache Software Foundation (ASF) under one

		@InjectMocks
		private DiagnosticsServiceImpl diagnosticsService = new DiagnosticsServiceImpl();

Remote Diagnostics API #13

Remote Diagnostics API #13

Uh oh!

Conversation

dhlaluku commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhlaluku Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhlaluku Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhlaluku commented May 24, 2018 •

edited

Loading

dhlaluku Jun 1, 2018 •

edited

Loading

dhlaluku Jun 1, 2018 •

edited

Loading