-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Fri, 22 Jan 2010, Steffen Maier wrote:
On 01/22/2010 12:21 AM, David Cantrell wrote:
Remove linuxrc.s390's usage of /sbin/ip, /sbin/ifconfig, and /sbin/route
and instead just write the settings to the ifcfg-DEVICE file, set the
device online in sysfs, and start NetworkManager. This prevents a bug
where linuxrc.s390 was bringing up the interface, then loader would
start, it would launch NM, and immediately the network connection would
come down.
Apparently, this looks like a bug in NM. If it was well-behaved, it
wouldn't touch already correctly configured devices and just kept them
running.
Changing linuxrc.s390 is just a workaround and even more so removes a
whole lot of usability that was just introduced and users liked it.
Therefore, I disagree with below changes.
More specific comments below.
The main goal of this change is to reduce the duplication of effort we
currently have in linuxrc.s390. We use NetworkManager on the other platforms
to bring up interfaces, so we should use it on s390x too. If it doesn't work
on s390x, we should fix NetworkManager rather than work around it in
linuxrc.s390.
Starting NM earlier prevents this and simplifies
linuxrc.s390 a lot.
---
loader/linuxrc.s390 | 313 ++++-----------------------------------------------
1 files changed, 22 insertions(+), 291 deletions(-)
diff --git a/loader/linuxrc.s390 b/loader/linuxrc.s390
index 7a6be69..dfdd8d9 100644
--- a/loader/linuxrc.s390
+++ b/loader/linuxrc.s390
@@ -660,30 +660,6 @@ function set_device_online() {
fi
}
-# sets device up and blocks until device appears to be up
-function set_device_up() {
- if [ -z "$DEVICE" ]; then
- echo $"Could not determine interface name to bring up device $SUBCHANNELS"
- return 1
- fi
- # Device does not come up fast enough to use "ip" to configure, so block.
- # While OSA come up themselves after setting online,
- # e.g. HiperSockets won't => set them up explicitly for the following check
- debug ip link set up $DEVICE
- local i=1
- while : ; do
- local tst=$(ip -o link show up dev $DEVICE)
- [ -n "$tst" ] && break
- sleep 1
- i=$((i+1))
- if [ "$i" -gt 10 ]; then
- echo $"Could not bring up device $DEVICE within timeout"
- return 1
- fi
- done
- return 0
-}
-
By removing this, you rely on enough time to have passed until NM gets
started which is fragile. Reminds me of device_cio_free or dasd_settle
et al.
NM continually polls the interfaces, so I don't see the point of this
function.
@@ -805,7 +726,6 @@ function rollback_config() {
fi
fi
[ -z "$mtu_was_set" ] && unset MTU
- [ -z "$mmtu_was_set" ] && unset MMTU
This is still required for show_parm() to work. I never fully understood
why this magic is there to distinguish between user input and parm/conf
file input. However, I wouldn't want to break any old feature that was
in linuxrc.s390 before I touched it.
The MMTU variable gets set to "mtu $MTU" and used as an argument to
/sbin/ifconfig and /sbin/ip. NetworkManager handles this fine when it sees an
MTU variable in the device ifcfg file.
@@ -1632,17 +1552,6 @@ function syntax_check_macaddr() {
return 1
}
-function handle_macaddr() {
- # - try to set macaddr right here w/ error handlg.
- # device needs to be online
- if debug ifconfig $DEVICE hw ether $MACADDR; then
- return 0
- fi
- echo $"MAC address $MACADDR could not be configured for"
- echo $" $SUBCHANNELS (network device $DEVICE)"
- return 1
-}
-
Have you tested this successfully with a real (non-virtual) OSA in
layer2 mode?
I'm just not sure, if NM will apply this MAC address early on the device.
It looks like NM does not do anything with the MACADDR variable currently. We
can patch ifcfg-rh to honor that.
-### NETWORK
-
-function do_network() {
- echo
- echo $"The NETWORK parameter isn't used anymore and will be ignored."
- echo $" It is sufficient to specify IPADDR and NETMASK."
- echo
-}
-
-### BROADCAST
-
-function do_broadcast() {
- echo
- echo $"The BROADCAST parameter isn't used anymore and will be ignored."
- echo $" It is sufficient to specify IPADDR and NETMASK."
- echo
-}
-
Those two functions are there on purpose, to help users with old
parm/conf file and to acquaint them with the changes we introduced.
Otherwise they would most probably never get to know that there are
parameters in their parm/conf files they do not longer need or they
wonder why they don't get applied any more. Please keep those functions.
Migration guide. Documentation.
### NETMASK (IPv6)
function syntax_check_prefix_v6() {
@@ -2015,43 +1906,6 @@ function do_netmask_v6() {
### GATEWAY (IPv4)
-function configure_ipv4_gateway() {
- # FIXME:
- # - Strictly speaking we should first check reachability of gateway
- # and then configure the gateway route.
- # This would require a new intermediate workflow_item step
- # so that the user might continue despite unreachable gateway.
- # done: Only adding default route might add multiple undesired default
- # routes on redoing the parameter item, so delete default route
- # before adding a new one.
- ip -4 route del default dev $DEVICE >& /dev/null
- [ -z "$GATEWAY" ] && return 0
- if ! tv route add default gw $GATEWAY dev $DEVICE; then
- echo $"Could net set default route on device $DEVICE via gateway $GATEWAY"
- return 1
- fi
- # BH FIXME: Workaround for manual MACADDR, need ping to update arp table
- echo $"Trying to reach gateway $GATEWAY..."
- if [ "$NETTYPE" = "ctc" ]; then
- # (virtual) CTC(/A) seems to need some time to get functional
- local i=1
- while : ; do
- ping -c 1 $GATEWAY >& /dev/null && break
- i=$((i+1))
- if [ "$i" -gt 3 ]; then
- echo $"Could not reach gateway $GATEWAY within timeout"
- return 1
- fi
- done
- else
- if ! ping -c 1 $GATEWAY >& /dev/null; then
- echo $"Could not reach your default gateway $GATEWAY"
- return 1
- fi
- fi
- return 0
-}
-
Now, we are starting to loose significant parts of usability, which was
the sole purpose of rewriting linuxrc.s390 in the first place. It is a
step back towards the old linuxrc which would only ask all parameters
and then try to configure the network ignoring error cases and not
helping the user to confirm his/her entries were syntactically and even
more so semantically correct.
This is exactly the kind of stuff that NetworkManager will handle for us. One
thing that led me to removing this code was that I kept hitting a problem
where the "ping -c 1" test on $GATEWAY would only work part of the time. If I
added "-w 5" for a timeout for the entire run, it worked. But then that's
just for my VM. NetworkManager can take care of this for us and can report if
the connection is up or not.
### GATEWAY (IPv6)
-function configure_ipv6_gateway() {
- # FIXME:
- # - Strictly speaking we should first check reachability of gateway
- # and then configure the gateway route.
- # This would require a new intermediate workflow_item step
- # so that the user might continue despite unreachable gateway.
- # done: Only adding default route might add multiple undesired default
- # routes on redoing the parameter item, so delete default route
- # before adding a new one.
- ip -6 route del default dev $DEVICE >& /dev/null
- [ -z "$GATEWAY" ] && return 0
- # IPv6 http://www.ibiblio.org/pub/Linux/docs/HOWTO/other-formats/html_single/Linux+IPv6-HOWTO.html#AEN1147
- # ip -6 route add ::/0 dev $DEVICE via $GATEWAY
- # (Could also be learned by autoconfiguration on the link:
- # after IP address setup and device up,
- # see if default route has been learned
- # ip -6 route show | grep ^default
- # However, we currently use manual IPv6 configuration only.)
- if ! debug ip -6 route add ::/0 dev $DEVICE via $GATEWAY; then
- echo $"Could net set default route on device $DEVICE"
- echo $" via gateway $GATEWAY"
- return 1
- fi
- # BH FIXME: Workaround for manual MACADDR, need ping to update arp table
- echo $"Trying to reach gateway $GATEWAY..."
- if ! ping6 -c 1 $GATEWAY >& /dev/null; then
- echo $"Could not reach your default gateway $GATEWAY"
- return 1
- fi
- return 0
-}
-
usability--;
Same explanation as above. Also, the way linuxrc.s390 is currently written
won't allow for dual stack configuration, which we need to support during
installation. We already have the code to collect the values from users and
NetworkManager can do dual stack configuration of the interface. I still need
to modify things slightly to allow for dual stack rather than the either/or
approach now.
@@ -2261,49 +2061,6 @@ function syntax_check_dns() {
fi
}
-function handle_dns() {
- # - foreach DNS try if server is reachable by one ping
- [ -z "$DNS" ] && return 0
- local dnsitem
- local allgood="yes"
- echo $"Trying to reach DNS servers..."
- if [ "$ipv6" ]; then
- while read dnsitem; do
- if ! ping6 -c 1 $dnsitem >& /dev/null; then
- echo $"Could not ping DNS server (might still serve DNS requests): $dnsitem"
- allgood="no"
- # this should not be a hard failure since some network
- # environments may prevent pings to DNS servers
- # => prevent workflow_item_menu in kickstart mode
- fi
- done < <(echo $DNS | sed 's/,/\n/g')
- else
- while read dnsitem; do
- # Some network environment may prevent a DNS server from being
- # reachable by ping, so it would make sense to use nslookup.
- # However, nslookup fails with "Resolver Error 0 (no error)"
- # at this stage of the setup progress => not useful
- if ! ping -c 1 $dnsitem >& /dev/null; then
- echo $"Could not ping DNS server: $dnsitem"
-# if nslookup $dnsitem $dnsitem >& /dev/null; then
-# echo $" but could resolve DNS server with itself: $dnsitem"
-# else
-# echo $"Could not resolve DNS server with itself: $dnsitem"
-# allgood="no"
-# fi
-# elif ! nslookup $dnsitem $dnsitem >& /dev/null; then
-# echo $"Could not resolve DNS server with itself: $dnsitem"
- allgood="no"
- fi
- done < <(echo $DNS | sed 's/:/\n/g')
- fi
- if [ "$allgood" = "yes" ]; then
- return 0
- else
- return 1
- fi
-}
-
usability--;
I think it's less than zero already.
So the problem I have with this function is that it's entirely too fragile.
Verifying that DNS servers are where we say they are and that we can look up
things is putting too much trust in DNS during installation. We don't enforce
this on other platforms and I don't see why we should here.
@@ -2674,9 +2431,7 @@ EOF
[ "$PORTNO" ] && echo "PORTNO=$PORTNO"
[ "$PEERID" ] && echo "PEERID=$PEERID"
[ "$CTCPROT" ] && echo "CTCPROT=$CTCPROT"
- if [ -n "$mmtu_was_set" ]; then
- echo "MMTU=\"$MMTU\""
- elif [ -n "$mtu_was_set" ]; then
+ if [ -n "$mtu_was_set" ]; then
echo "MTU=$MTU"
fi
[ "$DNS" ] && echo "DNS=$DNS"
@@ -2786,18 +2541,14 @@ fi
# Perform a network installation
[ -n "$MTU" ] && mtu_was_set=$MTU
-[ -n "$MMTU" ] && mmtu_was_set=$MMTU
What does this change cause? Did you test that this does not break any
previously available behavior?
I set an MTU of 4096 during installation and that still works when I use this
version of linuxrc.s390 that just runs NM.
[ -n "$VSWITCH" ] && vswitch_was_set=$VSWITCH
[ -n "$CHANDEV" ] && do_chandev
-[ -n "$NETWORK" ] && do_network
-[ -n "$BROADCAST" ] && do_broadcast
Those two should definitely stay.
Why? These are the functions that just echo to the console that you don't
need to set NETWORK or BROADCAST anymore.
else # ctc0
- if [ -z "$NETMASK" ]; then
- # If the user did not supply netmask, we add the right one.
- # Netmask MUST be present,
- # or pumpSetupInterface() blows routes.
- NETMASK="255.255.255.255"
- fi
This does not work well with show_parms() which shows NETMASK=$NETMASK
unconditionally.
OK, I'll bring this back in. We've got to have at least an IP address and
netmask or prefix.
- # don't ask for MTU, but use it if set in the parm file
- # don't overwrite MMTU if it has been set for CTC
- [ "$NETTYPE" = "ctc" -a -z "$MTU" -a -z "$MMTU" ] && \
- MMTU="mtu 1500"
Are you sure this keeps working for CTC?
No, I'm not, but I can add this back in. Force an MTU of 1500 if the user did
not specify one in the CMS conf file for ctc devices. That's easy enough.
@@ -2964,9 +2695,7 @@ NETTYPE="$NETTYPE"
IPADDR="$IPADDR"
GATEWAY="$GATEWAY"
MTU="$MTU"
-NETWORK="$NETWORK"
NETMASK="$NETMASK"
-BROADCAST="$BROADCAST"
SEARCHDNS="$SEARCHDNS"
PEERID="$PEERID"
SUBCHANNELS="$SUBCHANNELS"
@@ -2987,7 +2716,7 @@ fi
cat >> /tmp/install.cfg << EOF
export LANG PORTNAME S390ARCH TEXTDOMAIN TEXTDOMAINDIR
export HOSTNAME DEVICE NETTYPE IPADDR GATEWAY MTU
-export NETWORK NETMASK BROADCAST DNS DNS1 DNS2 SEARCHDNS
+export NETMASK DNS DNS1 DNS2 SEARCHDNS
You remove parts of install.cfg but the rest remains. I don't understand
what this would mean for loader.
I remove NETWORK and BROADCAST, which are no longer used. /tmp/install.cfg is
read by the booty code.
export PEERID ONBOOT SUBCHANNELS CTCPROT
EOF
# immediately read it in again to export these into the shell below
@@ -3002,7 +2731,7 @@ if [ ! -d "$NETSCRIPTS" ]; then
mkdir -p $NETSCRIPTS
fi
-# to please NetworkManager on startup in loader before loader reconfigures net
+# nm-system-settings reads the ifcfg-DEVICE files
cat > /etc/sysconfig/network << EOF
HOSTNAME=$HOSTNAME
EOF
@@ -3012,7 +2741,6 @@ DEVICE=$DEVICE
ONBOOT=yes
BOOTPROTO=static
GATEWAY=$GATEWAY
-BROADCAST=$BROADCAST
I suppose NM doesn't need this.
But does initscripts' network service still work with OSA, Hipersockets,
LCS, and CTC in all its flavors after the BROADCAST option is written no
more?
As long as NM does not support features such as bonding, data center
servers still need network service.
That's a good point, should probably keep them around in the ifcfg in that
case.
@@ -3084,6 +2814,7 @@ EOF
# reboot on SIGUSR2
trap doreboot SIGUSR2
+ /usr/sbin/NetworkManager --pid-file=/var/run/NetworkManager/NetworkManager.pid
Would we have to wait for NM as in iface.c:iface_start_NetworkManager()
for the network to be configured before sshd binds in startinetd?
Entirely possible. I don't have that now, but need to add it in.
And will loader stop calling iface_start_NetworkManager() or does
NetworkManager detect itself that it is already running?
loader will still call iface_start_NetworkManager(), which in turn calls
is_nm_running() and returns if we already have NM running.
- --
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
iEYEARECAAYFAktaKN8ACgkQ5hsjjIy1VkmIXQCdFv5DBDB14W13b+2MRHs6Fm4n
4OkAoKVhC3wTN3NRpC+WD81xHA2FrqVv
=dPPU
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
