Mike Kolesnik has posted comments on this change.
Change subject: engine: Configure management network at host activation
......................................................................
Patch Set 10: (10 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ActivateVdsCommand.java
Line 71: runVdsCommand(VDSCommandType.SetVdsStatus,
Line 72: new SetVdsStatusVDSCommandParameters(getVdsId(),
VDSStatus.Unassigned));
Line 73:
Line 74: VDSReturnValue returnValue =
Line 75:
getBackend().getResourceManager().RunVdsCommand(VDSCommandType.ActivateVds,
Why not use runVdsCommand() method as was used before?
Line 76: new
ActivateVdsVDSCommandParameters(getVdsId()));
Line 77: setSucceeded(returnValue.getSucceeded());
Line 78:
Line 79: if (getSucceeded()) {
Line 99: }
Line 100: }
Line 101:
Line 102: private void createManagementNetworkIfRequired(VDS host) {
Line 103: if (host != null) {
How would the host be null in this case, if the activate command succeeded?
Line 104: Network managementNetwork =
Line 105:
getNetworkDAO().getByNameAndDataCenter(Config.<String>
GetValue(ConfigValues.ManagementNetwork),
Line 106: host.getStoragePoolId());
Line 107:
Line 100: }
Line 101:
Line 102: private void createManagementNetworkIfRequired(VDS host) {
Line 103: if (host != null) {
Line 104: Network managementNetwork =
Why query for the management network if it's always the same name?
Or, why do it here and not inside findNicForManagementNetwork() method?
Line 105:
getNetworkDAO().getByNameAndDataCenter(Config.<String>
GetValue(ConfigValues.ManagementNetwork),
Line 106: host.getStoragePoolId());
Line 107:
Line 108: VdsNetworkInterface nic =
findNicForManagementNetwork(host, managementNetwork);
Line 108: VdsNetworkInterface nic =
findNicForManagementNetwork(host, managementNetwork);
Line 109: if (nic != null) {
Line 110: List<VdsNetworkInterface> interfaces =
filterBondsWithoutSlaves(host.getInterfaces());
Line 111: if (interfaces.contains(nic)) {
Line 112: nic.setNetworkName(managementNetwork.getName());
What if the interface the VDSM reports is a bridge, but not mgmt bridge?
Should we not use the underlying NIC, and drop the bridge in that case?
Line 113: SetupNetworksParameters parameters = new
SetupNetworksParameters();
Line 114: parameters.setVdsId(host.getId());
Line 115: parameters.setInterfaces(interfaces);
Line 116: parameters.setCheckConnectivity(true);
Line 123: }
Line 124: }
Line 125:
Line 126: private VdsNetworkInterface findNicForManagementNetwork(final VDS
host, Network network) {
Line 127: if (StringUtils.isEmpty(host.getActiveNic()) ||
network.getName().equals(host.getActiveNic())) {
This is assuming it's bridged, what if it's bridgeless?
Line 128: return null;
Line 129: }
Line 130:
Line 131: Map<String, VdsNetworkInterface> nameToIface =
Entities.entitiesByName(host.getInterfaces());
Line 128: return null;
Line 129: }
Line 130:
Line 131: Map<String, VdsNetworkInterface> nameToIface =
Entities.entitiesByName(host.getInterfaces());
Line 132: VdsNetworkInterface nic =
nameToIface.get(host.getActiveNic());
Didn't we decide in this case to override the old VLAN value?
Line 133: if (nic != null && !nicHasValidVlanId(network, nic)) {
Line 134: addCustomValue("VlanId", resolveVlanId(nic.getVlanId()));
Line 135: addCustomValue("MgmtVlanId",
resolveVlanId(network.getVlanId()));
Line 136: addCustomValue("InterfaceName", nic.getName());
Line 141: return nic;
Line 142: }
Line 143:
Line 144: private String resolveVlanId(Integer vlanId) {
Line 145: return vlanId == null ? "none" : vlanId.toString();
This is not getting translated..
Line 146: }
Line 147:
Line 148: private boolean nicHasValidVlanId(Network network,
VdsNetworkInterface nic) {
Line 149: int nicVlanId = nic.getVlanId() == null ? 0 : nic.getVlanId();
Line 155: List<VdsNetworkInterface> filteredList = new
ArrayList<VdsNetworkInterface>();
Line 156: Map<String, Integer> bonds = new HashMap<String, Integer>();
Line 157:
Line 158: for (VdsNetworkInterface iface : interfaces) {
Line 159: if (Boolean.TRUE.equals(iface.getBonded())) {
How about we start using instanceof instead, and check if it's Bond?
Line 160: bonds.put(iface.getName(), 0);
Line 161: }
Line 162: }
Line 163:
Line 167: }
Line 168: }
Line 169:
Line 170: for (VdsNetworkInterface iface : interfaces) {
Line 171: if (!bonds.containsKey(iface.getName()) ||
bonds.get(iface.getName()) >= 2) {
Why not support bond with only one slave?
Line 172: filteredList.add(iface);
Line 173: }
Line 174: }
Line 175:
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 452: NETWORK_UPDATE_VM_INTERFACE_LINK_UP=Link State is UP.
Line 453: NETWORK_UPDATE_VM_INTERFACE_LINK_DOWN=Link State is DOWN.
Line 454: NETWORK_UPDATE_VM_INTERFACE_FAILED=Failed to update Interface
${InterfaceName} (${InterfaceType}) for VM ${VmName}. (User: ${UserName})
Line 455: NETWORK_HOST_USING_WRONG_CLUSER_VLAN=${VdsName} is having wrong vlan
id: ${VlanIdHost}, expected vlan id: ${VlanIdCluster}
Line 456: NETWORK_HOST_MISSING_CLUSER_VLAN=${VdsName} is missing vlan id:
${VlanIdCluster} that is expected by the cluster
Perhaps the messages should start as: "Failed to activate host ${VdsName},
because ..."
This way it's clear what the user attempted to do which is activate the host,
the fact that something failed during that attempt, does not mean its the
action requested.
Line 457: INVALID_INTERFACE_FOR_MANAGEMENT_NETWORK_CONFIGURATION=Host
${VdsName} has an invalid interface ${InterfaceName} for the management network
configuration.
Line 458: VLAN_ID_MISMATCH_FOR_MANAGEMENT_NETWORK_CONFIGURATION=Host ${VdsName}
has an interface ${InterfaceName} for the management network configuration with
VLAN-ID (${VlanId}) other than data-center definition (${MgmtVlanId}).
Line 459: SETUP_NETWORK_FAILED_FOR_MANAGEMENT_NETWORK_CONFIGURATION=Failed to
configure management network on host ${VdsName}.
Line 460: PERSIST_NETWORK_FAILED_FOR_MANAGEMENT_NETWORK=Failed to persist
management network configuration on host ${VdsName}.
--
To view, visit http://gerrit.ovirt.org/10919
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia82e594b888b84b3aa079cc9aa76fa0dddc59f1d
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: 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
