Alon Bar-Lev has uploaded a new change for review.
Change subject: bootstrap: do not get unique id at canDoAction
......................................................................
bootstrap: do not get unique id at canDoAction
CURRENT IMPLEMENTATION
engine has duplicate complex vdsm logic to generate the vdsm id.
@Reloadable
@TypeConverterAttribute(String.class)
@DefaultValueAttribute(
"IDFILE=/etc/vdsm/vdsm.id; " +
"if [ -r \"${IDFILE}\" ]; then " +
"cat \"${IDFILE}\"; " +
"else " +
"UUID=\"$(" +
"dmidecode -s system-uuid 2> /dev/null | " +
"sed -e 's/.*Not.*//' " +
")\"; " +
"if [ -z \"${UUID}\" ]; then " +
"UUID=\"$(uuidgen 2> /dev/null)\" && " +
"mkdir -p \"$(dirname \"${IDFILE}\")\" && " +
"echo \"${UUID}\" > \"${IDFILE}\" && " +
"chmod 0644 \"${IDFILE}\"; " +
"fi; " +
"[ -n \"${UUID}\" ] && echo \"${UUID}\"; " +
"fi"
)
BootstrapNodeIDCommand(372),
The command is executed synchronously (UI wise) when host is added.
ASSUMPTIONS OF CURRENT IMPLEMENTATION
1. dmidecode exists out of the box in any distribution.
---> WRONG: fedora 17 has no, and other distributions may also lack.
2. host id is only dmidecode output.
---> WRONG: over time we saw that we need extra logic to keep the id
sane, especially when the hardware id does not exist or
malformed.
3. dmidecode utility is used for host id
---> WRONG: there are plans to make it more secure/robust using TPM,
which requires software at host to generate.
PROBLEMS IN CURRENT IMPLEMENTATION
1. if dmidecode utility is missing we cannot acquire host id before
performing bootstrap. The whole idea of bootstrap process is to take
vanilla distribution and install vdsm. dmidecode is missing from
vanilla, hence cannot be executed before bootstrap.
2. the logic of generating host id exists both in engine and vdsm, both
implementations need to synced, and kept synced between versions,
which in practice cannot be achieved. As there is too much static
noise (distribute on different channels, not be able to update all
IT components, etc...).
3. If host id generation method is changed, the engine implementation
should be changed as well, while engine should not really care how
vdsm maintain its identity.
NEW IMPLEMENTATION
Acquire vdsm id during bootstrap process, at the earliest. When all
dependencies are available.
If vdsm id is duplicate:
1. post an error to host event log.
2. set the state of the host to "install fail".
USER VISIBLE CHANGES
The following existing scenario:
1. user has host xxx.com with ip 1.1.1.1, he added this host using
xxx.com as host name.
2. user add new host, at the host field he *BY MISTAKE* writes 1.1.1.1.
3. the user is blocked from proceeding because of duplicate uuid between
xxx.com(existing) and 1.1.1.1(new).
4. the confused user fixes the host field to 1.1.1.2 and proceed.
Behaves as:
1. user has host xxx.com with ip 1.1.1.1, he added this host using
xxx.com as host name.
2. user add new host, at the host field he *BY MISTAKE* writes 1.1.1.1.
3. installation starts, and ends up with installation failed status on
the new host.
4. user sees the error message, and remove the host added by mistake.
REASONING
The above sequence of adding the same host with different name/address is not
common or frequent, and does not justify the need to duplicate vdsm logic into
engine, nor find a solution for distributions that locks the dmidecode utility
at their base system layout, nor to lock our-self to dmidecode utility.
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=875527
Change-Id: I0263dbae34aaa02c126c5ed1dc52a84f4f5e77f8
Signed-off-by: Alon Bar-Lev <alo...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
D
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstallHelper.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
4 files changed, 62 insertions(+), 180 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/59/9159/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
index a3ebb91..4a4719e 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
@@ -7,7 +7,6 @@
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang.StringUtils;
-import org.apache.commons.lang.exception.ExceptionUtils;
import org.ovirt.engine.core.bll.context.CommandContext;
import org.ovirt.engine.core.bll.context.CompensationContext;
import org.ovirt.engine.core.bll.job.ExecutionContext;
@@ -49,6 +48,7 @@
import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil;
import org.ovirt.engine.core.utils.transaction.TransactionMethod;
import org.ovirt.engine.core.utils.transaction.TransactionSupport;
+import org.ovirt.engine.core.utils.ssh.SSHClient;
@NonTransactiveCommandAttribute(forceCompensation = true)
public class AddVdsCommand<T extends AddVdsActionParameters> extends
VdsCommand<T> {
@@ -311,8 +311,10 @@
|| getParameters().getVdsStaticData().getport() >
65536) {
addCanDoActionMessage(VdcBllMessages.VDS_PORT_IS_NOT_LEGAL);
returnValue = false;
+ } else if
(!validateHostUniqueNameAndAddress(getParameters().getVdsStaticData())) {
+ returnValue = false;
} else {
- returnValue = returnValue && validateHostUniqueness(vds);
+ returnValue = returnValue && canConnect(vds);
}
}
}
@@ -337,45 +339,47 @@
return ClusterUtils.getInstance().hasServers(getVdsGroupId());
}
- public VdsInstallHelper getVdsInstallHelper() {
- return new VdsInstallHelper();
+ public SSHClient getSSHClient() {
+ return new SSHClient();
}
- private boolean validateHostUniqueness(VDS vds) {
- boolean retrunValue = true;
+ private boolean canConnect(VDS vds) {
+ boolean returnValue = true;
// execute the connectivity and id uniqueness validation for VDS type
hosts
if (vds.getvds_type() == VDSType.VDS && Config.<Boolean>
GetValue(ConfigValues.InstallVds)) {
- VdsInstallHelper installHelper = getVdsInstallHelper();
+ SSHClient sshclient = null;
try {
Long timeout =
TimeUnit.SECONDS.toMillis(Config.<Integer>
GetValue(ConfigValues.ConnectToServerTimeoutInSeconds));
- if (!installHelper.connectToServer(vds.gethost_name(),
getParameters().getRootPassword(), timeout)) {
-
addCanDoActionMessage(VdcBllMessages.VDS_CANNOT_CONNECT_TO_SERVER);
- retrunValue = false;
- } else {
- String serverUniqueId = installHelper.getServerUniqueId();
- if (serverUniqueId != null) {
- serverUniqueId = serverUniqueId.trim();
- }
- retrunValue = retrunValue && validateHostUniqueId(vds,
serverUniqueId);
- if(retrunValue) {
- vds.setUniqueId(serverUniqueId);
- }
- }
- } finally {
- try {
- installHelper.shutdown();
- } catch (Exception e) {
- log.errorFormat("Failed to terminate session with host {0}
with message: {1}",
- vds.getvds_name(),
- ExceptionUtils.getMessage(e));
- log.debug(e);
+
+ sshclient = getSSHClient();
+ sshclient.setHardTimeout(timeout);
+ sshclient.setSoftTimeout(timeout);
+ sshclient.setHost(vds.gethost_name());
+ sshclient.setUser("root");
+ sshclient.setPassword(getParameters().getRootPassword());
+ sshclient.connect();
+ sshclient.authenticate();
+ }
+ catch (Exception e) {
+ log.errorFormat(
+ "Failed to establish session with host {0}",
+ vds.getvds_name(),
+ e
+ );
+
+
addCanDoActionMessage(VdcBllMessages.VDS_CANNOT_CONNECT_TO_SERVER);
+ returnValue = false;
+ }
+ finally {
+ if (sshclient != null) {
+ sshclient.disconnect();
}
}
}
- return retrunValue &&
validateHostUniqueNameAndAddress(getParameters().getVdsStaticData());
+ return returnValue;
}
private boolean validateHostUniqueNameAndAddress(VdsStatic vdsStaticData) {
@@ -387,49 +391,6 @@
}
return !VdsHandler.isVdsExistForPendingOvirt(vdsStaticData,
getReturnValue().getCanDoActionMessages(), vdsForUniqueId);
- }
-
- /**
- * Validate if same unique-id associated with the given host exists in
other hosts.<br>
- * There should be up to one host with the same unique-id besides the new
host.<br>
- * If host typed oVirt has the same unique id, in status PendingApproval,
set that host-id<br>
- * on the parameters member of the command.
- *
- * @param vds
- * a new host to be added
- * @param serverUniqueId
- * a new host unique-id
- * @return true - if no host is associated with the unique-id, or if there
is a single oVirt node in PendingApproval
- * status, else - false.
- */
- private boolean validateHostUniqueId(VDS vds, String serverUniqueId) {
- boolean retrunValue = true;
- List<VDS> vdssByUniqueId =
VdsInstallHelper.getVdssByUniqueId(vds.getId(), serverUniqueId);
-
- if (!vdssByUniqueId.isEmpty()) {
- if (vdssByUniqueId.size() > 1) {
- StringBuilder sb = new StringBuilder();
- sb.append(vdssByUniqueId.get(0));
- for (int i = 1; i < vdssByUniqueId.size(); i++) {
- sb.append(",
").append(vdssByUniqueId.get(i).getvds_name());
- }
-
addCanDoActionMessage(VdcBllMessages.VDS_REGISTER_UNIQUE_ID_AMBIGUOUS);
- addCanDoActionMessage(String.format("$HostNameList %1$s",
sb.toString()));
- retrunValue = false;
- } else {
- VDS existedVds = vdssByUniqueId.get(0);
- if (vds.getvds_type() == VDSType.VDS
- && existedVds.getvds_type() == VDSType.oVirtNode
- && (existedVds.getstatus() ==
VDSStatus.PendingApproval)) {
- getParameters().setVdsForUniqueId(existedVds.getId());
- } else {
-
addCanDoActionMessage(VdcBllMessages.VDS_REGISTER_UNIQUE_ID_AMBIGUOUS);
- addCanDoActionMessage(String.format("$HostNameList %1$s",
existedVds.getvds_name()));
- retrunValue = false;
- }
- }
- }
- return retrunValue;
}
private boolean validateSingleHostAttachedToLocalStorage() {
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstallHelper.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstallHelper.java
deleted file mode 100644
index 941db80..0000000
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstallHelper.java
+++ /dev/null
@@ -1,98 +0,0 @@
-package org.ovirt.engine.core.bll;
-
-import java.util.List;
-
-import org.ovirt.engine.core.common.businessentities.VDS;
-import org.ovirt.engine.core.compat.Guid;
-import org.ovirt.engine.core.dal.dbbroker.DbFacade;
-import org.ovirt.engine.core.utils.hostinstall.IVdsInstallerCallback;
-import org.ovirt.engine.core.utils.hostinstall.VdsInstallerSSH;
-import org.ovirt.engine.core.utils.linq.LinqUtils;
-import org.ovirt.engine.core.utils.linq.Predicate;
-import org.ovirt.engine.core.common.config.Config;
-import org.ovirt.engine.core.common.config.ConfigValues;
-
-/**
- * Class designed to provide connectivity to host and retrieval of its
unique-id in order to validate host status before
- * starting the installation flow.
- */
-public class VdsInstallHelper{
-
- private VdsInstallerSSH wrapper;
- private SimpleCallback callback;
- private String server;
-
- public VdsInstallHelper() {
- callback = new SimpleCallback();
- wrapper = new VdsInstallerSSH();
- wrapper.setCallback(callback);
- }
-
- public boolean connectToServer(String server, String passwd, long timeout)
{
- this.server = server;
- return wrapper.connect(server, passwd, timeout);
- }
-
- public String getServerUniqueId() {
- wrapper.executeCommand(
- Config.<String> GetValue(ConfigValues.BootstrapNodeIDCommand)
- );
- String serverUniqueId = callback.serverUniqueId.trim();
- if (serverUniqueId.isEmpty()) {
- throw new RuntimeException(
- String.format(
- "Got empty unique id from host '%1$s'",
- server
- )
- );
- }
- return serverUniqueId;
- }
-
- public void shutdown() {
- wrapper.shutdown();
- wrapper = null;
- callback = null;
- }
-
- public static List<VDS> getVdssByUniqueId(final Guid vdsId, String
uniqueIdToCheck) {
- List<VDS> list =
DbFacade.getInstance().getVdsDao().getAllWithUniqueId(uniqueIdToCheck);
- return LinqUtils.filter(list, new Predicate<VDS>() {
- @Override
- public boolean eval(VDS vds) {
- return !vds.getId().equals(vdsId);
- }
- });
- }
-
- public static boolean isVdsUnique(final Guid vdsId, String
uniqueIdToCheck) {
- return getVdssByUniqueId(vdsId, uniqueIdToCheck).isEmpty();
- }
-
- private class SimpleCallback implements IVdsInstallerCallback {
-
- String serverUniqueId;
-
- @Override
- public void addError(String error) {
- }
-
- @Override
- public void addMessage(String message) {
- serverUniqueId = message;
- }
-
- @Override
- public void connected() {
- }
-
- @Override
- public void endTransfer() {
- }
-
- @Override
- public void failed(String error) {
- }
-
- }
-}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
index b61587f..fac007f 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
@@ -8,6 +8,7 @@
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.Date;
+import java.util.List;
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang.StringUtils;
@@ -31,6 +32,8 @@
import org.ovirt.engine.core.utils.hostinstall.VdsInstallerSSH;
import org.ovirt.engine.core.utils.log.Log;
import org.ovirt.engine.core.utils.log.LogFactory;
+import org.ovirt.engine.core.utils.linq.LinqUtils;
+import org.ovirt.engine.core.utils.linq.Predicate;
import org.ovirt.engine.core.utils.transaction.TransactionMethod;
import org.ovirt.engine.core.utils.transaction.TransactionSupport;
@@ -491,6 +494,20 @@
}
+ public static List<VDS> getVdssByUniqueId(final Guid vdsId, String
uniqueIdToCheck) {
+ List<VDS> list =
DbFacade.getInstance().getVdsDao().getAllWithUniqueId(uniqueIdToCheck);
+ return LinqUtils.filter(list, new Predicate<VDS>() {
+ @Override
+ public boolean eval(VDS vds) {
+ return !vds.getId().equals(vdsId);
+ }
+ });
+ }
+
+ public static boolean isVdsUnique(final Guid vdsId, String
uniqueIdToCheck) {
+ return getVdssByUniqueId(vdsId, uniqueIdToCheck).isEmpty();
+ }
+
private boolean InsertUniqueId(String message) {
String uniqueId = message == null ? "" : message.trim();
@@ -503,7 +520,7 @@
return false;
}
- if (VdsInstallHelper.isVdsUnique(_vds.getId(), uniqueId)) {
+ if (isVdsUnique(_vds.getId(), uniqueId)) {
log.infoFormat("Installation of {0}. Assigning unique id {1} to
Host. (Stage: {2})", _serverName, uniqueId,
getCurrentInstallStage());
_vds.setUniqueId(uniqueId);
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
index 61e3404..dd1c825 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/itests/BasicTestSetup.java
@@ -1,9 +1,8 @@
package org.ovirt.engine.core.itests;
-import static org.mockito.Matchers.anyLong;
-import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.doNothing;
import static org.ovirt.engine.core.itests.AbstractBackendTest.testSequence;
import java.util.ArrayList;
@@ -19,7 +18,6 @@
import org.ovirt.engine.core.bll.AddVdsCommand;
import org.ovirt.engine.core.bll.Backend;
import org.ovirt.engine.core.bll.CpuFlagsManagerHandler;
-import org.ovirt.engine.core.bll.VdsInstallHelper;
import org.ovirt.engine.core.bll.interfaces.BackendInternal;
import org.ovirt.engine.core.bll.session.SessionDataContainer;
import org.ovirt.engine.core.common.action.AddImageFromScratchParameters;
@@ -76,6 +74,7 @@
import org.ovirt.engine.core.compat.Guid;
import org.ovirt.engine.core.compat.Version;
import org.ovirt.engine.core.dal.dbbroker.DbFacade;
+import org.ovirt.engine.core.utils.ssh.SSHClient;
/**
* This class will create basic virtualization "lab" composed from all basic
entities, Host, Cluster, Storage,
@@ -101,7 +100,7 @@
private final BackendInternal backend;
@Mock
- private VdsInstallHelper vdsInstallHelper;
+ private SSHClient sshclient;
public BasicTestSetup(BackendInternal backend) {
MockitoAnnotations.initMocks(this);
@@ -284,7 +283,7 @@
Boolean isMLA = Config.<Boolean>
GetValue(ConfigValues.IsMultilevelAdministrationOn);
setIsMultiLevelAdministrationOn(Boolean.FALSE);
- mockVdsInstallerHelper();
+ mockSSHClient();
AddVdsCommand<AddVdsActionParameters> addVdsCommand =
createAddVdsCommand(addHostParams);
VdcReturnValueBase addHostAction = addVdsCommand.executeAction();
setIsMultiLevelAdministrationOn(isMLA);
@@ -302,13 +301,16 @@
private AddVdsCommand<AddVdsActionParameters>
createAddVdsCommand(AddVdsActionParameters addHostParams) {
AddVdsCommand<AddVdsActionParameters> spyVdsCommand =
spy(new AddVdsCommand<AddVdsActionParameters>(addHostParams));
- when(spyVdsCommand.getVdsInstallHelper()).thenReturn(vdsInstallHelper);
+ when(spyVdsCommand.getSSHClient()).thenReturn(sshclient);
return spyVdsCommand;
}
- private void mockVdsInstallerHelper() {
- when(vdsInstallHelper.connectToServer(anyString(), anyString(),
anyLong())).thenReturn(true);
- when(vdsInstallHelper.getServerUniqueId()).thenReturn("");
+ private void mockSSHClient() {
+ try {
+ doNothing().when(sshclient).connect();
+ doNothing().when(sshclient).authenticate();
+ }
+ catch(Exception e) {}
}
private void setIsMultiLevelAdministrationOn(Boolean isMLA) {
--
To view, visit http://gerrit.ovirt.org/9159
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0263dbae34aaa02c126c5ed1dc52a84f4f5e77f8
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
