Shireesh Anjal has posted comments on this change.
Change subject: engine: fetch gluster host uuid during InitVdsOnUp
......................................................................
Patch Set 3: (6 inline comments)
Apart from few comments for Kanagaraj, I've also responded to some comments
from Omer, Yair and Michael.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitVdsOnUpCommand.java
Line 79: if (vdsGroup.supportsVirtService()) {
Line 80: setSucceeded(initVirtResources());
Line 81: }
Line 82:
Line 83: if (vdsGroup.supportsGlusterService()) {
I know this is not part of your patch, but I think we should do this only if
initVirtResources() had succeeded (in gluster+virt mode). So, what I suggest
here is,
if (getSucceeded() && vdsGroup.supportsGlusterService()) {
setSucceeded(initGlusterHost());
}
Line 84: setSucceeded(initGlusterHost());
Line 85: }
Line 86: }
Line 87:
Line 236: public AuditLogType getAuditLogTypeValue() {
Line 237: AuditLogType type = AuditLogType.UNASSIGNED;
Line 238:
Line 239: if (getVdsGroup().supportsGlusterService()) {
Line 240: if (!_glusterHostUuidFound) {
If you incorporate my suggestion of not even trying to perform
initGlusterHost() when the virt initialization has failed (in virt+gluster
mode), then _glusterHostUuidFound can still be false here, which might result
in an incorrect audit log type being returned.
So I suggest to reverse the sequence here - first check and return virt related
errors, and then proceed to gluster ones.
Line 241: type = AuditLogType.GLUSTER_HOST_UUID_NOT_FOUND;
Line 242: } else if (!_glusterPeerListSucceeded) {
Line 243: type = AuditLogType.GLUSTER_SERVERS_LIST_FAILED;
Line 244: } else if (!_glusterPeerProbeSucceeded) {
Line 256: } else if (getVds().getpm_enabled() && !_fenceSucceeded) {
Line 257: type = AuditLogType.VDS_FENCE_STATUS_FAILED;
Line 258: }
Line 259:
Line 260: // PM alerts
No, we're hiding all power management related functionality when in
gluster-only mode. It is more related to fencing for virt hosts, and the
functionality is not required in gluster-only mode.
Line 261: AuditLogableBase logable = new
AuditLogableBase(getVds().getId());
Line 262: if (getVds().getpm_enabled()) {
Line 263: if (!_vdsProxyFound) {
Line 264: logable.addCustomValue("Reason",
Line 323: }
Line 324:
Line 325: private boolean hostExists(List<GlusterServerInfo>
glusterServers, VDS server) {
Line 326: if
(GlusterFeatureSupported.glusterHostUuidSupported(getVdsGroup().getcompatibility_version()))
{
Line 327: VdsGluster vdsGluster =
DbFacade.getInstance().getVdsGlusterDao().getById(getVds().getId());
Instead of getVds().getId(), you can use server.getId()
Line 328: if (vdsGluster != null) {
Line 329: for (GlusterServerInfo glusterServer :
glusterServers) {
Line 330: if
(glusterServer.getUuid().equals(vdsGluster.getGlusterHostUuid())) {
Line 331: return true;
Line 338: if
(glusterServer.getHostnameOrIp().equals(server.getHostName())) {
Line 339: return true;
Line 340: }
Line 341: for (VdsNetworkInterface vdsNwInterface :
getVdsInterfaces(server.getId())) {
Line 342: if
(glusterServer.getHostnameOrIp().equals(vdsNwInterface.getAddress())) {
Again, I know this is existing code and not part of your patch, but changing
this as follows will make it better:
String glusterHostAddr =
InetAddress.getByName(glusterServer.getHostnameOrIp()).getHostAddress();
for (VdsNetworkInterface vdsNwInterface : getVdsInterfaces(server.getId())) {
if (glusterHostAddr.equals(vdsNwInterface.getAddress())) {
return true;
}
}
Basically, we're resolving the given "hostnameOrIp" to an IP address before
comparing it with IP addresses of all the network interfaces.
Line 343: return true;
Line 344: }
Line 345: }
Line 346: }
Line 350:
Line 351: private boolean saveGlusterHostUuid() {
Line 352: VdsGlusterDao vdsGlusterDao =
DbFacade.getInstance().getVdsGlusterDao();
Line 353: VdsGluster vdsGluster =
vdsGlusterDao.getById(getVds().getId());
Line 354: if (vdsGluster == null) {
Not so in case of gluster related VDS commands that extend from
AbstractGlusterBrokerCommand. We handle all gluster-vdsm errors in
AbstractGlusterBrokerCommand#ProceedProxyReturnValue() and set the succeeded
flag to false.
Hence returnValue.getSucceeded() *will* return false if the VDS command failed.
Line 355: VDSReturnValue returnValue =
runVdsCommand(VDSCommandType.GetGlusterHostUUID,
Line 356: new
VdsIdVDSCommandParametersBase(getVds().getId()));
Line 357: if (returnValue.getSucceeded() &&
returnValue.getReturnValue() != null) {
Line 358: vdsGluster = new VdsGluster();
--
To view, visit http://gerrit.ovirt.org/14166
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied8e8b006adc5722cd70ee19d75a4b6f783d5f44
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
