Re: [PATCH] bonding: Don't allow mode change via sysfs with slaves present
On Wed, Nov 16, 2011 at 01:02:21PM +0100, Nicolas de Pesloüan wrote:
> Le 15/11/2011 21:47, Andy Gospodarek a écrit :
>> Nicolas,
>>
>> I took a look at the ifenslave package for debian more closely and it
>> actually looks like devices are enslaved last, after mode is set. Can
>> you please take a look at this package and confirm what I'm seeing in
>> the 'pre-up' script.
>>
>> It appears to me that setup_master sets the mode and enslave_slaves is
>> called after and enslaves the devices:
>>
>> # Option slaves deprecated, replaced by bond-slaves, but still supported
>> # for backward compatibility.
>> IF_BOND_SLAVES=${IF_BOND_SLAVES:-$IF_SLAVES}
>>
>> if [ "$IF_BOND_MASTER" ] ; then
>> BOND_MASTER="$IF_BOND_MASTER"
>> BOND_SLAVES="$IFACE"
>> else
>> if [ "$IF_BOND_SLAVES" ] ; then
>> BOND_MASTER="$IFACE"
>> BOND_SLAVES="$IF_BOND_SLAVES"
>> fi
>> fi
>>
>> # Exit if nothing to do...
>> [ -z "$BOND_MASTER$BOND_SLAVES" ]&& exit
>>
>> add_master
>> early_setup_master
>> setup_master
>> enslave_slaves
>> exit 0
>
> Andy,
>
> I'm really surprise by this extract. In the most up to date version of
> the ifenslave-2.6 package (1.1.0-19), the order is:
>
> add_master
> early_setup_master
> enslave_slaves
> setup_master
>
> early_setup_master was created to be able to do things that absolutely
> need to be done before enslavement. (See the comment just before this
> function). The idea was to do every possible setup in setup_master, after
> enslavement, except those that need to be done in early_setup_master. So
> having enslave_slaves after setup_master instead of before is definitely
> a mistake. Some of the operations in setup_master must be done after
> enslavement, in particular selecting the primary slave.
>
> In version 1.1.0-18 (change log below), I checked all the possible order
> constraints of the sysfs interface and totally reworked most of the setup
> code, putting everything in the right order to achieve consistent
> results.
>
> ifenslave-2.6 (1.1.0-18) experimental; urgency=low
>
> * Apply patch from Nicolas de Pesloüan:
>
> - Major change: Check and fix the order in which the configuration is
> written into /sys files, to ensure reliable results, according to the
> tests done in the kernel (in drivers/net/bonding/bond_sysfs.c).
> - Ensure that master is properly brought down when changing a parameter
> that require it to be down.
> - Ensure the master is brought up at the end of the setup, in the case
> where ifup won't bring it up because it is currently configuring a slave.
> - Add support for some previously unsupported /sys files: ad_select,
> num_grat_arp, num_unsol_na, primary_reselect and queue_id.
> - Enhance the documentation (README.Debian), to describe all the currently
> supported bond-* options.
> - Many other changes in the documentation.
> - Reverse the order of the arguments to most sysfs_* internal functions, for
> better readability.
>
> It was a hard work, because there really exist many such constraints. I
> fail to find enough time to insert the result of this work into
> Documentation/networking/bonding.txt, but still plan to do so, even if
> the result is documented in the script you looked at.
>
> Of course, it is possible to change the scripts in ifenslave-2.6 package
> to arrange for one more constraint. For as far as I understand, this
> would cause the Debian kernel that introduce the change we discuss about
> and all the future Debian kernels to be flagged with 'Breaks:
> ifenslave-2.6 (<< 1.1.0-20)'. I'm not really comfortable with this and
> the Debian kernel team need to be involved. I copied them.
>
> All that being said, I really advocate for less constraints on the sysfs
> setup. This is not strictly related to sysfs setup. If we eventually add
> a NETLINK interface for all the things we can setup using sysfs, we will
> face the exact same problem.
I was looking at ifenslave 1.1.0-20. If you look at Debian bug #641250
you will see a very similar report to what prompted Veaceslav to come up
with this patch and post it here.
ifenslave-2.6 (1.1.0-20) unstable; urgency=low
* Use dashes consistently for bonding options in README.Debian.
Closes: #639244
* Enslave slaves only after fully setting up the master. Closes: #641250
* Add build-arch and build-indep targets to debian/rules.
-- Guus Sliepen <guus@debian.org> Mon, 14 Nov 2011 11:36:21 +0100
ifenslave-2.6 (1.1.0-19) unstable; urgency=low
* Don't bother trying to move configuration files anymore. This is not an
issue anymore in for the next stable release, and it was broken anyway.
Closes: #626959
* Bump Standards-Version.
-- Guus Sliepen <guus@debian.org> Wed, 25 May 2011 18:42:32 +0200
ifenslave-2.6 (1.1.0-18) experimental; urgency=low
* Apply patch from Nicolas de Pesloüan:
- Major change: Check and fix the order in which the configuration is
written into /sys files, to ensure reliable results, according to the
tests done in the kernel (in drivers/net/bonding/bond_sysfs.c).
- Ensure that master is properly brought down when changing a parameter
that require it to be down.
- Ensure the master is brought up at the end of the setup, in the case
where ifup won't bring it up because it is currently configuring a slave.
- Add support for some previously unsupported /sys files: ad_select,
num_grat_arp, num_unsol_na, primary_reselect and queue_id.
- Enhance the documentation (README.Debian), to describe all the currently
supported bond-* options.
- Many other changes in the documentation.
- Reverse the order of the arguments to most sysfs_* internal functions, for
better readability.
* Upload to experimental due to the freeze.
-- Guus Sliepen <guus@debian.org> Tue, 21 Dec 2010 12:46:04 +0100
> I perfectly understand, as Veaceslav noted in another email that there
> are a lot of mode-specific initialization stuff that's present only in
> bond_enslave(), but I think this is what needs to be fixed... Those
> initialization stuff should be moved out of bond_enslave() and called at
> appropriate sime, from bond_enslave() and from other locations, in
> particular when changing mode.
>
I think Veaceslav is working on this, but there is significant
re-organization that is needed to make it work properly and make sure it
is tested. I could be wrong about how long it will take him, but to
test it properly it will take some time.
Since this problem seems like a pretty major problem and now Debian,
Fedora, RHEL, and Ubuntu all seem to have proper initialization scripts
to handle it, I stand behind my original ACK.
Reply to: