webadmin: Refactor NUMA for VM

With recent changes in the VM NUMA area the behavior of the NUMA count and the NUMA support button was not consistent and sometimes the changes were not saved on the backend at all. This patch refactors the source code so that: - The numaEnabled field has been removed from the UnitVmModel because after the recent changes only the NUMA support button was affected by its value. Instead, we enable and disable directly the NUMA support action. - the NUMA support was renamed to NUMA pinning (in the code, not UI message) as we now enable NUMA by setting the numaCount - the updateNuma parameter sent to the backend is now calculated in the behaviors. For the new VM, it is sent if the numa count > 0. For existing VMs, there is a series of checks to determine if the NUMA should be updated on the backend. As a result, the initialVmNumaNodes has been moved from UnitVmModel to the ExistingVmModelBehavior because that is the only place where it is used. - added support from configuring NUMA when creating a new VM from a template page Bug-Url: https://bugzilla.redhat.com/2099225
oVirt · Jun 27, 2022 · 2f64015 · 2f64015
1 parent 9347171
commit 2f64015
Show file tree

Hide file tree

Showing 12 changed files with 110 additions and 104 deletions.
diff --git a/...src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java b/...src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java
@@ -119,6 +119,7 @@
 import org.ovirt.engine.ui.uicommonweb.models.vms.key_value.KeyValueModel;
 import org.ovirt.engine.ui.uicompat.EnumTranslator;
 import org.ovirt.engine.ui.uicompat.PropertyChangedEventArgs;
