Hello Moti Asayag,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/16848

to review the following change.

Change subject: engine: InstallVds cleanup and extract method
......................................................................

engine: InstallVds cleanup and extract method

The patch performs cleanup to the parameters class and
extracts methods from the exeucteCommand which became
too long.

Change-Id: I727a976146de00ab7c3b3267cf1eb74092a9bf0b
Signed-off-by: Moti Asayag <masa...@redhat.com>
---
M 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
M 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/InstallVdsParameters.java
2 files changed, 126 insertions(+), 124 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/48/16848/1

diff --git 
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
 
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
index 7e19fcf..fd2c0f9 100644
--- 
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
+++ 
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
@@ -13,12 +13,12 @@
 import org.ovirt.engine.core.common.businessentities.VDSType;
 import org.ovirt.engine.core.common.config.Config;
 import org.ovirt.engine.core.common.config.ConfigValues;
+import org.ovirt.engine.core.common.errors.VdcBllMessages;
 import 
org.ovirt.engine.core.common.vdscommands.SetVdsStatusVDSCommandParameters;
 import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
-import org.ovirt.engine.core.utils.NetworkUtils;
-import org.ovirt.engine.core.common.errors.VdcBllMessages;
 import org.ovirt.engine.core.compat.Guid;
 import org.ovirt.engine.core.compat.RpmVersion;
+import org.ovirt.engine.core.utils.NetworkUtils;
 import org.ovirt.engine.core.utils.log.Log;
 import org.ovirt.engine.core.utils.log.LogFactory;
 import org.ovirt.engine.core.vdsbroker.vdsbroker.VDSNetworkException;
