Shireesh Anjal has uploaded a new change for review.
Change subject: gluster: Add host: Improved gluster validation [2]
......................................................................
gluster: Add host: Improved gluster validation [2]
Currently we don't allow to add a host if it has peers. However, if one
(or more) of those peers are already part of the cluster in engine DB,
then it means that these have been added from gluster CLI and hence the
validation should not fail in such cases.
Change-Id: I36b3b6d00a71af50ef1b006e95daebfbfe3f67f5
Signed-off-by: Shireesh Anjal <san...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVdsCommandTest.java
M
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/gluster/GlusterUtil.java
M
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/gluster/GlusterUtilTest.java
4 files changed, 61 insertions(+), 19 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/42/14242/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 68d8100..a5f84a8 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
@@ -5,6 +5,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.concurrent.TimeUnit;
import javax.naming.AuthenticationException;
@@ -50,6 +51,7 @@
import org.ovirt.engine.core.dal.VdcBllMessages;
import org.ovirt.engine.core.dal.dbbroker.DbFacade;
import org.ovirt.engine.core.dal.job.ExecutionMessageDirector;
+import org.ovirt.engine.core.dao.gluster.GlusterDBUtils;
import org.ovirt.engine.core.utils.gluster.GlusterUtil;
import org.ovirt.engine.core.utils.ssh.SSHClient;
import org.ovirt.engine.core.utils.threadpool.ThreadPoolUtil;
@@ -383,7 +385,7 @@
sshclient.connect();
sshclient.authenticate();
- return isValidGlusterPeer(sshclient);
+ return isValidGlusterPeer(sshclient, vds.getVdsGroupId());
} catch (AuthenticationException e) {
log.errorFormat(
"Failed to authenticate session with host {0}",
@@ -411,19 +413,33 @@
/**
* Checks if the server can be a valid gluster peer. Fails if it is
already part of another cluster (when current
- * cluster is not empty). This is done by executing the 'gluster peer
status' command on the server.<br>
- * Note: In case glusterd is down or not installed on the server (which is
a possibility on a new server), the
- * command can fail. In such cases, we just log it as a debug message and
return true.
+ * cluster is not empty). This is done by executing the 'gluster peer
status' command on the server.<p>
+ * In case glusterd is down or not installed on the server (which is a
possibility on a new server), the command can
+ * fail. In such cases, we just log it as a debug message and return
true.<p>
+ * Another interesting case is where one of the peers of the server is
already present as part of this cluster in
+ * the engine DB. This means that one ore more hosts were probably added
to the gluster cluster using gluster CLI,
+ * and hence this server should be allowed to be added.
*
* @param sshclient
* SSH client that can be used to execute 'gluster peer status'
command on the server
+ * @param clusterId
+ * ID of the cluster to which the server is being added.
* @return true if the server is good to be added to a gluster cluster,
else false.
*/
- private boolean isValidGlusterPeer(SSHClient sshclient) {
+ private boolean isValidGlusterPeer(SSHClient sshclient, Guid clusterId) {
if (isGlusterSupportEnabled() && clusterHasServers()) {
try {
// Must not allow adding a server that already is part of
another gluster cluster
- if (getGlusterUtil().hasPeers(sshclient)) {
+ Set<String> peers = getGlusterUtil().getPeers(sshclient);
+ if (peers.size() > 0) {
+ for(String peer : peers) {
+ if(getGlusterDBUtils().serverExists(clusterId, peer)) {
+ // peer present in cluster. so server being added
is valid.
+ return true;
+ }
+ }
+
+ // none of the peers present in the cluster. fail with
appropriate error.
return
failCanDoAction(VdcBllMessages.SERVER_ALREADY_PART_OF_ANOTHER_CLUSTER);
}
} catch (Exception e) {
@@ -437,6 +453,10 @@
return true;
}
+ protected GlusterDBUtils getGlusterDBUtils() {
+ return GlusterDBUtils.getInstance();
+ }
+
protected GlusterUtil getGlusterUtil() {
return GlusterUtil.getInstance();
}
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVdsCommandTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVdsCommandTest.java
index 93d98d9..cf13de5 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVdsCommandTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVdsCommandTest.java
@@ -3,9 +3,12 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.when;
+import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.doCallRealMethod;
import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
import org.junit.Before;
import org.junit.ClassRule;
@@ -23,6 +26,7 @@
import org.ovirt.engine.core.dal.VdcBllMessages;
import org.ovirt.engine.core.dao.VdsDAO;
import org.ovirt.engine.core.dao.VdsGroupDAO;
+import org.ovirt.engine.core.dao.gluster.GlusterDBUtils;
import org.ovirt.engine.core.utils.MockConfigRule;
import org.ovirt.engine.core.utils.gluster.GlusterUtil;
import org.ovirt.engine.core.utils.log.Log;
@@ -30,6 +34,7 @@
@RunWith(MockitoJUnitRunner.class)
public class AddVdsCommandTest {
+ private static final String PEER_1 = "peer1";
private static final Guid vdsId = Guid.NewGuid();
private AddVdsActionParameters parameters;
@@ -43,6 +48,8 @@
private ClusterUtils clusterUtils;
@Mock
private GlusterUtil glusterUtil;
+ @Mock
+ private GlusterDBUtils glusterDBUtils;
@Mock
private SSHClient sshClient;
@Mock
@@ -96,6 +103,7 @@
setupCommonMock(false);
}
+ @SuppressWarnings("unchecked")
private void setupGlusterMock(boolean clusterHasServers, VDS upServer,
boolean hasPeers) throws Exception {
setupCommonMock(true);
@@ -104,7 +112,10 @@
doCallRealMethod().when(commandMock).addCanDoActionMessage(any(VdcBllMessages.class));
when(commandMock.getGlusterUtil()).thenReturn(glusterUtil);
- when(glusterUtil.hasPeers(any(SSHClient.class))).thenReturn(hasPeers);
+ when(glusterUtil.getPeers(any(SSHClient.class))).thenReturn(hasPeers ?
Collections.singleton(PEER_1)
+ : Collections.EMPTY_SET);
+
+ when(commandMock.getGlusterDBUtils()).thenReturn(glusterDBUtils);
when(clusterUtils.hasServers(any(Guid.class))).thenReturn(clusterHasServers);
when(clusterUtils.getUpServer(any(Guid.class))).thenReturn(upServer);
@@ -127,13 +138,16 @@
@Test
public void canDoActionSucceedsWhenHasPeersThrowsException() throws
Exception {
setupGlusterMock(true, new VDS(), true);
- when(glusterUtil.hasPeers(any(SSHClient.class))).thenThrow(new
RuntimeException());
+ when(glusterUtil.getPeers(any(SSHClient.class))).thenThrow(new
RuntimeException());
+
assertTrue(commandMock.canDoAction());
}
@Test
public void canDoActionFailsWhenGlusterServerHasPeers() throws Exception {
setupGlusterMock(true, new VDS(), true);
+ when(glusterDBUtils.serverExists(any(Guid.class),
eq(PEER_1))).thenReturn(false);
+
assertFalse(commandMock.canDoAction());
assertTrue(commandMock.getReturnValue()
.getCanDoActionMessages()
@@ -141,6 +155,14 @@
}
@Test
+ public void canDoActionSucceedsWhenGlusterServerHasPeersThatExistInDB()
throws Exception {
+ setupGlusterMock(true, new VDS(), true);
+ when(glusterDBUtils.serverExists(any(Guid.class),
eq(PEER_1))).thenReturn(true);
+
+ assertTrue(commandMock.canDoAction());
+ }
+
+ @Test
public void canDoActionSucceedsWhenGlusterServerHasNoPeers() throws
Exception {
setupGlusterMock(true, new VDS(), false);
assertTrue(commandMock.canDoAction());
diff --git
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/gluster/GlusterUtil.java
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/gluster/GlusterUtil.java
index 9c68173..748fcc2 100644
---
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/gluster/GlusterUtil.java
+++
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/gluster/GlusterUtil.java
@@ -68,17 +68,17 @@
}
/**
- * Given an SSHClient (already connected and authenticated), execute the
"gluster peer status" command, check if the
- * server has any peers and return true if it has at least one. Note that
this method does <b>not</b> close the
- * connection, and it is the responsibility of the calling code to do the
same.
+ * Given an SSHClient (already connected and authenticated), execute the
"gluster peer status" command, and return
+ * the set of the peers returned by the command. Note that this method
does <b>not</b> close the connection, and it
+ * is the responsibility of the calling code to do the same.
*
* @param client
* The already connected and authenticated SSHClient object
- * @return true if the server has at least one peer, else false
+ * @return Set of peers of the server
*/
- public boolean hasPeers(SSHClient client) {
+ public Set<String> getPeers(SSHClient client) {
String serversXml = executePeerStatusCommand(client);
- return extractServers(serversXml).size() > 0;
+ return extractServers(serversXml);
}
/**
diff --git
a/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/gluster/GlusterUtilTest.java
b/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/gluster/GlusterUtilTest.java
index cdd57ca..ac09698 100644
---
a/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/gluster/GlusterUtilTest.java
+++
b/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/gluster/GlusterUtilTest.java
@@ -3,9 +3,8 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.assertFalse;
-import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import java.util.Map;
@@ -82,12 +81,13 @@
@Test
public void testHasPeersTrue() {
- assertTrue(glusterUtil.hasPeers(client));
+ assertNotNull(glusterUtil.getPeers(client));
+ assertEquals(2, glusterUtil.getPeers(client).size());
}
@Test
public void testHasPeersFalse() {
doReturn(OUTPUT_XML_NO_PEERS).when(glusterUtil).executePeerStatusCommand(client);
- assertFalse(glusterUtil.hasPeers(client));
+ assertTrue(glusterUtil.getPeers(client).isEmpty());
}
}
--
To view, visit http://gerrit.ovirt.org/14242
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I36b3b6d00a71af50ef1b006e95daebfbfe3f67f5
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <san...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
