Mike Kolesnik has posted comments on this change.
Change subject: engine: Modify Vm Interface commands to work with VmNic
......................................................................
Patch Set 22:
(5 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java
Line 194: if (getInterface() != null &&
getInterface().getVnicProfileId() != null && getVm() != null) {
Line 195:
Line 196: VnicProfile profile =
getVnicProfileDao().get(getInterface().getVnicProfileId());
Line 197:
Line 198: if (profile != null && profile.isPortMirroring()) {
I'm not sure why is there a distinction here, I thought the ability to set port
mirroring was on the profile so the check for PORT_MIRRORING action group
should happen there, no?
Line 199: permissionList.add(new
PermissionSubject(profile.getId(),
Line 200: VdcObjectType.VnicProfile,
Line 201: ActionGroup.PORT_MIRRORING));
Line 202: } else {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java
Line 87: public Void runInTransaction() {
Line 88: getCompensationContext().snapshotEntity(oldIface);
Line 89:
getCompensationContext().snapshotEntity(oldVmDevice);
Line 90: getVmNicDao().update(getInterface());
Line 91: getDbFacade().getVmDeviceDao().update(oldVmDevice);
So what did change in the VM device? Seems like nothing changed so should we
snapshot/update it even?
Line 92: getCompensationContext().stateChanged();
Line 93: return null;
Line 94: }
Line 95: });
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
Line 444: public static VmDevice addNetworkInterfaceDevice(VmDeviceId id,
boolean plugged, Guid vnicProfileId) {
Line 445: Map<String, String> customProperties = null;
Line 446: if (vnicProfileId != null) {
Line 447: VnicProfile profile =
DbFacade.getInstance().getVnicProfileDao().get(vnicProfileId);
Line 448: customProperties = profile == null ? null :
profile.getCustomProperties();
This design doesn't make sense to me..
You should use the properties from the profile always, there's no need to copy
them to each NIC that uses that profile..
If we will have redundant data we will have to sync it and that doesn't sound
like the design here.
Instead, simply have the VM/Device handling code that needs the properties take
them directly from the profile..
In the future if we would want to allow the user to specify properties per NIC
he can do it and that data solely will be saved on the VM device, and will take
precedence over the data in the profile.
Line 449: }
Line 450:
Line 451: return addManagedDevice(id,
Line 452: VmDeviceGeneralType.INTERFACE,
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugNicVDSCommand.java
Line 43: Network network = null;
Line 44: if (nic.getVnicProfileId() != null) {
Line 45: vnicProfile =
getDbFacade().getVnicProfileDao().get(nic.getVnicProfileId());
Line 46: if (vnicProfile != null) {
Line 47: network =
getDbFacade().getNetworkDao().get(vnicProfile.getNetworkId());
Perhaps it makes more sense to put this code on the BLL side, since it's not
really a concern of this command, and just pass it the profile & network?
Line 48: }
Line 49: }
Line 50: map.put(VdsProperties.Type, vmDevice.getType().getValue());
Line 51: map.put(VdsProperties.Device, VmDeviceType.BRIDGE.getName());
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/UpdateVmInterfaceVDSCommand.java
Line 29:
Line 30: VmNic nic = getParameters().getNic();
Line 31: VnicProfile vnicProfile = null;
Line 32: Network network = null;
Line 33: if (nic.getVnicProfileId() != null) {
Same thought as in the hot plug command
Line 34: vnicProfile =
getDbFacade().getVnicProfileDao().get(nic.getVnicProfileId());
Line 35: if (vnicProfile != null) {
Line 36: network =
getDbFacade().getNetworkDao().get(vnicProfile.getNetworkId());
Line 37: }
--
To view, visit http://gerrit.ovirt.org/17229
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I92480426f2e1d49d19a1859d4ef1008699568402
Gerrit-PatchSet: 22
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
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