@@ -86,136 +86,138 @@
 
     @Override
     protected void executeCommand() {
-        if (
-            getVds() != null &&
-            isOvirtReInstallOrUpgrade()
-        ) {
-            OVirtNodeUpgrade upgrade = null;
-            try {
-                T parameters = getParameters();
-                upgrade = new OVirtNodeUpgrade(
+        if (getVds() == null) {
+            return;
+        }
+
+        if (isOvirtReInstallOrUpgrade()) {
+            reinstallOrUpgradeHost();
+        } else {
+            installHost();
+        }
+    }
+
+    private void installHost() {
+        VdsDeploy installer = null;
+        try {
+            log.infoFormat(
+                    "Before Installation host {0}, {1}",
+                    getVds().getId(),
+                    getVds().getName()
+                    );
+
+            T parameters = getParameters();
+            installer = new VdsDeploy(getVds());
+            installer.setCorrelationId(getCorrelationId());
+            boolean configureNetworkUsingHostDeploy =
+                    
!VersionSupport.isActionSupported(VdcActionType.SetupNetworks,
+                            getVds().getVdsGroupCompatibilityVersion());
+            installer.setReboot(parameters.isRebootAfterInstallation() && 
configureNetworkUsingHostDeploy);
+            if (configureNetworkUsingHostDeploy) {
+                
installer.setManagementNetwork(NetworkUtils.getEngineNetwork());
+            }
+
+            switch (getVds().getVdsType()) {
+            case VDS:
+                installer.setUser("root");
+                installer.setPassword(parameters.getRootPassword());
+                installer.setFirewall(parameters.getOverrideFirewall());
+                break;
+            case oVirtNode:
+                if (parameters.getOverrideFirewall()) {
+                    log.warnFormat(
+                            "Installation of Host {0} will ignore Firewall 
Override option, since it is not supported for Host type {1}",
+                            getVds().getName(),
+                            getVds().getVdsType().name()
+                            );
+                }
+                installer.setUser("root");
+                installer.useDefaultKeyPair();
+                break;
+            default:
+                throw new IllegalArgumentException(
+                        String.format(
+                                "Not handled VDS type: %1$s",
+                                getVds().getVdsType()
+                                )
+                        );
+            }
+            setVdsStatus(VDSStatus.Installing);
+            installer.execute();
+
+            switch (installer.getDeployStatus()) {
+            case Failed:
+                throw new VdsInstallException(VDSStatus.InstallFailed, 
StringUtils.EMPTY);
+            case Incomplete:
+                throw new VdsInstallException(VDSStatus.InstallFailed, 
"Partial installation");
+            case Reboot:
+                setVdsStatus(VDSStatus.Reboot);
+                RunSleepOnReboot();
+                break;
+            case Complete:
+                if (!configureNetworkUsingHostDeploy) {
+                    configureManagementNetwork();
+                }
+            default:
+                setVdsStatus(VDSStatus.Initializing);
+                break;
+            }
+            log.infoFormat(
+                    "After Installation host {0}, {1}",
+                    getVds().getName(),
+                    getVds().getVdsType().name()
+                    );
+            setSucceeded(true);
+        } catch (VdsInstallException e) {
+            handleError(e, e.getStatus());
+        } catch (Exception e) {
+            handleError(e, VDSStatus.InstallFailed);
+        } finally {
+            if (installer != null) {
+                installer.close();
+            }
+        }
+    }
+
+    private void reinstallOrUpgradeHost() {
+        OVirtNodeUpgrade upgrade = null;
+        try {
+            T parameters = getParameters();
+            upgrade = new OVirtNodeUpgrade(
                     getVds(),
                     parameters.getoVirtIsoFile()
-                );
-                upgrade.setCorrelationId(getCorrelationId());
-                log.infoFormat(
+                    );
+            upgrade.setCorrelationId(getCorrelationId());
+            log.infoFormat(
                     "Execute upgrade host {0}, {1}",
                     getVds().getId(),
                     getVds().getName()
-                );
-                upgrade.execute();
-                log.infoFormat(
+                    );
+            upgrade.execute();
+            log.infoFormat(
                     "After upgrade host {0}, {1}: success",
                     getVds().getId(),
                     getVds().getName()
-                );
-                setSucceeded(true);
-                if (getVds().getStatus() == VDSStatus.Reboot) {
-                    RunSleepOnReboot();
-                }
+                    );
+            setSucceeded(true);
+            if (getVds().getStatus() == VDSStatus.Reboot) {
+                RunSleepOnReboot();
             }
-            catch (Exception e) {
-                log.errorFormat(
+        }
+        catch (Exception e) {
+            log.errorFormat(
                     "Host installation failed for host {0}, {1}.",
                     getVds().getId(),
                     getVds().getName(),
                     e
-                );
-                setSucceeded(false);
-                _failureMessage = e.getMessage();
-            }
-            finally {
-                if (upgrade != null) {
-                    upgrade.close();
-                }
-            }
-            return;
-        }
-
-        if (getVds() != null) {
-            VdsDeploy installer = null;
-            try {
-                log.infoFormat(
-                    "Before Installation host {0}, {1}",
-                    getVds().getId(),
-                    getVds().getName()
-                );
-
-                T parameters = getParameters();
-                installer = new VdsDeploy(getVds());
-                installer.setCorrelationId(getCorrelationId());
-                boolean configureNetworkUsingHostDeploy =
-                        
!VersionSupport.isActionSupported(VdcActionType.SetupNetworks,
-                                getVds().getVdsGroupCompatibilityVersion());
-                installer.setReboot(parameters.isRebootAfterInstallation() && 
configureNetworkUsingHostDeploy);
-                if (configureNetworkUsingHostDeploy) {
-                    
installer.setManagementNetwork(NetworkUtils.getEngineNetwork());
-                }
-
-                switch (getVds().getVdsType()) {
-                case VDS:
-                    installer.setUser("root");
-                    installer.setPassword(parameters.getRootPassword());
-                    installer.setFirewall(parameters.getOverrideFirewall());
-                    break;
-                case oVirtNode:
-                    if (parameters.getOverrideFirewall()) {
-                        log.warnFormat(
-                            "Installation of Host {0} will ignore Firewall 
Override option, since it is not supported for Host type {1}",
-                            getVds().getName(),
-                            getVds().getVdsType().name()
-                        );
-                    }
-                    installer.setUser("root");
-                    installer.useDefaultKeyPair();
-                    break;
-                default:
-                    throw new IllegalArgumentException(
-                        String.format(
-                            "Not handled VDS type: %1$s",
-                            getVds().getVdsType()
-                        )
                     );
-                }
-                setVdsStatus(VDSStatus.Installing);
-                installer.execute();
-
-                switch (installer.getDeployStatus()) {
-                case Failed:
-                    throw new VdsInstallException(VDSStatus.InstallFailed, 
StringUtils.EMPTY);
-                case Incomplete:
-                    throw new VdsInstallException(VDSStatus.InstallFailed, 
"Partial installation");
-                case Reboot:
-                    setVdsStatus(VDSStatus.Reboot);
-                    RunSleepOnReboot();
-                    break;
-                case Complete:
-                    if (!configureNetworkUsingHostDeploy) {
-                        configureManagementNetwork();
-                    }
-                default:
-                    setVdsStatus(VDSStatus.Initializing);
-                    break;
-                }
-                log.infoFormat(
-                    "After Installation host {0}, {1}",
-                    getVds().getName(),
-                    getVds().getVdsType().name()
-                );
-                setSucceeded(true);
+            setSucceeded(false);
+            _failureMessage = e.getMessage();
+        }
+        finally {
+            if (upgrade != null) {
+                upgrade.close();
             }
-            catch (VdsInstallException e) {
-                handleError(e, e.getStatus());
-            }
-            catch (Exception e) {
-                handleError(e, VDSStatus.InstallFailed);
-            }
-            finally {
-                if (installer != null) {
-                    installer.close();
-                }
-            }
-            return;
         }
     }
 
diff --git 
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/InstallVdsParameters.java
 
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/InstallVdsParameters.java
index ceab53b..dcb7b78 100644
--- 
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/InstallVdsParameters.java
+++ 
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/InstallVdsParameters.java
@@ -6,8 +6,8 @@
 
     private static final long serialVersionUID = 5066290843683399113L;
 
-    private boolean privateIsReinstallOrUpgrade;
-    private String privateoVirtIsoFile;
+    private boolean reinstallOrUpgrade;
+    private String oVirtIsoFile;
 
 
     public InstallVdsParameters() {
@@ -15,24 +15,24 @@
 
     public InstallVdsParameters(Guid vdsId, String password) {
         super();
-        this.setVdsId(vdsId);
+        setVdsId(vdsId);
         setRootPassword(password);
     }
 
     public boolean getIsReinstallOrUpgrade() {
-        return privateIsReinstallOrUpgrade;
+        return reinstallOrUpgrade;
     }
 
     public String getoVirtIsoFile() {
-        return privateoVirtIsoFile;
+        return oVirtIsoFile;
     }
 
     public void setIsReinstallOrUpgrade(boolean value) {
-        privateIsReinstallOrUpgrade = value;
+        reinstallOrUpgrade = value;
     }
 
     public void setoVirtIsoFile(String value) {
-        privateoVirtIsoFile = value;
+        oVirtIsoFile = value;
     }
 
 


-- 
To view, visit http://gerrit.ovirt.org/16848
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I727a976146de00ab7c3b3267cf1eb74092a9bf0b
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to