Shireesh Anjal has posted comments on this change.
Change subject: engine: Validation added in Import Cluster
......................................................................
Patch Set 2: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersForImportQuery.java
Line 54: if (getVdsStaticDao().get(getParameters().getServerName()) !=
null) {
Line 55: getQueryReturnValue().setSucceeded(false);
Line 56:
getQueryReturnValue().setExceptionString(VdcBllMessages.SERVER_ALREADY_EXIST_IN_ANOTHER_CLUSTER.name());
Line 57: return;
Line 58: }
The method you are using to see if the server already exists, checks against
the vds_name field, which represents neither the hostname nor the ip address of
the server, but just a name assigned by the user in the UI.
You should be using getAllForHost() *and* getAllWithIpAddress() to check
existence of the server.
Line 59:
Line 60: SSHClient client = null;
Line 61:
Line 62: try {
Line 67:
Line 68: Map<String, String> serverFingerPrint =
extractServers(serversXml);
Line 69: if (serverFingerPrint == null) {
Line 70: getQueryReturnValue().setSucceeded(false);
Line 71:
getQueryReturnValue().setExceptionString(VdcBllMessages.SERVER_ALREADY_EXIST_IN_ANOTHER_CLUSTER.name());
If fingerprint is null, set exception saying server already exists? not very
readable.
It is better to perform the check for whether the server exists or not outside
of extractServers().
Line 72: return;
Line 73: }
Line 74: getQueryReturnValue().setReturnValue(serverFingerPrint);
Line 75: } catch (Exception e) {
Line 102: // Add the server only if the state is 3
Line 103: if (state == PEER_IN_CLUSTER) {
Line 104: String hostName =
XmlUtils.getTextValue(firstHostElement, HOST_NAME);
Line 105: // Check if any of the server in the peer list is
already part of some other cluster.
Line 106: if (getVdsStaticDao().get(hostName) != null) {
Same as the first comment.
Line 107: return null;
Line 108: }
Line 109: fingerprints.put(hostName,
getFingerprint(hostName));
Line 110: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQueryTest.java
Line 1
Line 2
Why is the Query class renamed with 'git mv', but not this one?
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 664: VDS_CANNOT_REMOVE_HOST_HAVING_GLUSTER_VOLUME,
Line 665: ACTION_TYPE_FAILED_NO_GLUSTER_HOST_TO_PEER_PROBE,
Line 666: MIGRATE_PAUSED_VM_IS_UNSUPPORTED,
Line 667: ACTION_TYPE_FAILED_SERVER_NAME_REQUIRED,
Line 668: SERVER_ALREADY_EXIST_IN_ANOTHER_CLUSTER,
SERVER_ALREADY_EXIST*S*_IN_ANOTHER_CLUSTER
Line 669:
Line 670: VM_INTERFACE_NOT_EXIST,
Line 671: ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_DEVICE,
Line 672: ACTION_TYPE_FAILED_IMPORT_CLONE_NOT_COLLAPSED,
--
To view, visit http://gerrit.ovirt.org/9784
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8323f2dcc4f278dfbc01f99ebe18b0f2bd0296ca
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