+import org.ovirt.engine.ui.uicompat.UIConstants;
 
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.event.dom.client.ClickEvent;
@@ -1087,6 +1088,8 @@ interface ViewUiBinder extends UiBinder<DialogTabPanel, AbstractVmPopupWidget> {
     private static final CommonApplicationConstants constants = AssetProvider.getConstants();
     private static final CommonApplicationMessages messages = AssetProvider.getMessages();
 
+    private static final UIConstants uiConstants = GWT.create(UIConstants.class);
+
     private final Map<TabName, OvirtTabListItem> tabMap = new HashMap<>();
 
     public AbstractVmPopupWidget() {
@@ -1352,7 +1355,7 @@ protected void initPoolSpecificWidgets() {
         editPoolMaxAssignedVmsPerUserIcon =
                 new InfoIcon(templates.italicText(messages.maxAssignedVmsPerUserHelp()));
 
-        numaInfoIcon = new InfoIcon(SafeHtmlUtils.fromTrustedString("")); //$NON-NLS-1$
+        numaInfoIcon = new InfoIcon(SafeHtmlUtils.fromTrustedString(uiConstants.numaInfoMessage()));
 
         isRngEnabledInfoIcon = new InfoIcon(SafeHtmlUtils.fromTrustedString(constants.rngDevExplanation()));
 
@@ -1733,11 +1736,8 @@ private <T> void initClusterDefaultValueListener(ClusterDefaultRenderer<T> rende
     private void enableNumaSupport(final UnitVmModel model) {
         numaSupportButton.setCommand(model.getNumaSupportCommand());
         numaPanel.setVisible(false);
-        enableNumaFields(false);
-        model.getNumaEnabled().getEntityChangedEvent().addListener((ev, sender, args) -> {
+        model.getNumaSupportCommand().getPropertyChangedEvent().addListener((ev, sender, args) -> {
             numaPanel.setVisible(true);
-            enableNumaFields(model.getNumaEnabled().getEntity());
-            setNumaInfoMsg(model.getNumaEnabled().getMessage());
         });
     }
 
@@ -1747,17 +1747,6 @@ private void localizeSafeHtmlFields() {
         detachableMinAllocatedMemoryEditor.setExplanation(SafeHtmlUtils.fromString(constants.physMemGuarInfoIcon()));
     }
 
-    private void setNumaInfoMsg(String message) {
-        if (message == null) {
-            message = ""; //$NON-NLS-1$
-        }
-        numaInfoIcon.setText(multiLineItalicSafeHtml(message));
-    }
-
-    private void enableNumaFields(boolean enabled) {
-        numaSupportButton.setEnabled(enabled);
-    }
-
     @UiHandler("refreshButton")
     void handleRefreshButtonClick(ClickEvent event) {
         unitVmModel.getBehavior().refreshCdImages();

diff --git a/.../main/java/org/ovirt/engine/ui/uicommonweb/builders/vm/CpuPinningVmBaseToUnitBuilder.java b/.../main/java/org/ovirt/engine/ui/uicommonweb/builders/vm/CpuPinningVmBaseToUnitBuilder.java
@@ -9,6 +9,7 @@ public class CpuPinningVmBaseToUnitBuilder extends BaseSyncBuilder<VmBase, UnitV
 
     @Override
     protected void build(VmBase vm, UnitVmModel model) {
+        model.setOriginalCpuPinningPolicy(vm.getCpuPinningPolicy());
         model.getCpuPinningPolicy().setSelectedCpuPolicy(vm.getCpuPinningPolicy());
         if (vm.getCpuPinningPolicy() == CpuPinningPolicy.MANUAL) {
             model.getCpuPinning().setEntity(vm.getCpuPinning());

diff --git a/...eb/src/main/java/org/ovirt/engine/ui/uicommonweb/builders/vm/NumaUnitToVmBaseBuilder.java b/...eb/src/main/java/org/ovirt/engine/ui/uicommonweb/builders/vm/NumaUnitToVmBaseBuilder.java
@@ -4,6 +4,7 @@
 import java.util.List;
 import java.util.Optional;
 
+import org.ovirt.engine.core.common.businessentities.CpuPinningPolicy;
 import org.ovirt.engine.core.common.businessentities.VmBase;
 import org.ovirt.engine.core.common.businessentities.VmNumaNode;
 import org.ovirt.engine.core.common.utils.NumaUtils;
@@ -24,8 +25,13 @@ protected void build(UnitVmModel model, VmBase vm) {
             return;
         }
 
+        boolean isNumaPinningAllowed = model.getNumaSupportCommand().getIsExecutionAllowed();
+
+        boolean isResizeAndPinDropped = model.getOriginalCpuPinningPolicy() == CpuPinningPolicy.RESIZE_AND_PIN_NUMA
+                && model.getCpuPinningPolicy().getSelectedItem().getPolicy() != CpuPinningPolicy.RESIZE_AND_PIN_NUMA;
+
         List<VmNumaNode> nodeList = model.getVmNumaNodes();
-        if (nodeList == null || nodeList.size() != nodeCount) {
+        if (nodeList == null || nodeList.size() != nodeCount || !isNumaPinningAllowed || isResizeAndPinDropped) {
             nodeList = new ArrayList<>(nodeCount);
             for (int i = 0; i < nodeCount; i++) {
                 VmNumaNode newNode = new VmNumaNode();

diff --git a/...onweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/VmBaseListModel.java b/...onweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/VmBaseListModel.java
@@ -620,8 +620,7 @@ public void init(Runnable callback) {
                     confirmModel.addRecommendationForCpuPinning(isVmAssignedToSpecificHosts, isVmCpuPinningSet);
 
                     // Handle NUMA
-                    final boolean isVmVirtNumaSet =
-                            model.getNumaEnabled().getEntity() && model.getNumaNodeCount().getEntity() > 0;
+                    final boolean isVmVirtNumaSet = model.getNumaNodeCount().getEntity() > 0;
                     final boolean isVmVirtNumaPinned =
                             model.getVmNumaNodes() != null
                                     && !model.getVmNumaNodes().isEmpty()

diff --git a/...ain/java/org/ovirt/engine/ui/uicommonweb/models/vms/CloneVmFromSnapshotModelBehavior.java b/...ain/java/org/ovirt/engine/ui/uicommonweb/models/vms/CloneVmFromSnapshotModelBehavior.java
@@ -58,6 +58,6 @@ public void initStorageDomains() {
     }
 
     @Override
-    protected void updateNumaEnabled() {
+    protected void updateNumaFields() {
     }
 }
diff --git a/...web/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingVmModelBehavior.java b/...web/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingVmModelBehavior.java
@@ -49,6 +49,7 @@ public class ExistingVmModelBehavior extends VmModelBehaviorBase<UnitVmModel> {
     private int hostCpu;
     private VDS runningOnHost;
     private Guid bootableDiskStorageDomainId;
+    private List<VmNumaNode> initialVmNumaNodes = new ArrayList<>();
 
     public ExistingVmModelBehavior(VM vm) {
         this.vm = vm;
@@ -96,18 +97,18 @@ public void initialize() {
 
         if (vm.isNextRunConfigurationExists()) {
             getModel().setVmNumaNodes(vm.getvNumaNodeList());
-            getModel().setInitialVmNumaNodes(vm.getvNumaNodeList());
             getModel().updateNodeCount(vm.getvNumaNodeList().size());
             getModel().updateCpuPinningPolicy();
+            initialVmNumaNodes = vm.getvNumaNodeList();
         } else {
             Frontend.getInstance().runQuery(QueryType.GetVmNumaNodesByVmId,
                     new IdQueryParameters(vm.getId()),
                     new AsyncQuery<QueryReturnValue>(returnValue -> {
                         List<VmNumaNode> nodes = returnValue.getReturnValue();
                         getModel().setVmNumaNodes(nodes);
-                        getModel().setInitialVmNumaNodes(nodes);
                         getModel().updateNodeCount(nodes.size());
                         getModel().updateCpuPinningPolicy();
+                        initialVmNumaNodes = vm.getvNumaNodeList();
                     }));
         }
         // load dedicated host names into host names list
@@ -501,27 +502,24 @@ protected VM getVmWithNuma() {
     }
 
     @Override
-    protected void updateNumaEnabled() {
-        super.updateNumaEnabled();
+    protected void updateNumaFields() {
+        super.updateNumaFields();
+        super.updateNumaPinningEnabled();
+
         boolean isResizeAndPin =
                 getModel().getCpuPinningPolicy().getSelectedItem().getPolicy() == CpuPinningPolicy.RESIZE_AND_PIN_NUMA;
-        updateNumaEnabledHelper(!isResizeAndPin);
-
-        if (getModel().getVmNumaNodes() != null) {
-            getModel().getNumaNodeCount().setEntity(getModel().getVmNumaNodes().size());
-        }
 
         if (isResizeAndPin) {
             if (!getModel().getTotalCPUCores().getEntity().equals(Integer.toString(getVm().getNumOfCpus()))) {
                 getModel().getTotalCPUCores().setEntity(Integer.toString(getVm().getNumOfCpus()));
             }
 
-            if (!getModel().getNumaNodeCount().getEntity().equals(getModel().getInitialVmNumaNodes().size())) {
-                getModel().getNumaNodeCount().setEntity(getModel().getInitialVmNumaNodes().size());
+            if (!getModel().getNumaNodeCount().getEntity().equals(initialVmNumaNodes.size())) {
+                getModel().getNumaNodeCount().setEntity(initialVmNumaNodes.size());
             }
 
-            if (!getModel().getVmNumaNodes().equals(getModel().getInitialVmNumaNodes())) {
-                getModel().setVmNumaNodes(getModel().getInitialVmNumaNodes());
+            if (!getModel().getVmNumaNodes().equals(initialVmNumaNodes)) {
+                getModel().setVmNumaNodes(initialVmNumaNodes);
             }
         }
     }
@@ -532,4 +530,41 @@ public void updateMaxMemory() {
             super.updateMaxMemory();
         }
     }
+
+    /**
+     * Returns true if the NUMA should be updated on the backend.
+     */
+    @Override
+    public boolean shouldUpdateNuma() {
+
+        if (getModel().getCpuPinningPolicy().getSelectedItem().getPolicy() == CpuPinningPolicy.RESIZE_AND_PIN_NUMA) {
+            return false;
+        }
+
+        if (vm.getCpuPinningPolicy() == CpuPinningPolicy.RESIZE_AND_PIN_NUMA && getModel().getCpuPinningPolicy()
+                .getSelectedItem()
+                .getPolicy() != CpuPinningPolicy.RESIZE_AND_PIN_NUMA) {
+            return true;
+        }
+
+        if (!getModel().getNumaNodeCount().getEntity().equals(initialVmNumaNodes.size())) {
+            return true;
+        }
+
+        if (!getModel().getVmNumaNodes().equals(initialVmNumaNodes)) {
+            return true;
+        }
+
+        if (getModel().getVmNumaNodes().size() > 0) {
+            if (!getModel().getMemSize().getEntity().equals(vm.getMemSizeMb())) {
+                return true;
+            }
+
+            if (!getModel().getTotalCPUCores().getEntity().equals(Integer.toString(vm.getNumOfCpus()))) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }
diff --git a/.../main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmFromTemplateModelBehavior.java b/.../main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmFromTemplateModelBehavior.java
@@ -99,4 +99,10 @@ protected void initClusters(final List<StoragePool> dataCenters) {
                 false);
     }
 
+    @Override
+    protected void updateNumaFields() {
+        super.updateNumaFields();
+        updateNumaPinningEnabled();
+    }
+
 }
diff --git a/...ommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmModelBehavior.java b/...ommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmModelBehavior.java
@@ -352,9 +352,9 @@ public InstanceTypeManager getInstanceTypeManager() {
     }
 
     @Override
-    protected void updateNumaEnabled() {
-        super.updateNumaEnabled();
-        updateNumaEnabledHelper(true);
+    protected void updateNumaFields() {
+        super.updateNumaFields();
+        updateNumaPinningEnabled();
     }
 
     @Override

diff --git a/...les/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java b/...les/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java
@@ -346,8 +346,6 @@ public void setVmAttachedToPool(boolean value) {
             getParallelMigrationsType().setIsChangeable(false);
             getCustomParallelMigrations().setIsChangeable(false);
 
-            getNumaEnabled().setIsChangeable(false);
-
             // ==High Availability==
             getIsHighlyAvailable().setIsChangeable(false);
             getLease().setIsChangeable(false);
@@ -1511,6 +1509,16 @@ public VmModelBehaviorBase getBehavior() {
     private void setBehavior(VmModelBehaviorBase value) {
     }
 
+    private CpuPinningPolicy originalCpuPinningPolicy;
+
+    public void setOriginalCpuPinningPolicy(CpuPinningPolicy originalCpuPinningPolicy) {
+        this.originalCpuPinningPolicy = originalCpuPinningPolicy;
+    }
+
+    public CpuPinningPolicy getOriginalCpuPinningPolicy() {
+        return originalCpuPinningPolicy;
+    }
+
     private CpuPinningListModel cpuPinningPolicy;
 
     public CpuPinningListModel getCpuPinningPolicy() {
@@ -1621,8 +1629,6 @@ public void setCpuProfiles(NotChangableForVmInPoolListModel<CpuProfile> cpuProfi
         this.cpuProfiles = cpuProfiles;
     }
 
-    private EntityModel<Boolean> numaEnabled;
-
     private NotChangableForVmInPoolEntityModel<Integer> numaNodeCount;
 
     public EntityModel<Integer> getNumaNodeCount() {
@@ -1633,16 +1639,6 @@ public void setNumaNodeCount(NotChangableForVmInPoolEntityModel<Integer> numaNod
         this.numaNodeCount = numaNodeCount;
     }
 
-    private List<VmNumaNode> initialVmNumaNodes = new ArrayList<>();
-
-    public List<VmNumaNode> getInitialVmNumaNodes() {
-        return initialVmNumaNodes;
-    }
-
-    public void setInitialVmNumaNodes(List<VmNumaNode> vmNumaNodes) {
-        this.initialVmNumaNodes = new ArrayList<>(vmNumaNodes);
-    }
-
     private List<VmNumaNode> vmNumaNodes;
 
     public List<VmNumaNode> getVmNumaNodes() {
@@ -1663,8 +1659,6 @@ private void setNumaSupportCommand(UICommand numaSupportCommand) {
         this.numaSupportCommand = numaSupportCommand;
     }
 
-    private boolean numaChanged = false;
-
     private ListModelWithClusterDefault<Boolean> autoConverge;
 
     public ListModel<Boolean> getAutoConverge() {
@@ -2056,19 +2050,18 @@ public void setSelectedItem(Integer value) {
         getCpuProfiles().setIsAvailable(false);
 
         setNumaNodeCount(new NotChangableForVmInPoolEntityModel<Integer>());
+        getNumaNodeCount().getEntityChangedEvent().addListener(this);
         getNumaNodeCount().setEntity(0);
-        setNumaEnabled(new EntityModel<Boolean>());
-        getNumaEnabled().setMessage(ConstantsManager.getInstance().getConstants().numaDisabledInfoMessage());
 
         setNumaSupportCommand(new UICommand("NumaSupport", new ICommandTarget() { //$NON-NLS-1$
                     @Override
                     public void executeCommand(UICommand command) {
-                        numaSupport();
+                        getBehavior().numaSupport();
                     }
 
                     @Override
                     public void executeCommand(UICommand uiCommand, Object... parameters) {
-                        numaSupport();
+                        getBehavior().numaSupport();
                     }
                 }));
 
@@ -2311,14 +2304,14 @@ public void eventRaised(Event<? extends EventArgs> ev, Object sender, EventArgs
                 updateTpmEnabled();
             } else if (sender == getCpuPinningPolicy()) {
                 cpuPinningPolicyChanged();
-                behavior.updateNumaEnabled();
+                behavior.updateNumaFields();
             } else if (sender == getConsoleDisconnectAction()){
                 consoleDisconnectActionSelectedItemChanged();
             }
         } else if (ev.matchesDefinition(ListModel.selectedItemsChangedEventDefinition)) {
             if (sender == getDefaultHost()) {
                 defaultHost_SelectedItemChanged(sender, args);
-                behavior.updateNumaEnabled();
+                behavior.updateNumaFields();
                 headlessModeChanged();
             }
         } else if (ev.matchesDefinition(HasEntity.entityChangedEventDefinition)) {
@@ -2333,8 +2326,10 @@ public void eventRaised(Event<? extends EventArgs> ev, Object sender, EventArgs
             } else if (sender == getIsAutoAssign()) {
                 behavior.updateUseHostCpuAvailability();
                 behavior.updateCpuPinningVisibility();
-                behavior.updateNumaEnabled();
+                behavior.updateNumaFields();
                 updateCpuPinningPolicy();
+            } else if (sender == getNumaNodeCount()) {
+                behavior.updateNumaPinningEnabled();
             } else if (sender == getProvisioning()) {
                 provisioning_SelectedItemChanged(sender, args);
             } else if (sender == getProvisioningThin_IsSelected()) {
@@ -3797,19 +3792,6 @@ public boolean isCreateInstanceOnly() {
         return ((CurrentUserRole) TypeResolver.getInstance().resolve(CurrentUserRole.class)).isCreateInstanceOnly();
     }
 
-    private void numaSupport() {
-        setNumaChanged(true);
-        getBehavior().numaSupport();
-    }
-
-    public EntityModel<Boolean> getNumaEnabled() {
-        return numaEnabled;
-    }
-
-    public void setNumaEnabled(EntityModel<Boolean> numaEnabled) {
-        this.numaEnabled = numaEnabled;
-    }
-
     @Override
     public void executeCommand(UICommand command) {
         super.executeCommand(command);
@@ -3831,14 +3813,6 @@ private void onNumaSupport() {
         updateCpuPinningPolicy();
     }
 
-    public void setNumaChanged(boolean numaChanged) {
-        this.numaChanged = numaChanged;
-    }
-
-    public boolean isNumaChanged() {
-        return numaChanged || initialVmNumaNodes.size() != getNumaNodeCount().getEntity();
-    }
-
     public void updateNodeCount(int size) {
         getNumaNodeCount().setEntity(size);
     }
@@ -4089,11 +4063,7 @@ protected void updateCpuPinningPolicy() {
                     .isDedicatedPolicySupportedByVersion(getCompatibilityVersion());
         }
 
-        boolean numaNodesUnpinned =
-                getNumaEnabled() == null
-                        || getNumaEnabled().getEntity() == null
-                        || !getNumaEnabled().getEntity()
-                        || getVmNumaNodes() == null
+        boolean numaNodesUnpinned = getVmNumaNodes() == null
                         || getVmNumaNodes().stream().allMatch(numa -> numa.getVdsNumaNodeList().isEmpty());
 
         cpuPinningPolicy.setCpuPolicyEnabled(CpuPinningPolicy.MANUAL, defaultHostSelected);