Liron Aravot has posted comments on this change. Change subject: core: Limitation addition for downgrading DC's compatible version. ......................................................................
Patch Set 12: (7 comments) reviewed the command only http://gerrit.ovirt.org/#/c/35790/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java: Line 213: StoragePoolValidator validator = createStoragePoolValidator(); Line 214: return validate(validator.isNotLocalfsWithDefaultCluster()); Line 215: } Line 216: Line 217: private boolean isDowngradeAllowedForDomains(List<StorageDomainStatic> poolDomains) { this method is not aware that it's being called only for downgrade :) i'd remove the downgrade from the name Line 218: List<String> formatProblematicDomains = new ArrayList<>(); Line 219: List<String> typeProblematicDomains = new ArrayList<>(); Line 220: boolean failOnSupportedTypeMixing = false; Line 221: Line 220: boolean failOnSupportedTypeMixing = false; Line 221: Line 222: for (StorageDomainStatic domainStatic : poolDomains) { Line 223: StorageDomain domain = new StorageDomain(); Line 224: domain.setStorageStaticData(domainStatic); I think that this part is extremely breakable- We can't assume that creating a dummy domain is enough for the validator, that's relying on the internal implementation. what if tommorow we'll check there another property of the storage domain? our validation result here won't be reilable. we should load the domain properly with all it's data and then check it (or support validating it only by static data). Line 225: Line 226: StorageDomainToPoolRelationValidator attachDomainValidator = getAttachDomainValidator(domain); Line 227: Line 228: if (!failOnSupportedTypeMixing && !attachDomainValidator.isStorageDomainTypeFitsPoolIfMixed().isValid()) { Line 224: domain.setStorageStaticData(domainStatic); Line 225: Line 226: StorageDomainToPoolRelationValidator attachDomainValidator = getAttachDomainValidator(domain); Line 227: Line 228: if (!failOnSupportedTypeMixing && !attachDomainValidator.isStorageDomainTypeFitsPoolIfMixed().isValid()) { you can remove the failOnSupportedTypeMixing check from the if condition, no cost in setting it again to true. Line 229: failOnSupportedTypeMixing = true; Line 230: } Line 231: if (!attachDomainValidator.isStorageDomainTypeSupportedInPool().isValid()) { Line 232: typeProblematicDomains.add(domain.getName()); Line 235: formatProblematicDomains.add(domain.getName()); Line 236: } Line 237: } Line 238: Line 239: int numberOfProblematicDomainsToShow = 10; why 10? Line 240: if (failOnSupportedTypeMixing) { Line 241: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_MIXED_STORAGE_TYPES_NOT_ALLOWED); Line 242: } Line 243: if (!formatProblematicDomains.isEmpty()) { Line 236: } Line 237: } Line 238: Line 239: int numberOfProblematicDomainsToShow = 10; Line 240: if (failOnSupportedTypeMixing) { on what cases do we show multiple messages of different checks together? not sure about the uniformity of that with the rest of the system Line 241: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_MIXED_STORAGE_TYPES_NOT_ALLOWED); Line 242: } Line 243: if (!formatProblematicDomains.isEmpty()) { Line 244: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DECREASING_COMPATIBILITY_VERSION_CAUSES_STORAGE_FORMAT_DOWNGRADING); Line 245: getReturnValue().getCanDoActionMessages().addAll(ReplacementUtils.replaceWith("formatDowngradedDomains", formatProblematicDomains, "," , numberOfProblematicDomainsToShow)); Line 246: } Line 247: if (!typeProblematicDomains.isEmpty()) { Line 248: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAINS_ARE_NOT_SUPPORTET_IN_DOWNGRADED_VERSION); Line 249: getReturnValue().getCanDoActionMessages().addAll(ReplacementUtils.replaceWith("unsupportedVersionDomains", typeProblematicDomains , ",", numberOfProblematicDomainsToShow)); consider export this part to a method to ease the read of this method.. Line 250: } Line 251: Line 252: return (typeProblematicDomains.isEmpty() && formatProblematicDomains.isEmpty() && !failOnSupportedTypeMixing); Line 253: } Line 248: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAINS_ARE_NOT_SUPPORTET_IN_DOWNGRADED_VERSION); Line 249: getReturnValue().getCanDoActionMessages().addAll(ReplacementUtils.replaceWith("unsupportedVersionDomains", typeProblematicDomains , ",", numberOfProblematicDomainsToShow)); Line 250: } Line 251: Line 252: return (typeProblematicDomains.isEmpty() && formatProblematicDomains.isEmpty() && !failOnSupportedTypeMixing); you can remove the outer parenthesis Line 253: } Line 254: Line 255: protected StorageDomainToPoolRelationValidator getAttachDomainValidator(StorageDomain domain) { Line 256: return new StorageDomainToPoolRelationValidator(domain, getStoragePool()); -- To view, visit http://gerrit.ovirt.org/35790 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib97488c705a32fe73b390ef226b1ad19a3784b76 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
