Shireesh Anjal has posted comments on this change.
Change subject: engine: Refresh gluster data periodically
......................................................................
Patch Set 14: (4 inline comments)
Hi Moti,
Thanks for pointing our an interesting issue. I've explained my thinking and
suggested an approach to tackle this in-line. Next patch-set will also include
the new test case for the newly introduced SP.
~Shireesh
....................................................
File backend/manager/dbscripts/network_sp.sql
Line 310: AS $procedure$
Line 311: BEGIN
Line 312: RETURN QUERY SELECT *
Line 313: FROM vds_interface_view
Line 314: WHERE addr = v_addr;
I kept this SP generic to return all interfaces having a given ipaddress.
Within the java code (GlusterVolumesListReturnForXmlRpc#getServer()), I have
logic to pick up only the server that belongs to the appropriate cluster.
It might be possible to have multiple servers to have same ip address in case
of a pure virt cluster. However it just won't work in case of a gluster
cluster. Let us look at two scanarios:
1) The server was peer probed (gluster terminology for add host) using fully
qualified domain name: In this case, the glusterVolumesList verb will also
return the FQDN, and we can find it using getVdsDao().getAllForHostname().
There is no need to even try to fetch the hosts by ip address. So no problem
here.
2) The server was peer probed using ip address. The peer probe will succeed
only if the ip address is resolvable from the existing nodes of the cluster. If
another host with same ip address already exists in the cluster, it cannot be
added to a gluster cluster at all (peer probe command will fail). So there is
no way we can end up with multiple servers with same ip address inside a
gluster cluster.
There is one rare scenario, however, where we have a problem. Consider this:
- You have two servers who have same ip address on one of their interfaces
- User wants to add both these in a gluster+virt cluster, from webadmin UI
- First host added successfully to the cluster (using hostname)
- User tries to add the second host
- Host gets added to engine DB. In InitVdsOnUpCommand, a gluster peer probe of
this server will be attempted, which will fail
- So this server goes to non-operational state
- Such servers are automatically removed by GlusterManager in case of
gluster-only clusters, but *not* in case of virt+gluster clusters
- Effectively, we now have two servers in engine DB who have same ip address
I plan to change the code as follows to tackle this:
- Change the SP to fetch all network interfaces having given ip address *in the
given cluster*
- If this SP returns more than one rows, throw an exception and do not update
the bricks of the current volume being processed. This will make sure that we
don't end up wrongly changing host id of an existing brick or inserting a new
brick with the wrong host.
Let me know if you have any objections to any of this, or have a better
suggestion.
Line 315:
Line 316: END; $procedure$
Line 317: LANGUAGE plpgsql;
Line 318:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterManager.java
Line 259:
Line 260: private List<String> getVdsIps(VDS vds) {
Line 261: List<String> vdsIps = new ArrayList<String>();
Line 262: for (VdsNetworkInterface iface :
getInterfaceDao().getAllInterfacesForVds(vds.getId())) {
Line 263: vdsIps.add(iface.getAddress());
It will not result in false positives, because server.getHostnameOrIp() will
never be null. This comes from the hostname/ip address returned by the 'gluster
volume info' command, which can never be empty or null. So while the list of ip
addresses of the host interfaces might contain null, what we are looking for in
this list will never be null, and hence it should be ok.
Having said this, I think it is still a good suggestion to filter out null
values, and will do so in the next patch-set.
Line 264: }
Line 265: return vdsIps;
Line 266: }
Line 267:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/InterfaceDAODbFacadeImpl.java
Line 130: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource()
Line 131: .addValue("addr", ipAddress);
Line 132:
Line 133: return
getCallsHandler().executeReadList("Getinterface_viewByAddr",
Line 134: vdsNetworkInterfaceRowMapper, parameterSource);
Done
Line 135: }
Line 136:
Line 137: @SuppressWarnings("unchecked")
Line 138: @Override
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumesListReturnForXmlRpc.java
Line 170: if(servers.size() > 0) {
Line 171: return getServerOfCluster(clusterId, servers);
Line 172: }
Line 173:
Line 174: List<VdsNetworkInterface> ifaces =
getInterfaceDao().getAllInterfacesWithIpAddress(hostnameOrIp);
Have responded to the corresponding comment. Will change this piece of code
based on the conclusion of that discussion.
Line 175: if(ifaces.size() > 0) {
Line 176: for(VdsNetworkInterface iface : ifaces) {
Line 177: VDS server = getVdsDao().get(iface.getVdsId());
Line 178: if(server.getvds_group_id().equals(clusterId)) {
--
To view, visit http://gerrit.ovirt.org/7288
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6b61eb6e93105e46e2706eac1d94bc10717224c2
Gerrit-PatchSet: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
