Hello Shmuel Melamud,
I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/40132
to review the following change.
Change subject: core: Save RNG device when VM is running
......................................................................
core: Save RNG device when VM is running
Added missing functionality to detect changes of RNG device properties
when VM is running and to save/restore them to/from NEXT_RUN snapshot.
For this purpose Optional<T> class was added to
VmManagementParametersBase. This class combines data for the device that
needs to be updated with a flag that switches the update on/off. This
change makes possible to use @EditableDeviceOnVmStatusField annotation
for rngDevice field and to combine rngDevice update with other device
updates in a single list. Optional<T> class may be used as well for
other fields in VmManagementParametersBase that use the same pattern.
Watchdog is one of them.
Change-Id: If9aa41b0cacd77120b002123d8e196e215e20fdc
Signed-off-by: Shmuel Melamud <[email protected]>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceCommonUtils.java
M
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
M
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfTemplateWriter.java
M
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfVmWriter.java
M
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/instancetypes/InstanceTypeManager.java
9 files changed, 150 insertions(+), 50 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/32/40132/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java
index 89de6e5..9ff3b55 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessDownVmCommand.java
@@ -22,6 +22,7 @@
import org.ovirt.engine.core.common.businessentities.VmPayload;
import org.ovirt.engine.core.common.businessentities.VmPool;
import org.ovirt.engine.core.common.businessentities.VmPoolType;
+import org.ovirt.engine.core.common.businessentities.VmRngDevice;
import org.ovirt.engine.core.common.businessentities.VmWatchdog;
import org.ovirt.engine.core.common.businessentities.aaa.DbUser;
import org.ovirt.engine.core.common.errors.VdcBllMessages;
@@ -184,6 +185,7 @@
updateVmParams.setBalloonEnabled(false);
updateVmParams.setVirtioScsiEnabled(false);
updateVmParams.setClearPayload(true);
+ updateVmParams.setUpdateRngDevice(true);
for (GraphicsType graphicsType : GraphicsType.values()) {
updateVmParams.getGraphicsDevices().put(graphicsType, null);
}
@@ -212,6 +214,9 @@
case CONSOLE:
updateVmParams.setConsoleEnabled(true);
break;
+ case RNG:
+ updateVmParams.setRngDevice(new VmRngDevice(device));
+ break;
case GRAPHICS:
updateVmParams.getGraphicsDevices().put(GraphicsType.fromString(device.getDevice()),
new GraphicsDevice(device));
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
index 69dc3f4..ad36f9f 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
@@ -848,6 +848,11 @@
Object value = field.get(objectWithEditableDeviceFields);
if (value instanceof Boolean) {
addDeviceUpdateOnNextRun(vmId, annotation, null, value,
fieldList);
+ } else if (value instanceof
VmManagementParametersBase.Optional) {
+ VmManagementParametersBase.Optional<?> optional =
(VmManagementParametersBase.Optional<?>) value;
+ if (optional.isUpdate()) {
+ addDeviceUpdateOnNextRun(vmId, annotation, null,
optional.getValue(), fieldList);
+ }
} else if (value instanceof Map) {
Map<?, ?> map = (Map<?, ?>) value;
for (Map.Entry<?, ?> entry : map.entrySet()) {
@@ -864,8 +869,6 @@
log.warn("VmHandler::getVmDevicesFieldsToUpdateOnNextRun:
Reflection error");
log.debug("Original exception was:", e);
}
-
-
}
return fieldList;
@@ -873,8 +876,12 @@
private static boolean addDeviceUpdateOnNextRun(Guid vmId,
EditableDeviceOnVmStatusField annotation,
Object key, Object value,
List<VmDeviceUpdate> updates) {
- VmDeviceGeneralType generalType = annotation.generalType();
- VmDeviceType type = annotation.type();
+ return addDeviceUpdateOnNextRun(vmId, annotation.generalType(),
annotation.type(), annotation.isReadOnly(),
+ key, value, updates);
+ }
+
+ private static boolean addDeviceUpdateOnNextRun(Guid vmId,
VmDeviceGeneralType generalType, VmDeviceType type,
+ boolean readOnly, Object key, Object value,
List<VmDeviceUpdate> updates) {
if (key != null) {
VmDeviceGeneralType keyGeneralType = VmDeviceGeneralType.UNKNOWN;
@@ -910,15 +917,15 @@
if (value == null) {
if (VmDeviceUtils.vmDeviceChanged(vmId, generalType, typeName,
false)) {
- updates.add(new VmDeviceUpdate(generalType, type,
annotation.isReadOnly(), false));
+ updates.add(new VmDeviceUpdate(generalType, type, readOnly,
false));
}
} else if (value instanceof Boolean) {
if (VmDeviceUtils.vmDeviceChanged(vmId, generalType, typeName,
(Boolean) value)) {
- updates.add(new VmDeviceUpdate(annotation, (Boolean) value));
+ updates.add(new VmDeviceUpdate(generalType, type, readOnly,
(Boolean) value));
}
} else if (value instanceof VmDevice) {
- if (VmDeviceUtils.vmDeviceChanged(vmId, generalType, typeName,
true)) {
- updates.add(new VmDeviceUpdate(generalType, type,
annotation.isReadOnly(), (VmDevice) value));
+ if (VmDeviceUtils.vmDeviceChanged(vmId, generalType, typeName,
(VmDevice) value)) {
+ updates.add(new VmDeviceUpdate(generalType, type, readOnly,
(VmDevice) value));
}
} else {
log.warn("VmHandler::addDeviceUpdateOnNextRun: Unsupported value
type: " +
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
index e75a409..31531fe 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
@@ -1140,10 +1140,42 @@
return deviceEnabled == vmDevices.isEmpty();
}
+ /**
+ * Determines whether a VM device change has been request by the user.
+ * @param deviceGeneralType VmDeviceGeneralType.
+ * @param deviceTypeName VmDeviceType name.
+ * @param device device object provided by user
+ * @return true if a change has been requested; otherwise, false
+ */
+ public static boolean vmDeviceChanged(Guid vmId, VmDeviceGeneralType
deviceGeneralType, String deviceTypeName,
+ VmDevice device) {
+ List<VmDevice> vmDevices = deviceTypeName != null ?
+ dao.getVmDeviceByVmIdTypeAndDevice(vmId, deviceGeneralType,
deviceTypeName):
+ dao.getVmDeviceByVmIdAndType(vmId, deviceGeneralType);
+
+ if (device == null)
+ return !vmDevices.isEmpty();
+ if (device != null && vmDevices.isEmpty())
+ return true;
+ if (device.getSpecParams() != null) { // if device.getSpecParams() ==
null, it is not used for comparison
+ for (VmDevice vmDevice : vmDevices) {
+ if (!vmDevice.getSpecParams().equals(device.getSpecParams())) {
+ return true;
+ }
+ }
+ }
+
+ return false;
+ }
+
public static boolean vmDeviceChanged(Guid vmId, VmDeviceGeneralType
deviceType, boolean deviceEnabled) {
return vmDeviceChanged(vmId, deviceType, null, deviceEnabled);
}
+ public static boolean vmDeviceChanged(Guid vmId, VmDeviceGeneralType
deviceType, VmDevice device) {
+ return vmDeviceChanged(vmId, deviceType, null, device);
+ }
+
/**
* Returns a map (device ID to VmDevice) of devices that are relevant for
next run by examining
* properties that are annotated by EditableDeviceOnVmStatusField.
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
index b457b43..4e23a8b 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
@@ -1,5 +1,6 @@
package org.ovirt.engine.core.common.action;
+import java.io.Serializable;
import java.util.HashMap;
import java.util.Map;
import javax.validation.Valid;
@@ -21,6 +22,41 @@
private static final long serialVersionUID = -7695630335738521510L;
+ /**
+ * This class combines a value and update flag. If update flag is false,
the value is not used to update the VM.
+ * This is used to maintain backward compatibility in REST API: when null
value comes from REST API it doesn't mean
+ * the value must be cleaned in the VM. REST API has separate commands to
update values marked as Optional<T> here.
+ *
+ * @param <T> type of the value
+ */
+ public static class Optional<T> implements Serializable {
+
+ private static final long serialVersionUID = 2456445711176920294L;
+
+ private boolean update;
+ private T value;
+
+ public Optional() {
+ }
+
+ public boolean isUpdate() {
+ return update;
+ }
+
+ public void setUpdate(boolean update) {
+ this.update = update;
+ }
+
+ public T getValue() {
+ return value;
+ }
+
+ public void setValue(T value) {
+ this.value = value;
+ }
+
+ }
+
@Valid
private VmStatic vmStatic;
private boolean makeCreatorExplicitOwner;
@@ -30,24 +66,16 @@
private boolean clearPayload;
private Boolean balloonEnabled;
private VM vm;
- private VmWatchdog watchdog;
- private VmRngDevice rngDevice;
private boolean copyTemplatePermissions;
private boolean applyChangesLater;
private boolean updateNuma;
- /*
- * This parameter is needed at update to make sure that when we get a null
watchdog from rest-api it is not meant to
- * be removing the watchdog, rest-api will simply call watchdog commands
directly. Default is false so to avoid
- * breaking rest-api.
- */
- private boolean updateWatchdog;
- /*
- * This parameter is used to decide if to create sound device or not if it
is null then: for add vm legacy logic
- * will be used: create device for desktop type for update the current
configuration will remain
- * Used by rest-api.
- */
- private boolean updateRngDevice;
+
+ private Optional<VmWatchdog> watchdog = new Optional<>();
+
+ @EditableDeviceOnVmStatusField(generalType = VmDeviceGeneralType.RNG, type
= VmDeviceType.VIRTIO)
+ private Optional<VmRngDevice> rngDevice = new Optional<>();
+
/*
* This parameter is used to decide if to create sound device or not
* if it is null then:
@@ -56,6 +84,7 @@
*/
@EditableDeviceOnVmStatusField(generalType = VmDeviceGeneralType.SOUND,
type = VmDeviceType.UNKNOWN, isReadOnly = true)
private Boolean soundDeviceEnabled;
+
/*
* This parameter is used to decide if to create console device or not if
it is null then: for add vm don't add
* console device for update the current configuration will remain
@@ -183,38 +212,38 @@
}
public VmWatchdog getWatchdog() {
- return watchdog;
+ return watchdog.getValue();
}
public void setWatchdog(VmWatchdog watchdog) {
- this.watchdog = watchdog;
+ this.watchdog.setValue(watchdog);
}
public VmRngDevice getRngDevice() {
- return rngDevice;
+ return rngDevice.getValue();
}
public void setRngDevice(VmRngDevice rngDevice) {
- this.rngDevice = rngDevice;
- if (this.rngDevice != null) {
- this.rngDevice.setVmId(getVmId());
+ this.rngDevice.setValue(rngDevice);
+ if (this.rngDevice.getValue() != null) {
+ this.rngDevice.getValue().setVmId(getVmId());
}
}
public boolean isUpdateRngDevice() {
- return updateRngDevice;
+ return rngDevice.isUpdate();
}
public void setUpdateRngDevice(boolean updateRngDevice) {
- this.updateRngDevice = updateRngDevice;
+ this.rngDevice.setUpdate(updateRngDevice);
}
public boolean isUpdateWatchdog() {
- return updateWatchdog;
+ return watchdog.isUpdate();
}
public void setUpdateWatchdog(boolean updateWatchdog) {
- this.updateWatchdog = updateWatchdog;
+ this.watchdog.setUpdate(updateWatchdog);
}
public Boolean isConsoleEnabled() {
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceCommonUtils.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceCommonUtils.java
index b679b6f..7d8f424 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceCommonUtils.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceCommonUtils.java
@@ -370,6 +370,7 @@
|| (VmDeviceType.SPICEVMC.getName().equals(device) &&
VmDeviceGeneralType.REDIR == type)
|| (VmDeviceType.MEMBALLOON.getName().equals(device) &&
VmDeviceGeneralType.BALLOON == type))
|| (VmDeviceType.WATCHDOG.getName().equals(device) &&
VmDeviceGeneralType.WATCHDOG == type)
+ || (VmDeviceType.VIRTIO.getName().equals(device) &&
VmDeviceGeneralType.RNG == type)
|| (VmDeviceType.VIRTIOSERIAL.getName().equals(device) &&
VmDeviceGeneralType.CONTROLLER == type)
|| (VmDeviceType.VIRTIOSCSI.getName().equals(device) &&
VmDeviceGeneralType.CONTROLLER == type);
}
diff --git
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
index 790eb4d..ef1ce15 100644
---
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
+++
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
@@ -214,7 +214,6 @@
* Reads vm device attributes from OVF and stores it in the collection
*
* @param node
- * @param vmBase
* @param deviceId
*/
private VmDevice readVmDevice(XmlNode node, Guid deviceId) {
diff --git
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfTemplateWriter.java
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfTemplateWriter.java
index b49dd46..d593401 100644
---
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfTemplateWriter.java
+++
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfTemplateWriter.java
@@ -250,7 +250,7 @@
// monitors
writeMonitors(_vmTemplate);
// graphics
- writeGraphics(vmBase);
+ writeGraphics(_vmTemplate);
// CD
writeCd(_vmTemplate);
// ummanged devices
diff --git
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfVmWriter.java
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfVmWriter.java
index 1b041de..5d8bb41 100644
---
a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfVmWriter.java
+++
b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfVmWriter.java
@@ -344,7 +344,7 @@
writeGraphics(vmBase);
// CD
writeCd(vmBase);
- // ummanged devices
+ // unmanaged devices
writeOtherDevices(vmBase, _writer);
// End hardware section
diff --git
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/instancetypes/InstanceTypeManager.java
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/instancetypes/InstanceTypeManager.java
index 3ed8392..ee994f5 100644
---
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/instancetypes/InstanceTypeManager.java
+++
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/instancetypes/InstanceTypeManager.java
@@ -9,6 +9,7 @@
import org.ovirt.engine.core.common.businessentities.MigrationSupport;
import org.ovirt.engine.core.common.businessentities.VmBase;
import org.ovirt.engine.core.common.businessentities.VmDevice;
+import org.ovirt.engine.core.common.businessentities.VmDeviceGeneralType;
import org.ovirt.engine.core.common.businessentities.VmEntityType;
import org.ovirt.engine.core.common.businessentities.VmRngDevice;
import org.ovirt.engine.core.common.businessentities.VmTemplate;
@@ -266,8 +267,23 @@
return null;
}
+ private VmDevice findVmDeviceByGeneralType(Map<Guid, VmDevice>
vmManagedDeviceMap,
+ VmDeviceGeneralType
generalType) {
+ for (VmDevice vmDevice : vmManagedDeviceMap.values()) {
+ if (vmDevice.getType() == generalType) {
+ return vmDevice;
+ }
+ }
+
+ return null;
+ }
+
private boolean isVmDeviceExists(Map<Guid, VmDevice> vmManagedDeviceMap,
String name) {
return findVmDeviceByName(vmManagedDeviceMap, name) != null;
+ }
+
+ private boolean isVmDeviceExists(Map<Guid, VmDevice> vmManagedDeviceMap,
VmDeviceGeneralType generalType) {
+ return findVmDeviceByGeneralType(vmManagedDeviceMap, generalType) !=
null;
}
protected void doUpdateManagedFieldsFrom(final VmBase vmBase) {
@@ -401,23 +417,34 @@
protected void updateRngDevice(final VmBase vmBase) {
if (model.getIsRngEnabled().getIsChangable() &&
model.getIsRngEnabled().getIsAvailable()) {
- Frontend.getInstance().runQuery(VdcQueryType.GetRngDevice, new
IdQueryParameters(vmBase.getId()), new AsyncQuery(
- this,
- new INewAsyncCallback() {
- @Override
- public void onSuccess(Object model, Object
returnValue) {
- deactivate();
- List<VmDevice> rngDevices = ((VdcQueryReturnValue)
returnValue).getReturnValue();
-
getModel().getIsRngEnabled().setEntity(!rngDevices.isEmpty());
- if (!rngDevices.isEmpty()) {
- VmRngDevice rngDevice = new
VmRngDevice(rngDevices.get(0));
- getModel().setRngDevice(rngDevice);
+ if (!isNextRunConfigurationExists()) {
+ Frontend.getInstance().runQuery(VdcQueryType.GetRngDevice, new
IdQueryParameters(vmBase.getId()), new AsyncQuery(
+ this,
+ new INewAsyncCallback() {
+ @Override
+ public void onSuccess(Object model, Object
returnValue) {
+ deactivate();
+ List<VmDevice> rngDevices =
((VdcQueryReturnValue) returnValue).getReturnValue();
+
getModel().getIsRngEnabled().setEntity(!rngDevices.isEmpty());
+ if (!rngDevices.isEmpty()) {
+ VmRngDevice rngDevice = new
VmRngDevice(rngDevices.get(0));
+ getModel().setRngDevice(rngDevice);
+ }
+ activate();
+ updateVirtioScsi(vmBase);
}
- activate();
- updateVirtioScsi(vmBase);
}
- }
- ));
+ ));
+ } else {
+ deactivate();
+ VmDevice rngDevice =
findVmDeviceByGeneralType(vmBase.getManagedDeviceMap(),
VmDeviceGeneralType.RNG);
+ getModel().getIsRngEnabled().setEntity(rngDevice != null);
+ if (rngDevice != null) {
+ getModel().setRngDevice(new VmRngDevice(rngDevice));
+ }
+ activate();
+ updateVirtioScsi(vmBase);
+ }
} else {
updateVirtioScsi(vmBase);
}
--
To view, visit https://gerrit.ovirt.org/40132
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If9aa41b0cacd77120b002123d8e196e215e20fdc
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <[email protected]>
Gerrit-Reviewer: Shmuel Melamud <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
