[GitHub] [cloudstack] nvazquez commented on a change in pull request #4978: KVM High Availability regardless of storage

[GitBox <gi...@apache.org>]

nvazquez commented on a change in pull request #4978:
URL: https://github.com/apache/cloudstack/pull/4978#discussion_r830156484



##########
File path: plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHAConfig.java
##########
@@ -53,4 +53,32 @@
     public static final ConfigKey<Long> KvmHAFenceTimeout = new ConfigKey<>("Advanced", Long.class, "kvm.ha.fence.timeout", "60",
             "The maximum length of time, in seconds, expected for a fence operation to complete.", true, ConfigKey.Scope.Cluster);
 
+    public static final ConfigKey<Integer> KvmHaWebservicePort = new ConfigKey<Integer>("Advanced", Integer.class, "kvm.ha.webservice.port", "8443",
+            "It sets the port used to communicate with the KVM HA Agent Microservice that is running on KVM nodes. Default value is 8443.",
+            true, ConfigKey.Scope.Cluster);
+
+    public static final ConfigKey<Boolean> IsKvmHaWebserviceEnabled = new ConfigKey<Boolean>("Advanced", Boolean.class, "kvm.ha.webservice.enabled", "false",

Review comment:
       This setting is a bit misleading, one can disable the setting however the HA webserver will continue running on each host. I think it could be removed since when it is used, the check through the agent is also performed

##########
File path: plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KvmHaAgentClient.java
##########
@@ -0,0 +1,346 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+package org.apache.cloudstack.kvm.ha;
+
+import com.cloud.host.Host;
+import com.cloud.host.Status;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.VMInstanceDao;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+import org.apache.commons.httpclient.HttpStatus;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpRequestBase;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.ssl.SSLContexts;
+import org.apache.log4j.Logger;
+import org.jetbrains.annotations.Nullable;
+
+import javax.inject.Inject;
+import javax.net.ssl.SSLContext;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
+import java.security.KeyManagementException;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.util.Base64;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * This class provides a client that checks Agent status via a webserver.
+ * <br>
+ * The additional webserver exposes a simple JSON API which returns a list
+ * of Virtual Machines that are running on that host according to Libvirt.
+ * <br>
+ * This way, KVM HA can verify, via Libvirt, VMs status with an HTTP-call
+ * to this simple webserver and determine if the host is actually down
+ * or if it is just the Java Agent which has crashed.
+ */
+public class KvmHaAgentClient {
+
+    private static final Logger LOGGER = Logger.getLogger(KvmHaAgentClient.class);
+    private static final int ERROR_CODE = -1;
+    private static final String EXPECTED_HTTP_STATUS = "2XX";
+    private static final String VM_COUNT = "count";
+    private static final String STATUS = "status";
+    private static final String CHECK_NEIGHBOUR = "check-neighbour";
+    private static final int WAIT_FOR_REQUEST_RETRY = 2;
+    private static final int MAX_REQUEST_RETRIES = 2;
+    private static final JsonParser JSON_PARSER = new JsonParser();
+    static final String HTTP_PROTOCOL = "http";
+    static final String HTTPS_PROTOCOL = "https";
+    private final static String APPLICATION_JSON = "application/json";
+    private final static String ACCEPT = "accept";
+
+    @Inject
+    private VMInstanceDao vmInstanceDao;

Review comment:
       I think some of the logic could be placed on other class (maybe KvmHaHelper) as long with the DB access, and keep the client class simply to interact with the HA agent

##########
File path: debian/control
##########
@@ -56,3 +56,10 @@ Package: cloudstack-integration-tests
 Architecture: all
 Depends: ${misc:Depends}, cloudstack-marvin (= ${source:Version})
 Description: The CloudStack Marvin integration tests
+
+Package: cloudstack-agent-ha-helper

Review comment:
       Ping @GabrielBrascher 

##########
File path: plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KvmHaAgentClient.java
##########
@@ -0,0 +1,346 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+package org.apache.cloudstack.kvm.ha;
+
+import com.cloud.host.Host;
+import com.cloud.host.Status;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.VMInstanceDao;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+import org.apache.commons.httpclient.HttpStatus;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpRequestBase;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.ssl.SSLContexts;
+import org.apache.log4j.Logger;
+import org.jetbrains.annotations.Nullable;
+
+import javax.inject.Inject;
+import javax.net.ssl.SSLContext;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
+import java.security.KeyManagementException;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.util.Base64;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * This class provides a client that checks Agent status via a webserver.
+ * <br>
+ * The additional webserver exposes a simple JSON API which returns a list
+ * of Virtual Machines that are running on that host according to Libvirt.
+ * <br>
+ * This way, KVM HA can verify, via Libvirt, VMs status with an HTTP-call
+ * to this simple webserver and determine if the host is actually down
+ * or if it is just the Java Agent which has crashed.
+ */
+public class KvmHaAgentClient {
+
+    private static final Logger LOGGER = Logger.getLogger(KvmHaAgentClient.class);
+    private static final int ERROR_CODE = -1;
+    private static final String EXPECTED_HTTP_STATUS = "2XX";
+    private static final String VM_COUNT = "count";
+    private static final String STATUS = "status";
+    private static final String CHECK_NEIGHBOUR = "check-neighbour";
+    private static final int WAIT_FOR_REQUEST_RETRY = 2;
+    private static final int MAX_REQUEST_RETRIES = 2;
+    private static final JsonParser JSON_PARSER = new JsonParser();
+    static final String HTTP_PROTOCOL = "http";
+    static final String HTTPS_PROTOCOL = "https";
+    private final static String APPLICATION_JSON = "application/json";
+    private final static String ACCEPT = "accept";
+
+    @Inject
+    private VMInstanceDao vmInstanceDao;
+
+    /**
+     *  Returns the number of VMs running on the KVM host according to Libvirt.
+     */
+    public int countRunningVmsOnAgent(Host host) {
+        String protocol = getProtocolString();
+        String url = String.format("%s://%s:%d", protocol, host.getPrivateIpAddress(), getKvmHaMicroservicePortValue(host));
+        HttpResponse response = executeHttpRequest(url);
+
+        if (response == null)
+            return ERROR_CODE;
+
+        JsonObject responseInJson = processHttpResponseIntoJson(response);
+        if (responseInJson == null) {
+            return ERROR_CODE;
+        }
+
+        return responseInJson.get(VM_COUNT).getAsInt();
+    }
+
+    /**
+     * Returns the HTTP protocol. It can be 'HTTP' or 'HTTPS' depending on configuration 'kvm.ha.webservice.ssl.enabled'
+     */
+    protected String getProtocolString() {
+        boolean KvmHaWebserviceSslEnabled = KVMHAConfig.KvmHaWebserviceSslEnabled.value();
+        String protocol = HTTP_PROTOCOL;
+        if (KvmHaWebserviceSslEnabled) {
+            protocol = HTTPS_PROTOCOL;
+        }
+        return protocol;
+    }
+
+    /**
+     * Returns the port from the KVM HA Helper according to the configuration 'kvm.ha.webservice.port'
+     */
+    protected int getKvmHaMicroservicePortValue(Host host) {
+        Integer haAgentPort = KVMHAConfig.KvmHaWebservicePort.value();
+        if (haAgentPort == null) {
+            LOGGER.warn(String.format("Using default kvm.ha.webservice.port: %s as it was set to NULL for the cluster [id: %d] from %s.",
+                    KVMHAConfig.KvmHaWebservicePort.defaultValue(), host.getClusterId(), host));
+            haAgentPort = Integer.parseInt(KVMHAConfig.KvmHaWebservicePort.defaultValue());
+        }
+        return haAgentPort;
+    }
+
+    /**
+     * Lists VMs on host according to vm_instance DB table. The states considered for such listing are: 'Running', 'Stopping', 'Migrating'.
+     * <br>
+     * <br>
+     * Note that VMs on state 'Starting' are not common to be at the host, therefore this method does not list them.
+     * However, there is still a probability of a VM in 'Starting' state be already listed on the KVM via '$virsh list',
+     * but that's not likely and thus it is not relevant for this very context.
+     */
+    public List<VMInstanceVO> listVmsOnHost(Host host) {

Review comment:
       Maybe this method could be outside the client with the DAO and keep `countRunningVmsOnAgent` on it?

##########
File path: packaging/systemd/cloudstack-agent-ha-helper.service
##########
@@ -0,0 +1,36 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Do not modify this file as your changes will be lost in the next CSM update.
+# If you need to add specific dependencies to this service unit do it in the
+# /etc/systemd/system/cloudstack-management.service.d/ directory
+
+[Unit]
+Description=CloudStack Agent HA Helper
+Documentation=http://www.cloudstack.org/
+Requires=libvirtd.service
+After=libvirtd.service
+
+[Service]
+Type=simple
+EnvironmentFile=/etc/default/cloudstack-agent-ha-helper

Review comment:
       Hi @GabrielBrascher can you check this one as well?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org