Martin Mucha has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 11: (35 comments) answers http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java: Line 11: import org.ovirt.engine.core.common.businessentities.MacPool; Line 12: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 13: import org.ovirt.engine.core.dao.MacPoolDAO; Line 14: Line 15: public class AddMacPoolCommand extends CommandBase<MacPoolParameter> { > You should add audit logging to the command, and also probably override set setActionMessageParameters: I wasn't aware of this. I've "cargo-culted" my best having storage-pool as imprint, but as "deep magic programming" is involved in this, it's probably wrong. Please point me out each lurking error. AuditLogging — I'll do it in separate patch since, if I'm reading wiki correctly, this would require some db changes as well. Line 16: Line 17: private MacPoolDAO macPoolDao; Line 18: Line 19: public AddMacPoolCommand(MacPoolParameter parameters) { Line 28: } Line 29: Line 30: @Override Line 31: protected boolean canDoAction() { Line 32: final boolean wrongInput = getParameters() == null || > getParameters() == null couldn't happen in a command (it's filtered on high I'll check this up, but I think it's not validated, it just fails, probably causing http500. But I'm removing this check from here. Line 33: getParameters().getMacPool() == null; Line 34: Line 35: if (!super.canDoAction() || wrongInput) { Line 36: return false; Line 29: Line 30: @Override Line 31: protected boolean canDoAction() { Line 32: final boolean wrongInput = getParameters() == null || Line 33: getParameters().getMacPool() == null; > This should be a javax.validation check on the parameters class itself Done Line 34: Line 35: if (!super.canDoAction() || wrongInput) { Line 36: return false; Line 37: } Line 42: addCanDoActionMessage(VdcBllMessages.MAC_POOL_CANNOT_SET_DEFAULT_POOL_THIS_WAY); Line 43: return false; Line 44: } Line 45: Line 46: if (StringUtils.isEmpty(macPool.getName())) { > This should be a javax.validation check on the MacPool class itself (checke Done Line 47: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_NAME); Line 48: return false; Line 49: } Line 50: Line 47: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_NAME); Line 48: return false; Line 49: } Line 50: Line 51: if (macPool.getRanges() == null || macPool.getRanges().isEmpty()) { > These should also be a javax.validation checks on the MacPool class itself Done Line 52: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_RANGE); Line 53: return false; Line 54: } Line 55: Line 52: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_RANGE); Line 53: return false; Line 54: } Line 55: Line 56: return true; > You should also have validations on the MacRange class that check that the parameter checking EXCEPT checking of multicasts — done. multicast checking is pointless since we're trimming invalid ranges to valid ones. So invalid inputs will be fixed instead of rejected. Rejecting invalid inputs will defeat purpose of having code which 'fixes' invalid ranges. Check for unique name of MacPool in system — done. Line 57: } Line 58: Line 59: private MacPool getMacPool() { Line 60: return getParameters().getMacPool(); Line 61: } Line 62: Line 63: @Override Line 64: protected void executeCommand() { Line 65: this.macPoolDao = getDbFacade().getMacPoolDao(); > I don't think it's necessary to keep this in a field. Done Line 66: MacPool macPool = getMacPool(); Line 67: Line 68: macPoolDao.save(macPool); Line 69: Line 62: Line 63: @Override Line 64: protected void executeCommand() { Line 65: this.macPoolDao = getDbFacade().getMacPoolDao(); Line 66: MacPool macPool = getMacPool(); > No need to save in local variable maybe I do not understand. macPool is used many times; if you're trying to say, that I can reach it using getter, then why that would be better? (please don't start with premature optimization) Line 67: Line 68: macPoolDao.save(macPool); Line 69: Line 70: MacPoolPerDC.getInstance().createPool(macPool); Line 64: protected void executeCommand() { Line 65: this.macPoolDao = getDbFacade().getMacPoolDao(); Line 66: MacPool macPool = getMacPool(); Line 67: Line 68: macPoolDao.save(macPool); > You need to generate an ID for the new pool that was covered by dao formerly and Moti already fixed this error Line 69: Line 70: MacPoolPerDC.getInstance().createPool(macPool); Line 71: setSucceeded(true); Line 72: getReturnValue().setActionReturnValue(macPool.getId()); http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java: Line 12: import org.ovirt.engine.core.compat.Guid; Line 13: import org.ovirt.engine.core.dao.MacPoolDAO; Line 14: import org.ovirt.engine.core.utils.transaction.RollbackHandler; Line 15: Line 16: public class RemoveMacPoolCommand extends org.ovirt.engine.core.bll.CommandBase<IdParameters> implements RollbackHandler { > No need for FQCN for org.ovirt.engine.core.bll.CommandBase and also no need Done Line 17: Line 18: private MacPoolDAO macPoolDao; Line 19: private MacPool deletedMacPool; Line 20: Line 14: import org.ovirt.engine.core.utils.transaction.RollbackHandler; Line 15: Line 16: public class RemoveMacPoolCommand extends org.ovirt.engine.core.bll.CommandBase<IdParameters> implements RollbackHandler { Line 17: Line 18: private MacPoolDAO macPoolDao; > This should be a getter, no need to save in a field Done Line 19: private MacPool deletedMacPool; Line 20: Line 21: public RemoveMacPoolCommand(IdParameters parameters) { Line 22: super(parameters); Line 24: } Line 25: Line 26: @Override Line 27: protected void executeCommand() { Line 28: final Guid macPoolId = getParameters().getId(); > There's no need to save this inside a variable it's used twice. Code's cleaner and better this way. Line 29: Line 30: deletedMacPool = macPoolDao.get(getParameters().getId()); Line 31: macPoolDao.remove(macPoolId); Line 32: MacPoolPerDC.getInstance().removePool(macPoolId); Line 37: @Override Line 38: protected boolean canDoAction() { Line 39: final boolean wrongInput = getParameters() == null || Line 40: getParameters().getId() == null || Line 41: getParameters().getId().equals(Guid.Empty); > As in add command, no need for this to be here.. Done Line 42: Line 43: if (!super.canDoAction() || wrongInput) { Line 44: return false; Line 45: } Line 45: } Line 46: Line 47: final MacPoolDAO macPoolDao = getDbFacade().getMacPoolDao(); Line 48: final Guid macPoolId = getParameters().getId(); Line 49: final MacPool macPool = macPoolDao.get(macPoolId); > You should save this to the field and use it later in execute, it is guaran Done Line 50: final int dcUsageCount = macPoolDao.getDCUsageCount(macPoolId); Line 51: Line 52: if (macPool == null) { Line 53: addCanDoActionMessage(VdcBllMessages.MAC_POOL_DOES_NOT_EXIST); Line 46: Line 47: final MacPoolDAO macPoolDao = getDbFacade().getMacPoolDao(); Line 48: final Guid macPoolId = getParameters().getId(); Line 49: final MacPool macPool = macPoolDao.get(macPoolId); Line 50: final int dcUsageCount = macPoolDao.getDCUsageCount(macPoolId); > this call should be moved after the if block, so if the mac pool isn't exis Done Line 51: Line 52: if (macPool == null) { Line 53: addCanDoActionMessage(VdcBllMessages.MAC_POOL_DOES_NOT_EXIST); Line 54: return false; Line 53: addCanDoActionMessage(VdcBllMessages.MAC_POOL_DOES_NOT_EXIST); Line 54: return false; Line 55: } Line 56: Line 57: final boolean canBeRemoved = macPool.canBeRemoved() && dcUsageCount == 0; > I think each condition merits a separate error message. Done Line 58: if (!canBeRemoved) { Line 59: addCanDoActionMessage(VdcBllMessages.MAC_POOL_CANNOT_BE_REMOVED); Line 60: return false; Line 61: } else { Line 58: if (!canBeRemoved) { Line 59: addCanDoActionMessage(VdcBllMessages.MAC_POOL_CANNOT_BE_REMOVED); Line 60: return false; Line 61: } else { Line 62: return true; > You can look at command that use org.ovirt.engine.core.bll.CommandBase.vali Done Line 63: } Line 64: } Line 65: Line 66: @Override http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java: Line 12: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 13: import org.ovirt.engine.core.compat.Guid; Line 14: import org.ovirt.engine.core.dao.MacPoolDAO; Line 15: Line 16: public class UpdateMacPoolCommand extends CommandBase<MacPoolParameter> { > Same comments as in the add pool command Done Line 17: Line 18: private final MacPoolDAO macPoolDao; Line 19: private MacPool oldMacPool; Line 20: Line 14: import org.ovirt.engine.core.dao.MacPoolDAO; Line 15: Line 16: public class UpdateMacPoolCommand extends CommandBase<MacPoolParameter> { Line 17: Line 18: private final MacPoolDAO macPoolDao; > No need to save this as field Done Line 19: private MacPool oldMacPool; Line 20: Line 21: public UpdateMacPoolCommand(MacPoolParameter parameters) { Line 22: super(parameters); Line 26: @Override Line 27: protected boolean canDoAction() { Line 28: final boolean wrongInput = getParameters() == null || Line 29: getMacPool() == null || Line 30: getMacPoolId() == null; > As in add command, no need for this to be here.. Done Line 31: Line 32: if (!super.canDoAction() || wrongInput) { Line 33: return false; Line 34: } Line 32: if (!super.canDoAction() || wrongInput) { Line 33: return false; Line 34: } Line 35: Line 36: final MacPool macPool = getParameters().getMacPool(); > You should just use the getter (inlined) Done Line 37: if (StringUtils.isEmpty(macPool.getName())) { Line 38: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_NAME); Line 39: return false; Line 40: } Line 33: return false; Line 34: } Line 35: Line 36: final MacPool macPool = getParameters().getMacPool(); Line 37: if (StringUtils.isEmpty(macPool.getName())) { > As in add, this should be a javax.validation Done Line 38: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_NAME); Line 39: return false; Line 40: } Line 41: Line 38: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_NAME); Line 39: return false; Line 40: } Line 41: Line 42: if (macPool.getRanges() == null || macPool.getRanges().isEmpty()) { > As in add, this should be a javax.validation Done Line 43: addCanDoActionMessage(VdcBllMessages.MAC_POOL_MUST_HAVE_RANGE); Line 44: return false; Line 45: } Line 46: Line 44: return false; Line 45: } Line 46: Line 47: oldMacPool = macPoolDao.get(getMacPoolId()); Line 48: if (oldMacPool == null) { > This is same logic as in delete mac pool command, and can be extracted to c Done Line 49: addCanDoActionMessage(VdcBllMessages.MAC_POOL_DOES_NOT_EXIST); Line 50: return false; Line 51: } Line 52: Line 49: addCanDoActionMessage(VdcBllMessages.MAC_POOL_DOES_NOT_EXIST); Line 50: return false; Line 51: } Line 52: Line 53: return true; > Should also validate for this command the stuff you're missing in add comma Done Line 54: } Line 55: Line 56: private MacPool getMacPool() { Line 57: return getParameters().getMacPool(); http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcObjectType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcObjectType.java: Line 34: PROVIDER(24, "Provider"), Line 35: GlusterService(25, "GlusterService"), Line 36: ExternalTask(26, "ExternalTask"), Line 37: VnicProfile(27, "Vnic Profile"), Line 38: MacPool(28, "MacPool"); > Translation should probably be "MAC Pool" Done Line 39: Line 40: private int value; Line 41: private String vdcObjectTranslationVal; Line 42: private static final Map<Integer, VdcObjectType> map = new HashMap<Integer, VdcObjectType>(values().length); http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MacPoolParameter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/MacPoolParameter.java: Line 1: package org.ovirt.engine.core.common.action; Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.MacPool; Line 4: Line 5: public class MacPoolParameter extends VdcActionParametersBase{ > Makes more sense to name MacPoolParameter*s* as other paremeters classes ar Done Line 6: Line 7: private MacPool macPool; Line 8: Line 9: public MacPoolParameter() { http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 365: // Audit Log Line 366: RemoveAuditLogById(2100, false, QuotaDependency.NONE), Line 367: ClearAllDismissedAuditLogs(2101, false, QuotaDependency.NONE), Line 368: Line 369: SetDataOnSession(3000, false, QuotaDependency.NONE), > Would make sense to put it before this? a) caused by rebase b) do these numbers have any meaning so that any unique number isn't just fine? --> I'll change it to 310X, so the order is now fine. Line 370: Line 371: // Mac Pool Line 372: AddMacPool(2200, ActionGroup.CREATE_MAC_POOL, false, QuotaDependency.NONE), Line 373: UpdateMacPool(2201, ActionGroup.EDIT_MAC_POOL, false, QuotaDependency.NONE), http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ActionGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ActionGroup.java: Line 134: // affinity group CRUD commands Line 135: MANIPULATE_AFFINITY_GROUPS(1550, RoleType.ADMIN, true, ApplicationMode.VirtOnly), Line 136: Line 137: // MAC pool actions groups Line 138: CREATE_MAC_POOL(1660, RoleType.ADMIN, true, ApplicationMode.VirtOnly), > I would go with CONFIGURE_ENGINE currently, and add the correct permissions Done Line 139: EDIT_MAC_POOL(1661, RoleType.ADMIN, true, ApplicationMode.VirtOnly), Line 140: DELETE_MAC_POOL(1662, RoleType.ADMIN, true, ApplicationMode.VirtOnly); Line 141: Line 142: private int id; http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 363: MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES(ErrorType.CONFLICT), Line 364: MAC_POOL_CANNOT_BE_REMOVED(ErrorType.CONFLICT), Line 365: MAC_POOL_DOES_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 366: MAC_POOL_MUST_HAVE_NAME(ErrorType.BAD_PARAMETERS), Line 367: MAC_POOL_CANNOT_SET_DEFAULT_POOL_THIS_WAY(ErrorType.BAD_PARAMETERS), > NOT_SUPPORTED is more suitable Done Line 368: MAC_POOL_MUST_HAVE_RANGE(ErrorType.BAD_PARAMETERS), Line 369: MAC_POOL_TRYING_TO_REUSE_NON_SHARED_POOL(ErrorType.BAD_PARAMETERS), Line 370: VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL(ErrorType.CONFLICT), Line 371: VM_CLUSTER_IS_NOT_VALID(ErrorType.BAD_PARAMETERS), Line 365: MAC_POOL_DOES_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 366: MAC_POOL_MUST_HAVE_NAME(ErrorType.BAD_PARAMETERS), Line 367: MAC_POOL_CANNOT_SET_DEFAULT_POOL_THIS_WAY(ErrorType.BAD_PARAMETERS), Line 368: MAC_POOL_MUST_HAVE_RANGE(ErrorType.BAD_PARAMETERS), Line 369: MAC_POOL_TRYING_TO_REUSE_NON_SHARED_POOL(ErrorType.BAD_PARAMETERS), > Not sure this is used? Done Line 370: VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL(ErrorType.CONFLICT), Line 371: VM_CLUSTER_IS_NOT_VALID(ErrorType.BAD_PARAMETERS), Line 372: VM_CANNOT_REMOVE_VM_WHEN_STATUS_IS_NOT_DOWN(ErrorType.CONFLICT), Line 373: VM_CANNOT_REMOVE_WITH_DETACH_DISKS_SNAPSHOTS_EXIST(ErrorType.CONFLICT), http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java: Line 570: public StoragePoolDAO getStoragePoolDAO() { Line 571: return getDbFacade().getStoragePoolDao(); Line 572: } Line 573: Line 574: public MacPoolDao getMacPoolDAO() { > Should be D*ao*, also why not use this in the new commands if you added thi Done Line 575: return getDbFacade().getMacPoolDao(); Line 576: } Line 577: Line 578: public StorageDomainStaticDAO getStorageDomainStaticDAO() { http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 11: MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES=Not enough MAC addresses left in MAC Address Pool. Line 12: MAC_POOL_CANNOT_BE_REMOVED=Mac pool cannot be removed. Either it's used or it's default mac pool. Line 13: MAC_POOL_DOES_NOT_EXIST=Mac pool does not exist. Line 14: MAC_POOL_MUST_HAVE_NAME=Mac pool cannot be created. Shared MAC pools must have names. Line 15: MAC_POOL_MUST_HAVE_RANGE=MacPool must contain at least one mac range. > You should use the ACTION_TYPE standard, with the standard prefix "Cannot $ Done Line 16: MAC_POOL_TRYING_TO_REUSE_NON_SHARED_POOL=Trying to use non shared pool which is used by someone else Line 17: VMT_CANNOT_REMOVE_DETECTED_DERIVED_VM=Cannot delete Template. Template is being used by the following VMs: ${vmsList}. Line 18: VMT_CANNOT_REMOVE_BASE_WITH_VERSIONS=Cannot delete Base Template that has Template Versions, please first remove all Template Versions for this Template: ${versionsList}. Line 19: ACTION_TYPE_FAILED_DETECTED_DERIVED_DISKS=Cannot ${action} ${type}. The following Disk(s) are based on it: \n ${disksInfo}. Line 13: MAC_POOL_DOES_NOT_EXIST=Mac pool does not exist. Line 14: MAC_POOL_MUST_HAVE_NAME=Mac pool cannot be created. Shared MAC pools must have names. Line 15: MAC_POOL_MUST_HAVE_RANGE=MacPool must contain at least one mac range. Line 16: MAC_POOL_TRYING_TO_REUSE_NON_SHARED_POOL=Trying to use non shared pool which is used by someone else Line 17: VMT_CANNOT_REMOVE_DETECTED_DERIVED_VM=Cannot delete Template. Template is being used by the following VMs: ${vmsList}. > I think you're missing some translations here.. Done Line 18: VMT_CANNOT_REMOVE_BASE_WITH_VERSIONS=Cannot delete Base Template that has Template Versions, please first remove all Template Versions for this Template: ${versionsList}. Line 19: ACTION_TYPE_FAILED_DETECTED_DERIVED_DISKS=Cannot ${action} ${type}. The following Disk(s) are based on it: \n ${disksInfo}. Line 20: ACTION_TYPE_FAILED_DISKS_SNAPSHOTS_DONT_BELONG_TO_SAME_DISK="Cannot ${action} ${type}. The specified disk snapshots don't belong to the same Disk." Line 21: VMT_CANNOT_REMOVE_DOMAINS_LIST_MISMATCH=Cannot delete Template. The Template does not exist on the following Storage Domains: ${domainsList}.\nEither verify that Template exists on all Storage Domains listed on the domains list,\nor do not send domains list in order to delete all instances of the Template from the system. http://gerrit.ovirt.org/#/c/28705/11/frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: Line 8: IRS_NETWORK_ERROR=Storage Manager Service not responding. Line 9: IRS_PROTOCOL_ERROR=Storage Manager protocol error. Line 10: IRS_RESPONSE_ERROR=Storage Manager response error. Line 11: MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES=Not enough MAC addresses left in MAC Address Pool. Line 12: MAC_POOL_CANNOT_BE_REMOVED=Mac pool cannot be removed. Either it's used or it's default mac pool. > No need to edit this file since these are admin UI only operations. Done Line 13: MAC_POOL_DOES_NOT_EXIST=Mac pool does not exist. Line 14: MAC_POOL_MUST_HAVE_NAME=Mac pool cannot be created. Shared MAC pools must have names. Line 15: MAC_POOL_CANNOT_SET_DEFAULT_POOL_THIS_WAY=Do not specify default MAC pool on pool create/update. Use specific command to select default MAC pool. Line 16: MAC_POOL_MUST_HAVE_RANGE=MacPool must contain at least one mac range. -- To view, visit http://gerrit.ovirt.org/28705 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c0c3657e3e53699bcafa375befdce848b01a2f3 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
