Fix unexpected exception on assigning labels on host networks #892

sermakov-orion · 2023-11-21T05:07:31Z

Signed-off-by: Stepan Ermakov [email protected]

Changes introduced with this PR

This PR fixes the following issue: an error occurs while assigning labels on host network interfaces: "Error while executing action HostSetupNetworks: Unexpected exception"
Steps to reproduce:

Open "Compute->Hosts". List of hosts will be displayed.
Click on any host and select "Network Interfaces" tab. Network interfaces of the selected host will be displayed.
Click on "Setup Host Networks". "Setup Host Networks" modal dialog will be displayed.
Click on "Labels". Labels of the host networks will be displayed.
Drag-n-drop "New Label" on any network interface. Enter new label name and click "OK". New label will be added to the selected network.
Click "OK" in the "Setup Host Networks" modal dialog.

Expected behavior
The changes applied with no errors
Current behavior
The error message is displayed: "Error while executing action HostSetupNetworks: Unexpected exception". The new label was not added to the host network.

Background

The HostSetupNetworksCommand verifies if there are changes made in the "Setup Host Networks" modal dialog (see HostSetupNetworksCommand.executeCommand, if (noChangesDetected()) statement).
Since there are some changes (labels were added) it makes the HostSetupNetworks call to the host VDSM but passes empty payload inside the call (because there were no real changes of networks, just labels were changed). And VDSM fails with the Unexpected Error on the empty payload.
In this PR I introduced additional check if (hasNetworkChanges()) that verifies whether the HostSetupNetworks call to the host VDSM is needed or not.

Are you the owner of the code you are sending in, or do you have permission of the owner?

y

michalskrivanek · 2023-11-21T09:16:11Z

LGTM though i'm not sure about the completeness of the changes that need to go to the actual host, @almusil wdyt?

sermakov-orion · 2023-11-21T09:37:10Z

@michalskrivanek let me add bit more clarity on why I decided not to go to the actual host in case when there no changes other than labels. The HostSetupNetworks VDSM command is invoked with parameters created by the HostSetupNetworksCommand.createSetupNetworksParameters method. And if you walk through this and underlying methods you may notice that they use everything from getParameters() but not getParametes().getLables() and getParameters().getRemovedLables(). So, It looks that HostSetupNetworks does not require any changes in labels.

almusil

Hi,

thank you for the patch. Please add an explanation to the commit message so it is clear in the history why this change was required.

almusil · 2023-11-21T12:05:17Z

...dules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java


+                if (succeeded) {
                    setSucceeded(true);


setSucceeded(true) can be moved to the if above when there is only single boolean.

almusil · 2023-11-21T12:06:19Z

...dules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java

-                if (retVal != null) {
-                    if (!retVal.getSucceeded() && retVal.getVdsError() == null && getParameters().rollbackOnFailure()) {
-                        throw new EngineException(EngineError.SETUP_NETWORKS_ROLLBACK, retVal.getExceptionString());
+                boolean networksUpdated = false;


We need only single boolean. The fact that we have propagated succeed even if retVal.getSucceeded() == false seems to be a mistake. That was introduced by 28f34a4.

@almusil this significantly simplifies the proposed changes. Thanks a lot! Will do it soon - it may take a while to verify the changes.

almusil · 2023-11-21T12:09:02Z

...dules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java

                    }
+                } else {


We can flip the succeeded logic to avoid this else branch.

almusil · 2023-11-21T12:09:35Z

...dules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java

+                boolean succeeded = false;
+
+                if (hasNetworkChanges()) {
+                    //Update networks configuration on the host


Please add space after // and period at the end, that applies to all comments.

Signed-off-by: Stepan Ermakov <[email protected]> This PR fixes the following issue: an error occurs while assigning labels on host network interfaces: "Error while executing action HostSetupNetworks: Unexpected exception" Steps to reproduce: 1. Open "Compute->Hosts". List of hosts will be displayed. 2. Click on any host and select "Network Interfaces" tab. Network interfaces of the selected host will be displayed. 3. Click on "Setup Host Networks". "Setup Host Networks" modal dialog will be displayed. 4. Click on "Labels". Labels of the host networks will be displayed. 5. Drag-n-drop "New Label" on any network interface. Enter new label name and click "OK". New label will be added to the selected network. 6. Click "OK" in the "Setup Host Networks" modal dialog. Expected behavior: The changes applied with no errors Current behavior: The error message is displayed: "Error while executing action HostSetupNetworks: Unexpected exception". The new label was not added to the host network. Background: The HostSetupNetworksCommand verifies if there are changes made in the "Setup Host Networks" modal dialog (see HostSetupNetworksCommand.executeCommand, "if (noChangesDetected())" statement). Since there are some changes (labels were added) it makes the HostSetupNetworks call to the host VDSM but passes empty payload inside the call (because there were no real changes of networks, just labels were changed). And VDSM fails with the Unexpected Error on the empty payload. In this PR I introduced additional check "if (hasNetworkChanges())" that verifies whether the HostSetupNetworks call to the host VDSM is needed or not.

sermakov-orion · 2023-11-23T11:49:44Z

Hi,

thank you for the patch. Please add an explanation to the commit message so it is clear in the history why this change was required.

Hi @almusil.
I did this change as well as other mentioned here. Could you please do another round of the review?

almusil

Looks good, thanks! @michalskrivanek do we want to run ost? It will most likely fail in the UI tests though.

almusil · 2023-11-27T08:40:59Z

/ost

almusil · 2023-11-27T09:44:55Z

/ost basic-suite-master el9stream

sermakov-orion requested review from almusil, ahadas, bennyz, emesika, mwperina, michalskrivanek, oliel, sgratch, smelamud, didib and ljelinkova as code owners November 21, 2023 05:07

almusil reviewed Nov 21, 2023

View reviewed changes

sermakov-orion force-pushed the host-network-labels branch from 37c9ba4 to 6e8f1e3 Compare November 23, 2023 11:47

sermakov-orion requested a review from almusil November 24, 2023 06:47

almusil approved these changes Nov 24, 2023

View reviewed changes

almusil merged commit 9fa9d7d into oVirt:master Nov 27, 2023
4 checks passed

sermakov-orion deleted the host-network-labels branch November 28, 2023 04:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unexpected exception on assigning labels on host networks #892

Fix unexpected exception on assigning labels on host networks #892

sermakov-orion commented Nov 21, 2023

michalskrivanek commented Nov 21, 2023

sermakov-orion commented Nov 21, 2023

almusil left a comment

almusil Nov 21, 2023

sermakov-orion Nov 23, 2023

almusil Nov 21, 2023

sermakov-orion Nov 21, 2023

sermakov-orion Nov 23, 2023

almusil Nov 21, 2023

sermakov-orion Nov 23, 2023

almusil Nov 21, 2023

sermakov-orion Nov 23, 2023

sermakov-orion commented Nov 23, 2023

almusil left a comment

almusil commented Nov 27, 2023

almusil commented Nov 27, 2023

Fix unexpected exception on assigning labels on host networks #892

Fix unexpected exception on assigning labels on host networks #892

Conversation

sermakov-orion commented Nov 21, 2023

Changes introduced with this PR

Are you the owner of the code you are sending in, or do you have permission of the owner?

michalskrivanek commented Nov 21, 2023

sermakov-orion commented Nov 21, 2023

almusil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sermakov-orion commented Nov 23, 2023

almusil left a comment

Choose a reason for hiding this comment

almusil commented Nov 27, 2023

almusil commented Nov 27, 2023