Alon Bar-Lev has posted comments on this change.
Change subject: engine: Allow engine to configure management network
......................................................................
Patch Set 11: (9 inline comments)
Thank you so much! Looking real good.
Only minor 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());
I always fall into this one... can you please consider the configureNetwork
with configureNetworkUsingHostDeploy?
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);
are you sure that the null will not create problems down the road?
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 understand why you had this code. However, with my-self I always regret that
the following is not more readable, maybe only because of indent... not sure...
But what I don't like is the split of the exceptional recovery into somewhere
else in code.... I know this is common pattern... but I always find it much
more difficult to read and error prune, as someone may add another catch but
forget to call the common code.
try {
try {
bla
}
catch (VdsInstallException e) {
throw e;
}
catch (Exception e) {
throw new VdsInstallException(e.getMessage, failed, e);
}
}
catch (VdsInstallException e) {
single place to handle exceptional recovery
}
In your case it can also be:
try {
bla
}
catch (Exception e) {
Status status = failed;
if (e instanceOf VdsInstallException) {
status = e.getStatus()// with cast
}
single place to handle exceptional recovery
}
End with me wasting your time...
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()) {
Structural of exceptional parameters:
if (failure) {
throw exception
}
continue as usual
In this case:
if (!networkConfigurator.awaitVdsmResponse()) {
throw new VdsInstallException(VDSStatus.NonResponsive,
"Network error during communication with the host");
}
try {
...
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");
Please consider to chain cause exception.
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) {
Are you sure VdsInstallException is the right exception here?
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;
Suggest to set here DeployStatus.Failed.
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;
Suggest to remove this.
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;
Suggest to remove this.
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
