Allon Mureinik has posted comments on this change.
Change subject: core: check no running VMs use storage conn
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(15 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 102: }
Line 103:
Line 104: if (doDomainsUseConnection(newConnectionDetails) ||
doLunsUseConnection(newConnectionDetails)) {
Line 105: if (domains.size() == 1) {
Line 106: setStorageDomain(domains.get(0));
please rethink this - what does this domain represent?
what if there are several block domains depending on this connection?
Line 107: }
Line 108: else if (storageType.isFileDomain() && domains.size() >
0) {
Line 109: String domainNames = createDomainNamesList(domains);
Line 110: addCanDoActionMessage(String.format("$domainNames
%1$s", domainNames));
Line 138:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_DOMAINS_MAINTENANCE);
Line 139: }
Line 140: return isConnectionEditable;
Line 141: }
Line 142: else {
I'd unwrap the else
Line 143: if (luns != null && !luns.isEmpty()) {
Line 144: List<String> vmNames = new ArrayList();
Line 145: List<String> domainNames = new ArrayList();
Line 146: for (LUNs lun : luns) {
Line 139: }
Line 140: return isConnectionEditable;
Line 141: }
Line 142: else {
Line 143: if (luns != null && !luns.isEmpty()) {
use CollectionUtils.isEmpty(luns)
Line 144: List<String> vmNames = new ArrayList();
Line 145: List<String> domainNames = new ArrayList();
Line 146: for (LUNs lun : luns) {
Line 147: NGuid diskId = lun.getDiskId();
Line 141: }
Line 142: else {
Line 143: if (luns != null && !luns.isEmpty()) {
Line 144: List<String> vmNames = new ArrayList();
Line 145: List<String> domainNames = new ArrayList();
1. generics...
2. consider renaming to problematicVmNames or something of the sort
Line 146: for (LUNs lun : luns) {
Line 147: NGuid diskId = lun.getDiskId();
Line 148: if (diskId != null) {
Line 149: Map<Boolean, List<VM>> vmsMap =
getVmDAO().getForDisk(diskId.getValue());
Line 146: for (LUNs lun : luns) {
Line 147: NGuid diskId = lun.getDiskId();
Line 148: if (diskId != null) {
Line 149: Map<Boolean, List<VM>> vmsMap =
getVmDAO().getForDisk(diskId.getValue());
Line 150: List<VM> pluggedVms =
vmsMap.get(Boolean.TRUE);
consider adding VmDAO.getPluggedForDisk(diskId)
Line 151: if (pluggedVms != null &&
!pluggedVms.isEmpty()) {
Line 152: for (VM vm : pluggedVms) {
Line 153: if
(!vm.getStatus().equals(VMStatus.Down)) {
Line 154: vmNames.add(vm.getName());
Line 163: String domainName =
domain.getStorageName();
Line 164: domainNames.add(domainName);
Line 165: }
Line 166: else {
Line 167: if (domains == null) {
just initialize the list before the loop
Line 168: domains = new ArrayList();
Line 169: }
Line 170: domains.add(domain);
Line 171: }
Line 218: if (!hasConnectStorageSucceeded) {
Line 219: setSucceeded(false);
Line 220: VdcFault f = new VdcFault();
Line 221: f.setError(VdcBllErrors.StorageServerConnectionError);
Line 222: getReturnValue().setFault(f);
No need to fail the connection failed - also this can be moved to the end, as
it's only relevant to updating domain stats.
Line 223: return;
Line 224: }
Line 225: for (StorageDomain domain : domains) {
Line 226: map = getStoragePoolIsoMap(domain);
Line 223: return;
Line 224: }
Line 225: for (StorageDomain domain : domains) {
Line 226: map = getStoragePoolIsoMap(domain);
Line 227: changeStorageDomainStatusInTransaction(map,
StorageDomainStatus.Locked);
all of these can be done in the same transaction
Line 228: // connect to the new path
Line 229: // update info such as free space - because we
switched to a different server
Line 230: returnValueUpdatedStorageDomain =
getStatsForDomain(domain);
Line 231: if (returnValueUpdatedStorageDomain.getSucceeded()) {
Line 224: }
Line 225: for (StorageDomain domain : domains) {
Line 226: map = getStoragePoolIsoMap(domain);
Line 227: changeStorageDomainStatusInTransaction(map,
StorageDomainStatus.Locked);
Line 228: // connect to the new path
path is a file concept, please generalize
Line 229: // update info such as free space - because we
switched to a different server
Line 230: returnValueUpdatedStorageDomain =
getStatsForDomain(domain);
Line 231: if (returnValueUpdatedStorageDomain.getSucceeded()) {
Line 232: final StorageDomain updatedStorageDomain =
Line 233: (StorageDomain)
returnValueUpdatedStorageDomain.getReturnValue();
Line 234: executeInNewTransaction(new
TransactionMethod<Void>() {
Line 235: @Override
Line 236: public Void runInTransaction() {
Line 237:
getStorageDomainDynamicDao().update(updatedStorageDomain.getStorageDynamicData());
should also have compensation
Line 238: return null;
Line 239: }
Line 240: });
Line 241:
Line 241:
Line 242: }
Line 243: else {
Line 244: isUpdateSucceeded = false;
Line 245: break;
I don't want to fail the update connection if getting the storage stats fails.
perhaps add an audit log, but nothing more than that.
Line 246: }
Line 247: }
Line 248: }
Line 249: if(isUpdateSucceeded) {
Line 271: }
Line 272:
Line 273: protected boolean doLunsUseConnection(StorageServerConnections
connection) {
Line 274: luns =
getLunDao().getAllForStorageServerConnection(connection.getid());
Line 275: return !luns.isEmpty();
I'd protect it with if (luns != null), just in case
Line 276: }
Line 277:
Line 278: protected StoragePoolIsoMap getStoragePoolIsoMap(StorageDomain
storageDomain) {
Line 279: StoragePoolIsoMapId mapId = new
StoragePoolIsoMapId(storageDomain.getId(),
Line 354: return getDbFacade().getStoragePoolIsoMapDao();
Line 355: }
Line 356:
Line 357: @Override
Line 358: protected Map<String, Pair<String, String>> getExclusiveLocks() {
you should lock all the objects you refer to in CDA:
- domains
- lun disks
- vms plugged to those disks
this will leave us with an issue of someone adding a new lun disk on the same
target, but I think we can just file a BZ for it and handle later.
Line 359: Map<String, Pair<String, String>> locks = new HashMap<String,
Pair<String, String>>();
Line 360: domains =
getStorageDomainsByConnId(getParameters().getStorageServerConnection().getid());
Line 361: if (!domains.isEmpty() && domains.size() == 1) {
Line 362: setStorageDomain(domains.get(0));
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
Line 330: Guid storageDomainId = Guid.NewGuid();
Line 331: lun3.setStorageDomainId(storageDomainId);
Line 332: lun3.setvolume_group_id(Guid.NewGuid().toString());
Line 333: luns.add(lun3);
Line 334: Map<Boolean, List<VM>> vmsMap = new HashMap();
generics
Line 335: VM vm1 = new VM();
Line 336: vm1.setName("vm1");
Line 337: vm1.setStatus(VMStatus.Up);
Line 338: VM vm2 = new VM();
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 350: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_NOT_EMPTY=Cannot ${action}
${type}. Storage connection id is not empty.
Line 351: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 352:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS,
Posix, local or iSCSI data domains.
Line 353:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_DOMAINS_MAINTENANCE=Cannot
${action} ${type}. The data domains ${domainNames} should be in maintenance.
Line 354:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_RUNNING_VMS_AND_DOMAINS_MAINTENANCE=Cannot
${action} ${type}. The data domains ${domainNames} should be in maintenance,
and VMs ${vmNames} should be down..
s/../.
Line 355:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_RUNNING_VMS=Cannot
${action} ${type}. VMs ${vmNames} should be down.
Line 356:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Storage connection parameters are used by the following
storage domains : ${domainNames}.
Line 357:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are used by the following
storage domains : ${domainNames} and disks: ${diskNames}.
Line 358: ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_DISKS=Cannot
${action} ${type}. Storage connection parameters are used by the following
disks : ${diskNames}.
--
To view, visit http://gerrit.ovirt.org/16203
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I11fe22760952e1319654647eed661a7814675f64
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
