Moti Asayag has posted comments on this change.
Change subject: engine: Allow engine to configure management network
......................................................................
Patch Set 11: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 143:
Line 144: T parameters = getParameters();
Line 145: installer = new VdsDeploy(getVds());
Line 146: installer.setCorrelationId(getCorrelationId());
Line 147: boolean configureNetwork =
!FeatureSupported.setupNetworks(getVds().getVdsGroupCompatibilityVersion());
Done
Line 148:
installer.setReboot(parameters.isRebootAfterInstallation() && configureNetwork);
Line 149: if (configureNetwork) {
Line 150:
installer.setManagementNetwork(NetworkUtils.getEngineNetwork());
Line 151: }
Line 179: installer.execute();
Line 180:
Line 181: switch (installer.getDeployStatus()) {
Line 182: case Failed:
Line 183: throw new
VdsInstallException(VDSStatus.InstallFailed, null);
shouldn't be a problem, it should result with GENERIC_ERROR as the message.
However, i'll replace it with StringUtils.EMPTY ("") to avoid "null" text in
the stacktrace.
Line 184: case Incomplete:
Line 185: throw new
VdsInstallException(VDSStatus.InstallFailed, "Partial installation");
Line 186: case Reboot:
Line 187: setVdsStatus(VDSStatus.Reboot);
Line 205: catch (VdsInstallException e) {
Line 206: handleError(e, e.getStatus());
Line 207: }
Line 208: catch (Exception e) {
Line 209: handleError(e, VDSStatus.InstallFailed);
I don't agree on this one. There is already support for multiple catch blocks,
so evaluating the type of the exception is redundant and furthermore if new
exception types will be introduced you'll get a long condition like:
try {
bla
}
catch (OtherException e) {
...
}
catch (Exception e) {
Status status = failed;
if (e instanceOf VdsInstallException) {
status = e.getStatus()// with cast
} else if (e instanceOf OtherException) {
// some some other treatment
} else if (e instanceOf OtherException2) {
// some some other treatment
}
single place to handle exceptional recovery
}
Raising concern about adding new catch block is valid for your suggestion as
well, as no prevention to add another catch block on top of the exception.
try {
bla
}
catch (OtherException e) {
...
}
catch (Exception e) {
Status status = failed;
if (e instanceOf VdsInstallException) {
status = e.getStatus()// with cast
}
single place to handle exceptional recovery
}
Line 210: }
Line 211: finally {
Line 212: if (installer != null) {
Line 213: installer.close();
Line 231: }
Line 232:
Line 233: private void configureManagementNetwork() {
Line 234: final NetworkConfigurator networkConfigurator = new
NetworkConfigurator(getVds());
Line 235: if (networkConfigurator.awaitVdsmResponse()) {
Funny...had a version of that and decided not to push it.
We do so.
Line 236: try {
Line 237: networkConfigurator.refreshNetworkConfiguration();
Line 238:
networkConfigurator.createManagementNetworkIfRequired();
Line 239: } catch (VDSNetworkException e) {
Line 237: networkConfigurator.refreshNetworkConfiguration();
Line 238:
networkConfigurator.createManagementNetworkIfRequired();
Line 239: } catch (VDSNetworkException e) {
Line 240: throw new VdsInstallException(VDSStatus.NonResponsive,
Line 241: "Network error during communication with the
host");
Done
Line 242: } catch (VdsInstallException e) {
Line 243: throw new
VdsInstallException(VDSStatus.NonOperational,
Line 244: "Failed to configure manamgent network on the
host");
Line 245: }
Line 238:
networkConfigurator.createManagementNetworkIfRequired();
Line 239: } catch (VDSNetworkException e) {
Line 240: throw new VdsInstallException(VDSStatus.NonResponsive,
Line 241: "Network error during communication with the
host");
Line 242: } catch (VdsInstallException e) {
no...should be any Exception. wrong method extraction.
Line 243: throw new
VdsInstallException(VDSStatus.NonOperational,
Line 244: "Failed to configure manamgent network on the
host");
Line 245: }
Line 246: } else {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java
Line 92: private boolean _goingToReboot = false;
Line 93: private boolean _aborted = false;
Line 94: private boolean _installIncomplete = false;
Line 95: private String _managementNetwork = null;
Line 96: private DeployStatus deployStatus = null;
Done
Line 97:
Line 98: private String _certificate;
Line 99: private String _iptables = "";
Line 100:
Line 969: _messages.post(
Line 970: InstallerMessages.Severity.ERROR,
Line 971: "Processing stopped due to timeout"
Line 972: );
Line 973: deployStatus = DeployStatus.Failed;
Done
Line 974: throw e;
Line 975: }
Line 976: catch(Exception e) {
Line 977: log.errorFormat(
Line 978: "Error during host {0} install",
Line 979: _vds.getHostName(),
Line 980: e
Line 981: );
Line 982: deployStatus = DeployStatus.Failed;
Done
Line 983:
Line 984: if (_failException == null) {
Line 985: throw e;
Line 986: }
--
To view, visit http://gerrit.ovirt.org/14230
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf82e10481e595d690e7ce894283b4ed5b9b3269
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Martin Pavlik <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
