Retrieve diagnostics (for review purpose) #14

DaanHoogland · 2018-05-28T11:26:02Z

No description provided.

DaanHoogland

nice concepts you use. I do not see the overall yet, but it looks promissing

DaanHoogland · 2018-05-28T12:38:51Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

-    HashMap<DiagnosticsKey.DiagnosticsType, Set<DiagnosticsKey<?>>> _diagnosticsTypeLevelsMap = new HashMap<DiagnosticsKey.DiagnosticsType, Set<DiagnosticsKey<?>>>();
+
+
+    \


what happened here? seems syntactically fine but kind of strange formatting

DaanHoogland · 2018-05-28T12:38:57Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

@@ -81,7 +77,7 @@ protected void populateConfiguration(Date date, Configurable configurable) {

        s_logger.debug("Retrieving keys from " + configurable.getClass().getSimpleName());

-        for (DiagnosticsKey<?> key : configurable..etConfigKeys()) {
+        for (DiagnosticsKey<?> key : configurable. . ..get.get.getConfigKeys()) {


some artifact that wasn't intended?

DaanHoogland · 2018-05-28T12:39:39Z

framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsKey.java

+
+
+
+/*    public ConfigKey.Scope scope() {


code in comment need not be, as we use a versioning system

DaanHoogland · 2018-05-28T12:46:58Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

 import org.apache.cloudstack.framework.config.Configurable;
 import org.apache.cloudstack.framework.config.ConfigDepot;
 import org.apache.cloudstack.framework.config.ConfigDepotAdmin;
-import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
+import org.apache.cloudstack.framework.config.ScopedConfigStorage;


I cannot find this class. is it checked in?

DaanHoogland

I like how you use the concept of generics. I also see what you are trying to achieve with it. I hope you are not making it to complicated for yourself. We are going to need a lot of unit and integration tests for this to have a clean end result.

DaanHoogland · 2018-05-29T07:47:04Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

-import org.apache.cloudstack.framework.config.ConfigDepot;
-import org.apache.cloudstack.framework.config.ConfigDepotAdmin;
-import org.apache.cloudstack.framework.config.ScopedConfigStorage;
+import org.apache.cloudstack.framework.config.*;


DaanHoogland · 2018-05-29T07:50:13Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

    }

    @Override
    public <T> void set(ConfigKey<T> key, T value) {
-        _configDao.update(key.key(), value.toString());
+        //_diagnosticsDao.update(key.key(), value.toString());


code in comment

DaanHoogland · 2018-05-29T07:50:18Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

    }

    @Override
    public <T> void createOrUpdateConfigObject(String componentName, ConfigKey<T> key, String value) {
-        createOrupdateConfigObject(new Date(), componentName, key, value);
+        //createOrupdateConfigObject(new Date(), componentName, key, value);


code in comment

DaanHoogland · 2018-05-29T07:50:32Z

framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsKey.java

-    public DiagnosticsKey(String role, DiagnosticsEntryType diagnosticstype, String description) {
-        _role = role;
-        _diagnosticsType = diagnosticstype;
+   /* public DiagnosticsKey(Class<T> type, String name, String description) {


code in comment

DaanHoogland · 2018-05-29T07:50:47Z

framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsKey.java

@@ -118,4 +147,8 @@ protected String valueOf(String value) {
        }
    }

+/*    public static DiagnosticsKey<?> getDiagnosticsClassKeys() {


code in comment

DaanHoogland · 2018-05-29T07:51:10Z

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

-            //get list of default files from the database table for this diagnostics type
+                } else {
+                    throw new InvalidParameterValueException("Invalid parameters");
+                   // System.exit(-1);


code in comment

DaanHoogland

the branch still doesn't compile for me.

DaanHoogland · 2018-05-29T09:10:31Z

.../config/src/main/java/org/apache/cloudstack/framework/config/impl/RetrieveDiagnosticsVO.java

-
-    public RetrieveDiagnosticsVO(String roleId, String role, String className, String value) {
-        this.roleId = roleId;
+/*    public RetrieveDiagnosticsVO(String roleId, String role, String className, String value) {


no code in comment please

DaanHoogland · 2018-05-29T09:10:54Z

.../config/src/main/java/org/apache/cloudstack/framework/config/impl/RetrieveDiagnosticsVO.java

@@ -73,35 +70,26 @@ public void setRole(String role) {
        this.role = role;
    }

-    public void setClassName(String className) {
+    public void setDiagnosticsType(String className) {


DaanHoogland · 2018-05-29T09:11:37Z

api/src/main/java/org/apache/cloudstack/diagnostics/RetrieveDiagnosticsService.java

@@ -35,28 +33,12 @@

 public interface RetrieveDiagnosticsService extends Manager, PluggableService {

-    ConfigKey<Long> RetrieveDiagnosticsTimeOut = new ConfigKey<Long>("Advanced", Long.class, "retrieveDiagnostics.retrieval.timeout", "3600",


why move to the Impl? isn't it fine here?

DaanHoogland · 2018-05-29T09:12:28Z

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

+            "The interval between garbage collection executions in seconds", true, ConfigKey.Scope.Global);
+
+
+/*    DiagnosticsKey<String> IPTablesRemove = new DiagnosticsKey<String>(String.class, "IPtables.remove", "The IPtables rules to be removed", null, null);


please remove code instead of commenting it out. we have a versioning system to retrieve old code.

DaanHoogland

I had a new high-level pass on the whole of it. I have two main questions:

naming in general needs attention. Can we improve on that a bit.
I want to see why you created the generic. What is your purpose for it?

DaanHoogland · 2018-05-30T08:18:06Z

api/src/main/java/org/apache/cloudstack/diagnostics/RetrieveDiagnosticsService.java

+
+    RetrieveDiagnosticsResponse getDiagnosticsFiles(final RetrieveDiagnosticsCmd cmd) throws InvalidParameterValueException, ConfigurationException;
+
+    ConfigKey<?>[] getConfigKeys();


this method shouldn't be declared here. It is part of interface Configurable.

Removed ConfigKey<?> getConfigKeys() from ServiceImpl

It should remain in the ServiceImpl, @charles-phiri . But it shouldn't be in this interface. It is already defined in Configerable.

DaanHoogland · 2018-05-30T08:20:16Z

api/src/main/java/com/cloud/event/EventTypes.java

@@ -266,6 +267,7 @@
    public static final String EVENT_SSVM_STOP = "SSVM.STOP";
    public static final String EVENT_SSVM_REBOOT = "SSVM.REBOOT";
    public static final String EVENT_SSVM_HA = "SSVM.HA";
+    public static final String EVENT_SSVM_DIAGNOSTICS = "SSVM.DIAGNOSTICS";


EVENT_SSVM_DIAGNOSTICS is not used outside this file. Do we really need it?

Removed EVENT_SSVM_DIAGNOSTICS

DaanHoogland · 2018-05-30T08:21:31Z

engine/schema/src/main/resources/META-INF/db/schema-41110to41200.sql

+INSERT INTO `cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`, `sort_order`) values (UUID(), 4, 'moveNetworkAclItem', 'ALLOW', 260) ON DUPLICATE KEY UPDATE rule=rule;
+
+CREATE TABLE `cloud`.`diagnosticsdata` (


isn't this more of a 'diagnostics_defaults' or 'diagnostics_configuration'? I don't think the name diagnosticsdata is right for this table.

I think it is a matter of preference. The name was suggested by Dag....don't know what you think?

diagnostics data to me suggests that the result of any action or gathering is in this table, as opposed to it containing a means of steering how gathering is done. Hence my two suggestions. I am not religious obout those two, but I think the name diagnosticsdata is misleading.

DaanHoogland · 2018-05-30T08:22:22Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

+
+    private void createOrupdateDiagnosticsObject(String componentName,  DiagnosticsKey<?> diagnosticsType) {
+        RetrieveDiagnosticsVO vo = _diagnosticsDao.findById(diagnosticsType.key());
+        //DiagnosticsKey diagnosticsKey = new DiagnosticsKey(diagnosticsType.getClass(), diagnosticsType.key(), "new diagnostics")


code in comment

DaanHoogland · 2018-05-30T08:23:31Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

+
+    private final static Logger s_logger = Logger.getLogger(DiagnosticsConfigDepotImpl.class);
+    @Inject
+    RetrieveDiagnosticsDao _diagnosticsDao;


we don't need the antiquated convention of prefix '_' on member names.

DaanHoogland · 2018-05-30T08:27:09Z

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

+
+                } else {
+                    throw new InvalidParameterValueException("Invalid parameters");
+                   // System.exit(-1);


code in comment

DaanHoogland · 2018-05-30T08:27:28Z

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

+
+    public DiagnosticsKey<?>[] getDiagnosticsConfigKeys()
+    {
+        return null; //new DiagnosticsKey<?>[] { IPTablesRemove, IPTablesRetrieve, LOGFILES, PROPERTYFILES, DNSFILES, DHCPFILES, USERDATA, LB, VPN   };


code in comment

DaanHoogland · 2018-05-30T08:27:39Z

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

+        return cmdList;
+    }
+
+/*    public SecondaryStorageVmVO startSecondaryStorageVm(long secStorageVmId) {


code in comment

DaanHoogland · 2018-05-30T08:28:38Z

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

+                if (fileDetails != null) {
+                    filesToRetrieve = fileDetails.split(",");
+                } else {
+                    filesToRetrieve = null;//retrieve default files from db


is this comment a TODO?

DaanHoogland · 2018-05-30T08:30:18Z

tools/apidoc/gen_toc.py

@@ -190,7 +190,8 @@
    'CA': 'Certificate',
    'listElastistorInterface': 'Misc',
    'cloudian': 'Cloudian',
-    'Sioc' : 'Sioc'
+    'Sioc' : 'Sioc',
+    'retrieveDiagnostics': 'System VM'


is this the right tag/token? it seems to me that it deserves its own class. It will also be applicable to hosts in the future (maybe)

From the documentation, it looks to me like it is the right file.

yes, it is the right file. I wonder about the value of the tag, not the place it is defined

DaanHoogland

so far, now.
pleae push further changes asap.

DaanHoogland · 2018-05-30T11:16:36Z

core/src/main/java/com/cloud/agent/api/RetrieveDiagnosticsAnswer.java

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


please remove the nested comment

DaanHoogland · 2018-05-30T11:16:51Z

core/src/main/java/com/cloud/agent/api/RetrieveDiagnosticsAnswer.java

+ * // under the License.
+ */
+
+// Licensed to the Apache Software Foundation (ASF) under one


three licenses is a bit overdone

DaanHoogland · 2018-05-30T11:17:29Z

core/src/main/java/com/cloud/agent/api/RetrieveDiagnosticsCommand.java

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


same issue as in answer

DaanHoogland · 2018-06-05T07:53:24Z

...rc/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RetrieveDiagnosticsCmd.java


-    public RetrieveDiagnosticsService getRetrieveDiagnosticsService() {
-        return retrieveDiagnosticsService;
+    public static Logger getS_logger() {


why an accessor for the logger? it is only used locally isn't it?

also it should be called LOGGER or LOG as it is static final. and not have the obsoleted 's_' prefix

DaanHoogland · 2018-06-05T07:55:18Z

...rc/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RetrieveDiagnosticsCmd.java

    }

-    public void setRetrieveDiagnosticsService(RetrieveDiagnosticsService retrieveDiagnosticsService) {
-        this.retrieveDiagnosticsService = retrieveDiagnosticsService;
+    public static String getAPINAME() {


this is not to be accessed outside the class is it?

DaanHoogland · 2018-06-05T07:56:06Z

engine/schema/src/main/resources/META-INF/db/schema-41110to41200.sql

-INSERT INTO `cloud`.`diagnosticsdata` (`role`, `class`, `value`) values ('VR', 'VPN', '<vpn configuration file>');
-INSERT INTO `cloud`.`diagnosticsdata` (`role`, `class`, `value`) values ('VR', 'LOGFILES', 'cloud.log,agent.log');
-INSERT INTO `cloud`.`diagnosticsdata` (`role`, `class`, `value`) values ('CPVM', 'PROPERTYFILES', '<CPVM property file>');
+INSERT INTO `cloud`.`diagnosticsdata` (`role`, `class`, `value`) values ('SecondaryStorageVm', 'LOGFILES', 'cloud.log,agent.log,[IPTABLES]');


DaanHoogland · 2018-06-05T07:56:34Z

...work/config/src/main/java/org/apache/cloudstack/framework/config/DiagnosticsConfigDepot.java

@@ -0,0 +1,38 @@
+/*


can you remove the nesting in the comment, please?

no, it is still here

I have changed it. Will make a push soon after completing the code I am working on.

DaanHoogland · 2018-06-05T07:56:56Z

.../config/src/main/java/org/apache/cloudstack/framework/config/RetrieveDiagnosticsDaoImpl.java

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


please remove the nesting in the comment.

but you didn't commit it?

DaanHoogland · 2018-06-07T10:14:44Z

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

@@ -37,6 +37,11 @@
    @Param(description = "the date and time of the last download of the diagnostics files")
    private Date lastSent;

+    @SerializedName(ApiConstants.TIMEOUT)
+    @Param(description = "the timeout (in seconds) for requests to the retrieve diagnostics API")


what is the purpose of this in the response? i.e. what information does the user get from this response field?

DaanHoogland · 2018-06-08T09:51:42Z

api/src/main/java/com/cloud/event/EventTypes.java

@@ -740,6 +743,7 @@
        entityEventDetails.put(EVENT_SSVM_STOP, VirtualMachine.class);
        entityEventDetails.put(EVENT_SSVM_REBOOT, VirtualMachine.class);
        entityEventDetails.put(EVENT_SSVM_HA, VirtualMachine.class);
+        entityEventDetails.put(EVENT_SSVM_DIAGNOSTICS, VirtualMachine.class);


when is this fired?

Removed it but I think we might need Events at a later stage.

DaanHoogland · 2018-06-08T11:44:16Z

...rc/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RetrieveDiagnosticsCmd.java

+        RetrieveDiagnosticsResponse retrieveDiagnosticsResponse = new RetrieveDiagnosticsResponse();
+        try {
+            retrieveDiagnosticsService.getDiagnosticsFiles(this);
+            retrieveDiagnosticsResponse.setObjectName("retrievediagnostics");


is this the ideal object name? did you have other names considdered?

What can you suggest?

"diagnostics" maybe? or retrievedlogs, diagnosticsinformation. diagnosticdata.
I am really not sure it is a sincere question. It is just that retrievediagnostics (which is a spelling error btw, only one d) does not seem right.
from wiktionary: diagnostics

plural of diagnostic The process of determining the state of or capability of a component to perform its function(s).

retrieving a process is not intuitively right.

I think diagnosticsinformation makes sense. Do you want me to change the whole class or just retrieveDiagnosticsResponse.setObjectName("retrievediagnostics");

DaanHoogland · 2018-06-08T11:46:29Z

...rc/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RetrieveDiagnosticsCmd.java

+
+    @Override
+    public String getEventDescription() {
+        return "Retrieved diagnostics files from System VM =" + id;


"System VM =" may be too specific if we add hosts or hardware loadbalancers. We might go for "host: ". What do you think?

Perfectly makes sense

DaanHoogland · 2018-06-08T11:47:42Z

api/src/main/java/org/apache/cloudstack/api/command/admin/systemvm/DestroySystemVmCmd.java

@@ -16,8 +16,9 @@
 // under the License.
 package org.apache.cloudstack.api.command.admin.systemvm;

-import org.apache.log4j.Logger;


there is only imports chaged in this file. consider reverting it.

Reverted it.

DaanHoogland · 2018-06-08T11:55:44Z

core/src/main/java/com/cloud/agent/api/RetrieveDiagnosticsCommand.java

+    }
+
+    @Override
+    public boolean isQuery() {


please look at the formatting here

DaanHoogland · 2018-06-08T11:59:39Z

.../config/src/main/java/org/apache/cloudstack/framework/config/RetrieveDiagnosticsDaoImpl.java

+@Component
+public class RetrieveDiagnosticsDaoImpl extends GenericDaoBase<RetrieveDiagnosticsVO, String> implements RetrieveDiagnosticsDao
+{
+    private final SearchBuilder<RetrieveDiagnosticsVO> DiagnosticsSearchByType;


can you check if we need this, please. it is not used in the class or in its interface. I think you blindly copied my code from annotations and I should have removed it there myself. I am not sure, though.

DaanHoogland · 2018-06-08T12:03:05Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

+import java.util.HashMap;
+import java.util.List;
+
+public class DiagnosticsConfigDepotImpl implements DiagnosticsConfigDepot {


The name depot is not very fortunate. Please help think of a better one. I am thinking of just DiagnosticsConfiguration or DiagnosticsConfigurator.

You are right, thanks

DaanHoogland · 2018-06-08T12:03:59Z

...ig/src/main/java/org/apache/cloudstack/framework/config/impl/DiagnosticsConfigDepotImpl.java

+
+public class DiagnosticsConfigDepotImpl implements DiagnosticsConfigDepot {
+
+    private final static Logger s_logger = Logger.getLogger(DiagnosticsConfigDepotImpl.class);


the naming convention of s_ is obsolete, please use the name for a static final in all uppercase; i.e. LOG or LOGGER

Done. Will have to look at other classes and change as required.

DaanHoogland · 2018-06-08T12:05:42Z

...work/config/src/main/java/org/apache/cloudstack/framework/config/DiagnosticsConfigDepot.java

+import java.util.HashMap;
+import java.util.List;
+
+public interface DiagnosticsConfigDepot {


naming is a bit off. please see the -Impl for discussion

We have to discuss this so that I can learn the naming conversion. Meanwhile, I will leave it as it is.

I think you mean convention, right? I am not sure there is a convention for what you are trying to do here yet. That would mean we are free to choose. Of course with that freedom comes great responsibility ;) I must be intuitively right.

DaanHoogland · 2018-06-11T13:26:44Z

...rc/main/java/org/apache/cloudstack/api/command/admin/diagnostics/RetrieveDiagnosticsCmd.java

    @Parameter(name = ApiConstants.ID,
            type = CommandType.UUID,
-            entityType = RetrieveDiagnosticsResponse.class,
+            entityType = DomainRouterResponse.class,


So this call is now only valid for DomainRouters? using the DomainRouterResponse.class...? I think something went wrong here

DaanHoogland · 2018-06-14T07:53:39Z

.../config/src/main/java/org/apache/cloudstack/framework/config/RetrieveDiagnosticsDaoImpl.java

@@ -31,14 +31,14 @@
 @Component
 public class RetrieveDiagnosticsDaoImpl extends GenericDaoBase<RetrieveDiagnosticsVO, String> implements RetrieveDiagnosticsDao
 {
-    private final SearchBuilder<RetrieveDiagnosticsVO> DiagnosticsSearchByType;
+//    private final SearchBuilder<RetrieveDiagnosticsVO> DiagnosticsSearchByType;


did it work, without? (just remove, git will find it back if you need it;)

Yes, it did work thanks

DaanHoogland · 2018-06-15T09:12:54Z

core/src/main/java/com/cloud/agent/api/RetrieveDiagnosticsAnswer.java

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


this is still nested comment

borisstoyanov

Can we also add a marvin test?

borisstoyanov · 2018-07-02T07:53:57Z

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

+
+
+
+


is there any reason we need that many empty lines?

DaanHoogland · 2018-07-25T10:13:47Z

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

    }

-    protected Answer copyFileFromSystemVm(final NetworkElementCommand cmd, String script) throws Exception {
-        final File keyFile = getSystemVMKeyFile();
+    private CreateEntityDownloadURLAnswer downloadCompressedFileToSSVM(CreateEntityDownloadURLCommand cmd, final String diagnosticsZipFileName) {


can you break this up in smaller methods?
i.e. checkApache(), mkdirCommand(), linkCommand() etc...

DaanHoogland · 2018-07-26T14:59:14Z

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

+    private String secUrl;
+
+    @Inject
+    NetworkModel _networkModel;


please remove the underscore from the name

DaanHoogland · 2018-07-26T14:59:21Z

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

+    NetworkModel _networkModel;
+
+    @Inject
+    NetworkDao _networksDao = null;


please remove the underscore from the name

DaanHoogland · 2018-07-26T14:59:30Z

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

+    NetworkDao _networksDao = null;
+
+    @Inject
+    NicDao _nicDao = null;


please remove the underscore from the name

DaanHoogland · 2018-07-26T14:59:55Z

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

+    NetworkDao _networksDao = null;
+
+    @Inject
+    NicDao _nicDao = null;

    @Inject
    private HostDao _hostDao;


please remove the underscore from the names, here and below

DaanHoogland · 2018-07-26T15:00:46Z

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

@@ -220,6 +239,11 @@
    @Inject
    private VirtualMachineManager vmManager;

+    @Inject
+    protected DataCenterDao _dcDao = null;


please remove the underscore from the name

DaanHoogland · 2018-07-26T15:00:56Z

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

+    protected DataCenterDao _dcDao = null;
+
+    @Inject
+    protected PrimaryDataStoreDao _storagePoolDao = null;


please remove the underscore from the name

DaanHoogland · 2018-07-26T15:01:25Z

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

@@ -341,6 +364,8 @@ protected void loadDiagnosticsDataConfiguration() {
        }
        final Long vmId = cmd.getId();
        vmInstance = _vmDao.findByIdTypes(vmId, VirtualMachine.Type.ConsoleProxy, VirtualMachine.Type.DomainRouter, VirtualMachine.Type.SecondaryStorageVm);
+        //vmInstance.getDetails()


please remove code in comment

DaanHoogland · 2018-07-26T15:05:35Z

core/src/main/java/com/cloud/agent/api/RetrieveFilesCommand.java

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


this is a nested comment, please use either /* */ or //

DaanHoogland · 2018-07-26T15:06:16Z

...rk/config/src/main/java/org/apache/cloudstack/framework/config/impl/RetrieveDiagnostics.java

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


please remove nested comment

DaanHoogland · 2018-07-26T15:07:20Z

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

+    NicDao _nicDao = null;
+
+    @Inject
+    private HostDao _hostDao;


please remove the _ from this field and all those below

DaanHoogland · 2018-07-26T15:08:51Z

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

+                filesToRetrieve.add(files[i]);
+            }
+        }
+       // ManagementServerHostVO managementServerHostVO = managementServerHostDao.findById(systemVmId..getHostId());


remove commented code

DaanHoogland · 2018-07-26T15:09:02Z

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

+            }
+        }
+       // ManagementServerHostVO managementServerHostVO = managementServerHostDao.findById(systemVmId..getHostId());
+        //String ipHostAddress = managementServerHostVO.getServiceIP();


remove commented code

DaanHoogland · 2018-07-26T15:09:15Z

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

+        RetrieveFilesCommand retrieveFilesCommand = null;
+        Map<String, String> resultsMap;
+        final Long hostId = systemVmId.getHostId();
+        //Answer answer = null;


remove commented code

DaanHoogland · 2018-07-26T15:09:46Z

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

+                if (store == null) {
+                    throw new CloudRuntimeException("cannot find an image store for zone " + zoneId);
+                }
+                //DataStoreTO destStoreTO = store.getTO();


remove commented code

DaanHoogland · 2018-07-26T15:11:03Z

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

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


remove nested comments

DaanHoogland · 2018-07-26T15:11:55Z

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

+
+        }
+
+        Map<String, String> resultsMap = new HashMap<>();//retrieveDiagnosticsService.getDiagnosticsFiles(retrieveDiagnosticsCmd);


remove the commented code at the end of this line

DaanHoogland · 2018-07-26T15:12:31Z

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

+            except OSError as e:
+                print("Failed to create directory temp")
+
+        #manifest_file = open("temp/" + "manifest.txt", "w+")


remove commented code

DaanHoogland · 2018-07-26T15:12:50Z

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

+                print filename + " not found"
+                #manifest_file.write(filename + " not found.")
+
+        #zip_archive.write(manifest_file)


remove commented code

DaanHoogland · 2018-07-26T15:12:56Z

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

+                #manifest_file.write(filename + " not found.")
+
+        #zip_archive.write(manifest_file)
+        #manifest_file.close()


remove commented code

DaanHoogland · 2018-07-26T15:13:26Z

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

+    save_files = SaveIptablesToLogFile(arguments)
+    save_files.ensure_dir(file_path)
+    save_files.saveIpTableEntries(file_path)
+


too many extra lines

DaanHoogland · 2018-07-26T15:15:26Z

@charles-phiri please rebase to resolve conflicts and make sure the build passes

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* 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 <pearl1594@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <pearl1594@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <pearl1594@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <nicovazquez90@gmail.com> * 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 <nicovazquez90@gmail.com>

DaanHoogland commented May 28, 2018

View reviewed changes

DaanHoogland commented May 29, 2018

View reviewed changes

charles-phiri force-pushed the retrieve-diagnostics branch from 35eecfd to 9892e79 Compare May 29, 2018 10:06

DaanHoogland commented May 30, 2018

View reviewed changes

DaanHoogland commented Jun 7, 2018

View reviewed changes

DaanHoogland commented Jun 11, 2018

View reviewed changes

DaanHoogland commented Jun 14, 2018

View reviewed changes

DaanHoogland commented Jun 15, 2018

View reviewed changes

borisstoyanov reviewed Jul 2, 2018

View reviewed changes

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

Copy link

Member

borisstoyanov Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason we need that many empty lines?

charles-phiri added 19 commits July 8, 2018 14:16

Initial commit

1ee9980

some code to enhance the configure method in the Service Layer

9f7eff1

refactored Service interface

987534e

refactored Service interface

4ef4c2f

refactored Service interface

6256e91

refactored Service interface

c7e5572

refactored Service interface

74822ce

refactored Service interface

e017c1c

refactored Service interface

9438236

refactored Service interface

99bb448

refactored Service interface

3cdb059

refactored Service interface

be2d1b3

refactored Service interface

6c76aa9

refactored Service interface

1e24faa

refactored Service interface

ee7907c

refactored Service interface

65b7d38

Removed unused imports

182dea9

refactored Service interface

395b246

refactored Service interface

aae408c

DaanHoogland commented Jul 25, 2018

View reviewed changes

DaanHoogland commented Jul 26, 2018

View reviewed changes

charles-phiri added 5 commits July 26, 2018 22:11

Code cleanup and refactoring

3acbfe4

Complete RerieveDiagnostics API implementation for testing.

d08cc7b

Complete RerieveDiagnostics API .

3c12e07

Minor bug fixes.

f88bde0

Code for the RetrieveDiagnostics API.

720da4c

dhlaluku closed this Nov 29, 2018

shwstppr pushed a commit that referenced this pull request Aug 24, 2020

systemvm: use TLS v1.2 by default (#14)

32b5062

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

		HashMap<DiagnosticsKey.DiagnosticsType, Set<DiagnosticsKey<?>>> _diagnosticsTypeLevelsMap = new HashMap<DiagnosticsKey.DiagnosticsType, Set<DiagnosticsKey<?>>>();


		\

		@@ -35,28 +33,12 @@

		public interface RetrieveDiagnosticsService extends Manager, PluggableService {

		ConfigKey<Long> RetrieveDiagnosticsTimeOut = new ConfigKey<Long>("Advanced", Long.class, "retrieveDiagnostics.retrieval.timeout", "3600",

		"The interval between garbage collection executions in seconds", true, ConfigKey.Scope.Global);


		/* DiagnosticsKey<String> IPTablesRemove = new DiagnosticsKey<String>(String.class, "IPtables.remove", "The IPtables rules to be removed", null, null);


		RetrieveDiagnosticsResponse getDiagnosticsFiles(final RetrieveDiagnosticsCmd cmd) throws InvalidParameterValueException, ConfigurationException;

		ConfigKey<?>[] getConfigKeys();

		INSERT INTO `cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`, `sort_order`) values (UUID(), 4, 'moveNetworkAclItem', 'ALLOW', 260) ON DUPLICATE KEY UPDATE rule=rule;

		CREATE TABLE `cloud`.`diagnosticsdata` (

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

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

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


		public class DiagnosticsConfigDepotImpl implements DiagnosticsConfigDepot {

		private final static Logger s_logger = Logger.getLogger(DiagnosticsConfigDepotImpl.class);

		@@ -0,0 +1,24 @@

		/*
		* // Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,52 @@
		/*
		* // 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,228 @@
		/*
		* // Licensed to the Apache Software Foundation (ASF) under one


		}

		Map<String, String> resultsMap = new HashMap<>();//retrieveDiagnosticsService.getDiagnosticsFiles(retrieveDiagnosticsCmd);

Retrieve diagnostics (for review purpose) #14

Retrieve diagnostics (for review purpose) #14

Uh oh!

Conversation

DaanHoogland commented May 28, 2018

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!

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!

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!

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!

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!