Shireesh Anjal has posted comments on this change.
Change subject: gluster: Sync gluster service statuses
......................................................................
Patch Set 4: (12 inline comments)
Responses to comments in-line. New patch-set to follow incorporating Sahina's
comments.
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 135: select fn_db_add_config_value('GlusterRefreshRateLight', '5',
'general');
Line 136: select fn_db_add_config_value('GlusterRefreshRateHeavy', '300',
'general');
Line 137: select fn_db_add_config_value('GlusterServicesEnabled', 'false',
'3.0');
Line 138: select fn_db_add_config_value('GlusterServicesEnabled', 'false',
'3.1');
Line 139: select fn_db_add_config_value('GlusterServicesEnabled', 'false',
'3.2');
GlusterRefreshRateLight is an existing config which is being used for this new
feature also. The word "existing" appears in my commit message as well :).
You can see it just two lines above my changes here, with value of 5.
Current editing is for detecting whether the "GlusterServicesEnabled" feature
is supported in the oVirt version being used (Used in GlusterFeatureSupported).
So it is not supported till 3.2, and supported from 3.3 onwards.
Line 140: select fn_db_add_config_value('GlusterSupport', 'false', '3.0');
Line 141: select
fn_db_add_config_value('GlusterVolumeOptionGroupVirtValue','virt','general');
Line 142: select
fn_db_add_config_value('GlusterVolumeOptionOwnerUserVirtValue','36','general');
Line 143: select
fn_db_add_config_value('GlusterVolumeOptionOwnerGroupVirtValue','36','general');
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterServiceSyncJob.java
Line 109: for (Entry<ServiceType, GlusterServiceStatus> entry :
fetchedClusterServiceStatusMap.entrySet()) {
Line 110: ServiceType type = entry.getKey();
Line 111: GlusterServiceStatus status = entry.getValue();
Line 112:
Line 113: GlusterClusterService foundService =
existingClusterServiceMap.get(type);
ah.. thanks :) Done.
Line 114: if (foundService == null) {
Line 115: existingClusterServiceMap.put(type,
mapServiceTypeToCluster(cluster, type, status));
Line 116: } else if (foundService.getStatus() != status) {
Line 117: updateClusterServiceStatus(foundService, status);
Line 111: GlusterServiceStatus status = entry.getValue();
Line 112:
Line 113: GlusterClusterService foundService =
existingClusterServiceMap.get(type);
Line 114: if (foundService == null) {
Line 115: existingClusterServiceMap.put(type,
mapServiceTypeToCluster(cluster, type, status));
Done
Line 116: } else if (foundService.getStatus() != status) {
Line 117: updateClusterServiceStatus(foundService, status);
Line 118: }
Line 119: }
Line 120: }
Line 121:
Line 122: private Map<ServiceType, GlusterServiceStatus>
createFetchedClusterServiceStatusMap(List<Map<String, GlusterServiceStatus>>
serviceStatusMaps) {
Line 123: Map<ServiceType, GlusterServiceStatus>
fetchedClusterServiceStatusMap =
Line 124: new HashMap<ServiceType, GlusterServiceStatus>();
Done
Line 125: for (Entry<String, GlusterServiceStatus> entry :
mergeServiceStatusMaps(serviceStatusMaps).entrySet()) {
Line 126: String serviceName = entry.getKey();
Line 127: GlusterServiceStatus status = entry.getValue();
Line 128: ServiceType type =
getServiceNameMap().get(serviceName).getServiceType();
Line 130: GlusterServiceStatus foundStatus =
fetchedClusterServiceStatusMap.get(type);
Line 131: if (foundStatus == null) {
Line 132: fetchedClusterServiceStatusMap.put(type, status);
Line 133: } else {
Line 134: if (foundStatus != status) {
Done
Line 135: fetchedClusterServiceStatusMap.put(type,
GlusterServiceStatus.MIXED);
Line 136: }
Line 137: }
Line 138: }
Line 139: return fetchedClusterServiceStatusMap;
Line 140: }
Line 141:
Line 142: private Map<String, GlusterServiceStatus>
mergeServiceStatusMaps(List<Map<String, GlusterServiceStatus>>
serviceStatusMaps) {
Line 143: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Done
Line 144: for (Map<String, GlusterServiceStatus> pairResult :
serviceStatusMaps) {
Line 145: for (Entry<String, GlusterServiceStatus> entry :
pairResult.entrySet()) {
Line 146: String serviceName = entry.getKey();
Line 147: GlusterServiceStatus status = entry.getValue();
Line 178: */
Line 179: @SuppressWarnings({ "unchecked", "serial" })
Line 180: private Map<String, GlusterServiceStatus>
refreshServerServices(VDS server) {
Line 181: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Line 182:
Actually, even though the parameter of the acquireLock() api is called
clusterId as of now, it can accept any Guid and acquire a lock on it. I think
the lock related apis in GlusterJob should be refactored to rename the
parameter to entityId instead of clusterId. This can be done as a separate
refactoring patch.
Coming to the second point, we should lock at the server level, so that the
database updates that happen on start/stop service commands are synchronized
with the updates from this sync job. This is why I am passing server id to
acquireLock().
Line 183: acquireLock(server.getId());
Line 184: try {
Line 185: Map<Guid, GlusterServerService> existingServicesMap =
getExistingServices(server);
Line 186: List<GlusterServerService> servicesToUpdate = new
ArrayList<GlusterServerService>();
Line 193: "Updating statuses of all services on this
server to UNKNOWN.",
Line 194: server.getHostName(),
Line 195: returnValue.getVdsError().getMessage());
Line 196: logUtil.logServerMessage(server,
AuditLogType.GLUSTER_SERVICES_LIST_FAILED);
Line 197: return
updateStatusToUnknown(existingServicesMap.values());
existingServicesMap is created by calling getExistingServices(), which makes
sure that it can never be null. (First creates an empty map and populates it if
possible)
If the services are not installed, vdsm should return the service status of
NOT_INSTALLED, for which we have the corresponding enum available in
GlusterServiceStatus.
Line 198: }
Line 199:
Line 200: for (final GlusterServerService fetchedService :
(List<GlusterServerService>) returnValue.getReturnValue()) {
Line 201: serviceStatusMap.put(fetchedService.getServiceName(),
fetchedService.getStatus());
Line 239:
Line 240: private Map<String, GlusterServiceStatus>
updateStatusToUnknown(Collection<GlusterServerService> existingServices) {
Line 241: Map<String, GlusterServiceStatus> serviceStatusMap = new
HashMap<String, GlusterServiceStatus>();
Line 242:
Line 243: List<GlusterServerService> servicesToUpdate = new
ArrayList<GlusterServerService>();
Done
Line 244: for (GlusterServerService existingService : existingServices)
{
Line 245: existingService.setStatus(GlusterServiceStatus.UNKNOWN);
Line 246: servicesToUpdate.add(existingService);
Line 247: serviceStatusMap.put(existingService.getServiceName(),
existingService.getStatus());
Line 249:
Line 250: getGlusterServerServiceDao().updateAll(servicesToUpdate);
Line 251: return serviceStatusMap;
Line 252: }
Line 253:
Done
Line 254: private Map<Guid, GlusterServerService> getExistingServices(VDS
server) {
Line 255: List<GlusterServerService> existingServices =
getGlusterServerServiceDao().getByServerId(server.getId());
Line 256: Map<Guid, GlusterServerService> existingServicesMap = new
HashMap<Guid, GlusterServerService>();
Line 257: if (existingServices != null) {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterServiceSyncJobTest.java
Line 225: public void testRefreshGlusterServicesWithChanges() throws
Exception {
Line 226: mockWithChanges();
Line 227: syncJob.refreshGlusterServices();
Line 228: verifyWithChanges();
Line 229: }
Done
Line 230:
Line 231: private void verifyCommonCalls() {
Line 232: // all clusters fetched from db
Line 233: verify(clusterDao, times(1)).getAll();
Line 280:
Line 281: GlusterClusterService service =
(GlusterClusterService) argument;
Line 282: switch (service.getServiceType()) {
Line 283: case GLUSTER:
Line 284: return service.getStatus() ==
GlusterServiceStatus.STOPPED;
Yes - this check was wrong. The status will be MIXED for both service types,
and there should be 2 calls to clusterServiceDao.update() as against just 1
that I was verifying earlier. Have fixed this.
Line 285: case GLUSTER_SWIFT:
Line 286: return service.getStatus() ==
GlusterServiceStatus.MIXED;
Line 287: default:
Line 288: return false;
--
To view, visit http://gerrit.ovirt.org/14644
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I312cacf698cb3420e30d96c42a92959b257c4abd
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
