[Ovirt-devel] Re: [PATCH node] Report on physical network devices when identifying the node.

[Mohammed Morsi]

Don't want to ack this just yet as there are some outstanding issues I'm
not sure about.

Hugh there is a requirements question below which is why I'm cc'ing you


Darryl L. Pierce wrote:
> From: Mohammed Morsi <mmorsi at redhat.com>
>
>   
I'm still listed as the author here, can we change this?


> The default interface configuration is only applied if a configuration
> was not retrieved from the server for some reason.
>
> When ovirt-identify-node collects NIC details to send back to the
> server, it only collects for physical devices that support the net.80203
> capability as reported by HAL.
>
> Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
> ---
>  ovirt-identify-node/gather.c |   24 ++++++++++++------------
>  scripts/ovirt-early          |   31 +++++++++++++++++++------------
>  2 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/ovirt-identify-node/gather.c b/ovirt-identify-node/gather.c
> index c12ed08..2f26f46 100644
> --- a/ovirt-identify-node/gather.c
> +++ b/ovirt-identify-node/gather.c
> @@ -286,7 +286,7 @@ get_nic_info(void)
>  
>      int i;
>  
> -    nics = libhal_find_device_by_capability(hal_ctx, "net",
> +    nics = libhal_find_device_by_capability(hal_ctx, "net.80203",
>                                              &num_results, &dbus_error);
>   
Not too familiar w/ libhal, but I'm assuming net.80203 will only return
physical devices and not bridges and whatnot.

>  
>      DEBUG("Found %d NICs\n", num_results);
> @@ -294,15 +294,15 @@ get_nic_info(void)
>      for (i = 0; i < num_results; i++) {
>          char *nic = nics[i];
>  
> -        VERBOSE("Starting new NIC.\n");
> +        DEBUG("Starting new NIC; %s.\n", nic);
>  
> -        if (current != NULL) {
> -            last = current;
> -            current = create_nic_info();
> -            last->next = current;
> -        } else {
> -            nic_info = current = create_nic_info();
> -        }
> +	if (current != NULL) {
> +	  last = current;
> +	  current = create_nic_info();
> +	  last->next = current;
> +	} else {
> +	  nic_info = current = create_nic_info();
> +	}
>   
This seems to be an indentation issue only. Not too happy with ack'ing
this as part of this patch, but it won't hold my ack up.


>  
>          snprintf(current->mac_address, BUFFER_LENGTH, "%s",
>                   libhal_device_get_property_string(hal_ctx, nic,
> @@ -310,9 +310,9 @@ get_nic_info(void)
>                                                     &dbus_error));
>          get_nic_data(nic, current);
>  
> -        DEBUG("NIC details: MAC:%s, speed:%s, IP:%s\n",
> -              nic_info->mac_address, nic_info->bandwidth,
> -              nic_info->ip_address);
> +	DEBUG("NIC details: MAC:%s, IFACE_NAME: %s, speed:%s, IP:%s\n",
> +	      current->mac_address, current->iface_name,
> +	      current->bandwidth, current->ip_address);
>      }
>   
Same indentation comment.


>  
>      return result;
> diff --git a/scripts/ovirt-early b/scripts/ovirt-early
> index 9ab2981..9b7e902 100755
> --- a/scripts/ovirt-early
> +++ b/scripts/ovirt-early
> @@ -20,6 +20,7 @@ get_mac_addresses() {
>  
>  configure_from_network() {
>      DEVICE=$1
> +
>      if [ -n "$DEVICE" ]; then
>          log "Configuring network Using interface $DEVICE"
>          # setup temporary interface to retrieve configuration
> @@ -46,13 +47,14 @@ configure_from_network() {
>                          ovirt-process-config $cfgdb $BONDING_MODCONF_FILE $AUGTOOL_CONFIG
>  			if [ $? -eq 0 ]; then
>  			    log "Remote configuration retrieved and applied"
> +                            rm $cfgdb
> +			    return
>  			else
>  			    log "Failure to retrieve or apply remote configuration"
>  			fi
>                      else
>                          log "Failed to retrieve configuration bundle"
>                      fi
> -                    rm $cfgdb
>                  fi
>              fi
>          fi
> @@ -62,17 +64,22 @@ configure_from_network() {
>      ETHDEVS=$(cd /sys/class/net && ls -d eth*)
>      for eth in $ETHDEVS; do
>          BRIDGE=br$eth
> -        log "Applying default configuration to $eth and $BRIDGE"
> -        printf '%s\n' "DEVICE=$eth" ONBOOT=yes "BRIDGE=$BRIDGE" \
> -            > /etc/sysconfig/network-scripts/ifcfg-$eth
> -        if [ -z "$DEVICE" -o "$DEVICE" = "$eth" ]; then
> -            dhcp="BOOTPROTO=dhcp"
> -        else
> -            dhcp=""
> -        fi
> -        printf '%s\n' "DEVICE=$BRIDGE" "$dhcp" \
> -            ONBOOT=yes TYPE=Bridge PEERNTP=yes DELAY=0 \
> -            > /etc/sysconfig/network-scripts/ifcfg-$BRIDGE
> +	local ifcfg=/etc/sysconfig/network-scripts/ifcfg-$BRIDGE
> +
> +	# only write a default file if one does not exist
> +	if [ ! -f $ifcfg ]; then
> +            log "Applying default configuration to $eth and $BRIDGE"
> +            printf '%s\n' "DEVICE=$eth" ONBOOT=yes "BRIDGE=$BRIDGE" \
> +		> /etc/sysconfig/network-scripts/ifcfg-$eth
> +            if [ -z "$DEVICE" -o "$DEVICE" = "$eth" ]; then
> +		dhcp="BOOTPROTO=dhcp"
> +            else
> +		dhcp=""
> +            fi
> +            printf '%s\n' "DEVICE=$BRIDGE" "$dhcp" \
> +		ONBOOT=yes TYPE=Bridge PEERNTP=yes DELAY=0 \
> +		> /etc/sysconfig/network-scripts/ifcfg-$BRIDGE
> +	fi
>      done
>      log "Default config applied"
>  }
>   
The requirements issue I'm not sure about is whether or not we want to
configure all interfaces by default as you are doing so here. I recall
discussion pertaining to when the configuration isn't received from the
server, to only configure / initialize the boot device and not any of
the other nics. Hugh am I right about this?

Overall, other than those few issues, it looks good though.

   -Mo