[Devel] [PATCH 2/3] allow for distro-specific fix ups at creation time.

[Kir Kolyshkin]

On 05/20/2013 01:40 PM, Glauber Costa wrote:
> On 05/21/2013 12:32 AM, Kir Kolyshkin wrote:
>> On 05/20/2013 05:49 AM, Glauber Costa wrote:
>>> From: Glauber Costa <glommer at parallels.com>
>>>
>>> We will need that infrastucture when running with Linux upstream,
>>> since some
>>> support is very unlikely to ever land in the Kernel.  We need to do
>>> things like
>>> account for the fact that udev may kick in and destroy all the setup
>>> we have
>>> done for /dev.  Since preemptively preventing those actions to happen
>>> is very
>>> hard ( as an example, centos6 init binary will issue mount syscalls
>>> itself),
>>> the most robust approach is to insert a script that will be run
>>> shortly after
>>> rc.sysinit. Although this can't guarantee that it will run
>>> *immediately* after,
>>> it should be early enough to undo every harmful operation done by
>>> /sbin/init
>>> and rc.sysinit, therefore allowing operation to continue freely.
>> I understand that
>>
>> 1 this is something redhat/centos-specific?
>> 2 there is no need to do that for debian/ubuntu etc?
>>
> No, thi is old-udev specific.
> In more up to date distributions, udev is already aware that it might be
> being containerized and won't do all those initialization things.
>
> I haven't tested *all* distros we support, but among the most up to date
> in each variant, only centos6 needs it (ubuntu 12 runs fine, suse 12
> runs fine)
>
>> Am I right?
> Yes and no, see above.
>
>>> Signed-off-by: Glauber Costa <glommer at parallels.com>
>>> ---
>>>    etc/dists/redhat.conf       |  1 +
>>>    etc/dists/scripts/fixups.sh | 43
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>    include/dist.h              |  2 ++
>>>    include/env.h               |  3 ++-
>>>    src/lib/dist.c              | 10 +++++++++-
>>>    src/lib/env.c               | 10 ++++++----
>>>    src/lib/exec.c              |  2 +-
>>>    src/lib/hooks_ct.c          | 35 +++++++++++++++++++++++++++++++++++
>>>    8 files changed, 99 insertions(+), 7 deletions(-)
>>>    create mode 100755 etc/dists/scripts/fixups.sh
>>>
>>> diff --git a/etc/dists/redhat.conf b/etc/dists/redhat.conf
>>> index 727461d..49226c5 100644
>>> --- a/etc/dists/redhat.conf
>>> +++ b/etc/dists/redhat.conf
>>> @@ -25,3 +25,4 @@ SET_DNS=set_dns.sh
>>>    SET_USERPASS=set_userpass.sh
>>>    SET_UGID_QUOTA=set_ugid_quota.sh
>>>    POST_CREATE=postcreate.sh
>>> +CT_FIXUP=fixups.sh
>>> diff --git a/etc/dists/scripts/fixups.sh b/etc/dists/scripts/fixups.sh
>>> new file mode 100755
>>> index 0000000..da3fd24
>>> --- /dev/null
>>> +++ b/etc/dists/scripts/fixups.sh
>>> @@ -0,0 +1,43 @@
>>> +#!/bin/bash
>> Why bash? It is not available in some distros by default (notably Debian).
>> So, unless we do have a reason to use bash-specific features we'd rather
>> not to.
>>
> Will change to /bin/sh
>
>>> +#  Copyright (C) 2000-2009, Parallels, Inc. All rights reserved.
>> :))
> That was written by a past me.
>
>>> +#
>>> +#  This program is free software; you can redistribute it and/or modify
>>> +#  it under the terms of the GNU General Public License as published by
>>> +#  the Free Software Foundation; either version 2 of the License, or
>>> +#  (at your option) any later version.
>>> +#
>>> +#  This program is distributed in the hope that it will be useful,
>>> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +#  GNU General Public License for more details.
>>> +#
>>> +#  You should have received a copy of the GNU General Public License
>>> +#  along with this program; if not, write to the Free Software
>>> +#  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
>>> 02111-1307  USA
>>> +#
>>> +#
>>> +#  Run-time fixup for legacy distributions that will rely on udevd to
>>> populate
>>> +#  their /dev. Those distributions will effectively mount an empty
>>> tmpfs on top
>>> +#  of /dev, defeating any setup we may have done. This includes
>>> RHEL-based
>>> +#  distributions up to RHEL6. We are better off not allowing that to
>>> run at all
>>> +
>>> +function fixup_udev()
>> s/function //
>>
>> as this is bashism
>>
> ok

There is an easy way to check for such things if you don't know/remember 
those:
use checkbashisms script (in Fedora it is available in rpmdevtools package).

Having said that, if your script is and will be strictly for non-Debian 
based distros
(such as then the code is in rpm %post scripts or triggers) and there is 
a clear need
for bash (like the bash code is easier/better) you are free to use bash.

>
>>> index bde94ab..8047cf7 100644
>>> --- a/src/lib/dist.c
>>> +++ b/src/lib/dist.c
>>> @@ -36,7 +36,8 @@ static struct distr_conf {
>>>        {"SET_DNS", SET_DNS},
>>>        {"SET_USERPASS", SET_USERPASS},
>>>        {"SET_UGID_QUOTA", SET_UGID_QUOTA},
>>> -    {"POST_CREATE", POST_CREATE}
>>> +    {"POST_CREATE", POST_CREATE},
>>> +    {"CT_FIXUP", CT_FIXUP }
>> 1. Please add comma after } so the next author won't have to patch your
>> line.
>> I.e.
>>
>> +    {"CT_FIXUP", CT_FIXUP },
>>
>>
>> 2. Maybe we can give it more concrete name? FIXUP is, well, anything.
>>
>> This is very unfortunate. All the other distro scripts we maintain have
>> very
>> well defined names, associated actions and ways to run. They are described
>> in details in etc/dists/scripts/distribution.conf-template file.
>>
>> In this case we have a script with a name that doesn't tell us anything,
>> is only run for redhat* distros, only when user ns is available/enabled,
>> and in an extremely strange way -- not by executing it in a context of CT
>> like all other scripts do, but by putting it to /etc/rc{3,5}d/S00fixups.sh
>> before CT start. More to say, the logic of putting it this way is built
>> into
>> vzctl C code.
>>
>> Distro scripts meant to be flexible and configurable. This one is quite
>> the opposite.
>>
>> Can we make it more flexible and configurable? Like, have a PRE_START
>> script
>> which is to be executed in context of CT right before starting its init
>> process?
> No, because that needs to run after init.
> init + rc.sysinit, is where the harm lies. /sbin/init will remount proc
> undoing our /proc mount (this *still* does not harm us, because we have
> no /proc fuse replacement, but that is the plan), and then in rc.sysinit
> all the udev magic is done.
>
> So we need something that runs after rc.sysinit.
>
>> That script would then do all the logic of setting up those /etc/rc.d/
>> scripts.
>> If needed, we can pass some parameters to this script through the
>> environment,
>> like if we are enabling user ns.
>>
> If you can tell me a way to guarantee it will be run after rc.sysinit, I
> am fine with it.