Mike Kolesnik has posted comments on this change.
Change subject: engine: Allow engine to configure management network
......................................................................
Patch Set 12: (14 inline comments)
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 318: select fn_db_add_config_value('NonVmNetworkSupported','true','3.1');
Line 319: select fn_db_add_config_value('NonVmNetworkSupported','true','3.2');
Line 320: select fn_db_add_config_value('NonVmNetworkSupported','true','3.3');
Line 321: select fn_db_add_config_value('SetupNetworksSupported','false','3.0');
Line 322: select fn_db_add_config_value('SetupNetworksSupported','true','3.1');
You don't have to specify the lines with the same value as the default value,
as it will be used in case no overriding line is found.
Line 323: select fn_db_add_config_value('SetupNetworksSupported','true','3.2');
Line 324: select fn_db_add_config_value('SetupNetworksSupported','true','3.3');
Line 325: select
fn_db_add_config_value('NumberOfFailedRunsOnVds','3','general');
Line 326: select fn_db_add_config_value('NumberOfUSBSlots','4','general');
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 226: );
Line 227: setVdsStatus(status);
Line 228: setSucceeded(false);
Line 229: _failureMessage = getErrorMessage(e.getMessage());
Line 230: addCustomValue("FailedInstallMessage", _failureMessage);
Since you already extracted this code, you should notice that this line is
redundant and already done on line 80.
Also the getError message on the line above is done on line 80 anyway..
For your consideration
Line 231: }
Line 232:
Line 233: private void configureManagementNetwork() {
Line 234: final NetworkConfigurator networkConfigurator = new
NetworkConfigurator(getVds());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/NetworkConfigurator.java
Line 39:
Line 40: private static final int VDSM_RESPONSIVENESS_PERIOD_IN_SECONDS =
120;
Line 41: private static final String MANAGEMENET_NETWORK_CONFIG_ERR =
"Failed to configure management network";
Line 42: private static final String NETWORK_CONFIG_LOG_ERR = "Failed to
configure management network: {0}";
Line 43: private static final long POLLING_BREAK = 500;
You intend to wait 500 seconds, milliseconds, nanoseconds?
Line 44: private static final Log log =
LogFactory.getLog(NetworkConfigurator.class);
Line 45: final private VDS host;
Line 46:
Line 47: public NetworkConfigurator(VDS host) {
Line 41: private static final String MANAGEMENET_NETWORK_CONFIG_ERR =
"Failed to configure management network";
Line 42: private static final String NETWORK_CONFIG_LOG_ERR = "Failed to
configure management network: {0}";
Line 43: private static final long POLLING_BREAK = 500;
Line 44: private static final Log log =
LogFactory.getLog(NetworkConfigurator.class);
Line 45: final private VDS host;
should be:
private final VDS host;
Line 46:
Line 47: public NetworkConfigurator(VDS host) {
Line 48: this.host = host;
Line 49: }
Line 94: return true;
Line 95: }
Line 96:
Line 97: public boolean awaitVdsmResponse() {
Line 98: final int checks = VDSM_RESPONSIVENESS_PERIOD_IN_SECONDS /
Config.<Integer> GetValue(ConfigValues.SetupNetworksPollingTimeout);
This line is longer than 120 characters.
Line 99: for (int i = 0; i < checks; i++) {
Line 100: if (pollVds()) {
Line 101: log.infoFormat("Engine managed to communicate with
VDSM agent on host {0}",
Line 102: host.getName(),
Line 129:
Line 130: private VdsNetworkInterface findNicToSetupManagementNetwork() {
Line 131:
Line 132: if (StringUtils.isEmpty(host.getActiveNic())) {
Line 133: log.warnFormat("No interface was reported as
lastClientInterface by host {0} capabilities. " +
I think on split lines it makes sense to put the operator first, so it's easier
to see the new line is continuing the line above it..
For example:
methodCall("a"
+ "b")
Versus:
methodCall("a" +
"b")
In the latter form (on long and complex lines) it is harder to tell that string
"b" is a concatenation, rather than a new parameter.
Line 134: "There will be no attempt to create the
management network on the host.", host.getName());
Line 135: return null;
Line 136: }
Line 137:
Line 137:
Line 138: VdsNetworkInterface nic =
Entities.entitiesByName(host.getInterfaces()).get(host.getActiveNic());
Line 139:
Line 140: if (nic == null) {
Line 141: log.warnFormat("The lastClientInterface {0} of host {1}
is not a valid interface for the mangement network. " +
This line is longer than 120 characters.
Line 142: "If the interface is a bridge, it should be
teared-down manually.",
Line 143: host.getActiveNic(),
Line 144: host.getName());
Line 145: throw new
NetworkConfiguratorException(String.format("lastClientIface %s is not a valid
interface for management network",
Line 138: VdsNetworkInterface nic =
Entities.entitiesByName(host.getInterfaces()).get(host.getActiveNic());
Line 139:
Line 140: if (nic == null) {
Line 141: log.warnFormat("The lastClientInterface {0} of host {1}
is not a valid interface for the mangement network. " +
Line 142: "If the interface is a bridge, it should be
teared-down manually.",
Should be "torn down" not "teared down"
Line 143: host.getActiveNic(),
Line 144: host.getName());
Line 145: throw new
NetworkConfiguratorException(String.format("lastClientIface %s is not a valid
interface for management network",
Line 146: host.getActiveNic()));
Line 141: log.warnFormat("The lastClientInterface {0} of host {1}
is not a valid interface for the mangement network. " +
Line 142: "If the interface is a bridge, it should be
teared-down manually.",
Line 143: host.getActiveNic(),
Line 144: host.getName());
Line 145: throw new
NetworkConfiguratorException(String.format("lastClientIface %s is not a valid
interface for management network",
This line is longer than 120 characters.
Line 146: host.getActiveNic()));
Line 147: }
Line 148:
Line 149: Network managementNetwork =
Line 155: return null;
Line 156: }
Line 157:
Line 158: if (!nicHasValidVlanId(managementNetwork, nic)) {
Line 159: final AuditLogableBase event = new AuditLogableBase();
You can extract the creation of event with host to a method since it repeats
several times
Line 160: event.setVds(host);
Line 161: event.addCustomValue("VlanId",
resolveVlanId(nic.getVlanId()));
Line 162: event.addCustomValue("MgmtVlanId",
resolveVlanId(managementNetwork.getVlanId()));
Line 163: event.addCustomValue("InterfaceName", nic.getName());
Line 207:
Line 208: private void configureManagementNetwork(SetupNetworksParameters
parameters) {
Line 209: VdcReturnValueBase retVal =
getBackend().runInternalAction(VdcActionType.SetupNetworks, parameters);
Line 210: if (retVal.getSucceeded()) {
Line 211: retVal =
Not sure why new line here is necessary..
Line 212:
getBackend().runInternalAction(VdcActionType.CommitNetworkChanges,
Line 213: new
VdsActionParameters(parameters.getVdsId()));
Line 214: if (!retVal.getSucceeded()) {
Line 215: final AuditLogableBase event = new AuditLogableBase();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java
Line 402: VdsmEnv.MANAGEMENT_BRIDGE_NAME,
Line 403: _managementNetwork
Line 404: );
Line 405: }
Line 406: else if (_isNode) {
Shouldn't this logic sit in InstallVdsCommand?
There is already a division done there by host type on line 153, so in the same
manner the decision that is taken to install or not to install the management
network on the node can be done there.
Line 407: _parser.cliEnvironmentSet(
Line 408: VdsmEnv.MANAGEMENT_BRIDGE_NAME,
Line 409: NetworkUtils.getEngineNetwork()
Line 410: );
Line 840: *
Line 841: * @param managementNetwork
Line 842: */
Line 843: public void setManagementNetwork(String managementNetwork) {
Line 844: this._managementNetwork = managementNetwork;
Why do you need "this." ?
Line 845: }
Line 846:
Line 847: public void setCorrelationId(String correlationId) {
Line 848: _correlationId = correlationId;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java
Line 101: * Compatibility version to check for.
Line 102: * @return <code>true</code> if setup networks is supported for
the version, <code>false</code> if it's not.
Line 103: */
Line 104: public static boolean setupNetworks(Version version) {
Line 105: return supportedInConfig(ConfigValues.SetupNetworksSupported,
version);
Please consider if we should add another config value, or instead use the
ActionVersionMap here to check if the feature is supported or not (since the
data is already there).
Line 106: }
--
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: 12
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
