Re: [PATCH] isar: Clean mount point on bitbake exit

[Alexander Smirnov <asmirnov@ilbers.de>]

Hi,

On 02/09/2018 06:04 PM, Henning Schild wrote:
> The new next works for me, thanks!
> 

Thank you for the quick feedback!

> Henning
> 
> Am Fri, 9 Feb 2018 14:19:43 +0100
> schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> 
>> Am Fri, 9 Feb 2018 16:08:01 +0300
>> schrieb Alexander Smirnov <asmirnov@ilbers.de>:
>>
>>> On 02/09/2018 03:41 PM, Jan Kiszka wrote:
>>>> On 2018-02-09 13:40, Henning Schild wrote:
>>>>> Am Fri, 9 Feb 2018 13:35:15 +0100
>>>>> schrieb Jan Kiszka <jan.kiszka@siemens.com>:
>>>>>     
>>>>>> On 2018-02-09 13:33, [ext] Henning Schild wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> this patch is causing problems when building in a docker
>>>>>>> container, because sysfs can only be mounted ro. (Subject:
>>>>>>> current next bash in buildchroot problem)
>>>>>>> Now we could discuss whether we should relax the security of
>>>>>>> our containers even more, or whether Isar should care about
>>>>>>> that use-case.
>>>>>>>
>>>>>>> But this patch actually does several things at a time, it
>>>>>>> changes >>>> the way we mount and adds three new mounts. I
>>>>>>> would suggest to
>>>
>>> Actually not. It adds the only one new mount for sysfs. /proc was
>>> mounted inside do_build, /dev was mounted inside configscript.sh,
>>> so this is a kind of consolidation of these calls in one place.
>>
>> Ok, in that case sys should be in a separate patch.
>>
>>> I have no case for sysfs, so probably we could drop it for now.
>>> Please let me know ASAP because I'm going to release v0.4.
>>
>> I brought up sysfs as part of a "complete" chroot. If we do not have a
>> real case for it yet, and it hurts us in some docker-corner-case ...
>> leave it out for now.
>>
>> As a general advice for the release. Most Isar-users probably consume
>> git anyways. And turning next directly into a release sounds like a
>> bad idea. I would first update master and wait some time until you get
>> bug-reports for your new master.
>> But hey, it is just a tag for people that like tarballs, might as well
>> leave some bugs in there ;).

I see your point, yes, some products have such practice to provide 
release candidates and then official releases.

But for me this looks like the overhead with current Isar size.

1. At the moment there are several series in the mailing list that 
assume Isar-core refactoring, so 'next' branch could be populated quite 
fast withing next days/weeks by new features.

2. If somebody has found an issue with current 'master' and sent the 
fix, I mostly like to apply this patch to 'next' and then merge whole 
current 'next' to master to avoid headache with rebasing and non-linear 
history. So this means that 'master' will be populated by new feature 
which also needs some time for field reports.

This could lead to have releases very rarely, while in general 'master' 
contains working code that could be used. Also if users prefer to use 
official releases in their products, the functionality gap between two 
neighbor ones could be too big.

Alex

>>
>>>>>>> split it up so we can discuss the issues with dev and sys while
>>>>>>> already merging the rest.
>>>
>>> There is no official Docker support in Isar, so until there will be
>>> a document which specifies the container configuration, it really
>>> would be inefficient to block contributions. We can't support
>>> everything everywhere.
>>
>> Fair enough, but i can assure you that a lot of people build Isar
>> images in docker. I could even name the container for that etc. And
>> until that becomes an official feature we can still try and make sure
>> we do not break it.
>>
>> Henning
>>
>>>>>>
>>>>>> I think (didn't check if there was an update of next this
>>>>>> morning) it works for me - in Docker. How are you starting the
>>>>>> container?
>>>>>
>>>>> docker run -e USER_ID=$(id -u) --rm -t -i --cap-add=SYS_ADMIN
>>>>> --cap-add=MKNOD --device $(/sbin/losetup -f) -e ... proxy
>>>>> stuff ...
>>>
>>> Do you have instructions how to build Isar in container, so at least
>>> I could be able to reproduce the issue?
>>>
>>> Alex
>>>
>>>    
>>>> Try adding --privileged - that's needed for binfmt anyway.
>>>>
>>>> Jan
>>>>      
>>>>> inside my sysfs is ro, a bind-mount of sysfs is ro and a "mount
>>>>> -t sysfs ..." will be ro. Maybe i could add a "-o rw" to the
>>>>> mount but for now i just reverted the two patches that deal with
>>>>> mounting.
>>>>>
>>>>> Might also be a difference in our host systems.
>>>>>
>>>>> Henning
>>>>>     
>>>>>> Jan
>>>>>>     
>>>>>>>
>>>>>>> Henning
>>>>>>>
>>>>>>> Am Tue, 6 Feb 2018 22:55:16 +0300
>>>>>>> schrieb Alexander Smirnov <asmirnov@ilbers.de>:
>>>>>>>         
>>>>>>>> 8<--
>>>>>>>>
>>>>>>>> That's it! Branch 'asmirnov/devel', please test and enjoy :-)
>>>>>>>>
>>>>>>>> 8<--
>>>>>>>>
>>>>>>>> Now each multiconfig has registered handler for BuildCompleted
>>>>>>>> event (see class 'isar-event.bbclass'). Moreover, the
>>>>>>>> '/proc/mounts' file contains all the active mounts. In
>>>>>>>> addition, from event handler we could derive all the
>>>>>>>> variables like ${TMPDIR}, ${DISTRO} etc. So it's possible to
>>>>>>>> find all the active mounts for current multiconfig and clean
>>>>>>>> them.
>>>>>>>>
>>>>>>>> NOTE: if build is interrupted by double ^C, some mount points
>>>>>>>> could stay uncleaned. This is caused by remaining processes
>>>>>>>> started by bitbake, for example:
>>>>>>>>    - 'chroot build.sh ...'
>>>>>>>>    - 'multistrap ...'
>>>>>>>>
>>>>>>>> So please be careful when interrupting build.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
>>>>>>>> ---
>>>>>>>>    meta-isar/recipes-core/images/isar-image-base.bb   | 11
>>>>>>>> ++++------ meta/classes/dpkg-base.bbclass
>>>>>>>> | 12 ++++-------
>>>>>>>> meta/classes/isar-events.bbclass                   | 15
>>>>>>>> +++++++++++---
>>>>>>>> meta/recipes-devtools/buildchroot/buildchroot.bb   | 24
>>>>>>>> +++++++++------------- .../buildchroot/files/configscript.sh |
>>>>>>>> 4 ---- .../buildchroot/files/download_dev-random          | 13
>>>>>>>> ------------ 6 files changed, 30 insertions(+), 49
>>>>>>>> deletions(-) delete mode 100644
>>>>>>>> meta/recipes-devtools/buildchroot/files/download_dev-random
>>>>>>>>
>>>>>>>> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb
>>>>>>>> b/meta-isar/recipes-core/images/isar-image-base.bb index
>>>>>>>> e359ac3..8ddbabb 100644 ---
>>>>>>>> a/meta-isar/recipes-core/images/isar-image-base.bb +++
>>>>>>>> b/meta-isar/recipes-core/images/isar-image-base.bb @@ -55,14
>>>>>>>> +55,10 @@ do_rootfs() { -e
>>>>>>>> 's|##ISAR_DISTRO_SUITE##|${DEBDISTRONAME}|g' \
>>>>>>>> "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
>>>>>>>> +    # Do not use bitbake flag [dirs] here because this folder
>>>>>>>> should have
>>>>>>>> +    # specific ownership.
>>>>>>>>        [ ! -d ${IMAGE_ROOTFS}/proc ] && sudo install -d -o 0 -g
>>>>>>>> 0 -m 555 ${IMAGE_ROOTFS}/proc sudo mount -t proc none
>>>>>>>> ${IMAGE_ROOTFS}/proc
>>>>>>>> -    _do_rootfs_cleanup() {
>>>>>>>> -        ret=$?
>>>>>>>> -        sudo umount ${IMAGE_ROOTFS}/proc 2>/dev/null || true
>>>>>>>> -        (exit $ret) || bb_exit_handler
>>>>>>>> -    }
>>>>>>>> -    trap '_do_rootfs_cleanup' EXIT
>>>>>>>>    
>>>>>>>>        # Create root filesystem. We must use sudo -E here to
>>>>>>>> preserve the environment # because of proxy settings
>>>>>>>> @@ -72,5 +68,6 @@ do_rootfs() {
>>>>>>>>        sudo chroot ${IMAGE_ROOTFS} /${DISTRO_CONFIG_SCRIPT}
>>>>>>>> ${MACHINE_SERIAL} ${BAUDRATE_TTY} \ ${ROOTFS_DEV}
>>>>>>>>        sudo rm "${IMAGE_ROOTFS}/${DISTRO_CONFIG_SCRIPT}"
>>>>>>>> -    _do_rootfs_cleanup
>>>>>>>> +
>>>>>>>> +    sudo umount ${IMAGE_ROOTFS}/proc 2>/dev/null || true
>>>>>>>>    }
>>>>>>>> diff --git a/meta/classes/dpkg-base.bbclass
>>>>>>>> b/meta/classes/dpkg-base.bbclass index 5d5a924..a34c21f 100644
>>>>>>>> --- a/meta/classes/dpkg-base.bbclass
>>>>>>>> +++ b/meta/classes/dpkg-base.bbclass
>>>>>>>> @@ -20,15 +20,11 @@ dpkg_runbuild() {
>>>>>>>>    do_build() {
>>>>>>>>        mkdir -p ${BUILDROOT}
>>>>>>>>        sudo mount --bind ${WORKDIR} ${BUILDROOT}
>>>>>>>> -    _do_build_cleanup() {
>>>>>>>> -        ret=$?
>>>>>>>> -        sudo umount ${BUILDROOT} 2>/dev/null || true
>>>>>>>> -        sudo rmdir ${BUILDROOT} 2>/dev/null || true
>>>>>>>> -        (exit $ret) || bb_exit_handler
>>>>>>>> -    }
>>>>>>>> -    trap '_do_build_cleanup' EXIT
>>>>>>>> +
>>>>>>>>        dpkg_runbuild
>>>>>>>> -    _do_build_cleanup
>>>>>>>> +
>>>>>>>> +    sudo umount ${BUILDROOT} 2>/dev/null || true
>>>>>>>> +    sudo rmdir ${BUILDROOT} 2>/dev/null || true
>>>>>>>>    }
>>>>>>>>    
>>>>>>>>    # Install package to Isar-apt
>>>>>>>> diff --git a/meta/classes/isar-events.bbclass
>>>>>>>> b/meta/classes/isar-events.bbclass index 55fc106..ae0f791
>>>>>>>> 100644 --- a/meta/classes/isar-events.bbclass
>>>>>>>> +++ b/meta/classes/isar-events.bbclass
>>>>>>>> @@ -11,10 +11,19 @@ python isar_handler () {
>>>>>>>>        devnull = open(os.devnull, 'w')
>>>>>>>>    
>>>>>>>>        if isinstance(e, bb.event.BuildCompleted):
>>>>>>>> -        bchroot = d.getVar('BUILDCHROOT_DIR', True)
>>>>>>>> +        tmpdir = d.getVar('TMPDIR', True)
>>>>>>>> +        distro = d.getVar('DISTRO', True)
>>>>>>>> +        arch = d.getVar('DISTRO_ARCH', True)
>>>>>>>>    
>>>>>>>> -        # Clean up buildchroot
>>>>>>>> -        subprocess.call('/usr/bin/sudo /bin/umount ' +
>>>>>>>> bchroot
>>>>>>>> + '/isar-apt || /bin/true', stdout=devnull, stderr=devnull,
>>>>>>>> shell=True)
>>>>>>>> +        w = tmpdir + '/work/' + distro + '-' + arch
>>>>>>>> +
>>>>>>>> +        # '/proc/mounts' contains all the active mounts, so
>>>>>>>> knowing 'w' we
>>>>>>>> +        # could get the list of mounts for the specific
>>>>>>>> multiconfig and
>>>>>>>> +        # clean them.
>>>>>>>> +        with open('/proc/mounts', 'rU') as f:
>>>>>>>> +            for line in f:
>>>>>>>> +                if w in line:
>>>>>>>> +                    subprocess.call('sudo umount -f ' +
>>>>>>>> line.split()[1], stdout=devnull, stderr=devnull, shell=True)
>>>>>>>>        devnull.close()
>>>>>>>>    }
>>>>>>>> diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb
>>>>>>>> b/meta/recipes-devtools/buildchroot/buildchroot.bb index
>>>>>>>> 304c67e..df9df19 100644 ---
>>>>>>>> a/meta/recipes-devtools/buildchroot/buildchroot.bb +++
>>>>>>>> b/meta/recipes-devtools/buildchroot/buildchroot.bb @@ -12,7
>>>>>>>> +12,6 @@ FILESPATH =.
>>>>>>>> "${LAYERDIR_core}/recipes-devtools/buildchroot/files:"
>>>>>>>> SRC_URI = "file://multistrap.conf.in \ file://configscript.sh
>>>>>>>> \ file://setup.sh \
>>>>>>>> -           file://download_dev-random \
>>>>>>>>               file://build.sh"
>>>>>>>>    PV = "1.0"
>>>>>>>>    
>>>>>>>> @@ -32,8 +31,10 @@ BUILDCHROOT_PREINSTALL ?= "gcc \
>>>>>>>>    WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}"
>>>>>>>>    
>>>>>>>>    do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>>>>>>> -do_build[dirs] = "${WORKDIR}/hooks_multistrap \
>>>>>>>> -                  ${BUILDCHROOT_DIR}/isar-apt"
>>>>>>>> +do_build[dirs] = "${BUILDCHROOT_DIR}/isar-apt \
>>>>>>>> +                  ${BUILDCHROOT_DIR}/dev \
>>>>>>>> +                  ${BUILDCHROOT_DIR}/proc \
>>>>>>>> +                  ${BUILDCHROOT_DIR}/sys"
>>>>>>>>    do_build[depends] = "isar-apt:do_cache_config"
>>>>>>>>    
>>>>>>>>    do_build() {
>>>>>>>> @@ -41,7 +42,6 @@ do_build() {
>>>>>>>>    
>>>>>>>>        chmod +x "${WORKDIR}/setup.sh"
>>>>>>>>        chmod +x "${WORKDIR}/configscript.sh"
>>>>>>>> -    install -m 755 "${WORKDIR}/download_dev-random"
>>>>>>>> "${WORKDIR}/hooks_multistrap/"
>>>>>>>>        # Multistrap accepts only relative path in configuration
>>>>>>>> files, so get it: cd ${TOPDIR}
>>>>>>>> @@ -60,15 +60,6 @@ do_build() {
>>>>>>>>            -e
>>>>>>>> 's|##DIR_HOOKS##|./'"$WORKDIR_REL"'/hooks_multistrap|g' \
>>>>>>>> "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
>>>>>>>> -    [ ! -d ${BUILDCHROOT_DIR}/proc ] && install -d -m 555
>>>>>>>> ${BUILDCHROOT_DIR}/proc
>>>>>>>> -    sudo mount -t proc none ${BUILDCHROOT_DIR}/proc
>>>>>>>> -    _do_build_cleanup() {
>>>>>>>> -        ret=$?
>>>>>>>> -        sudo umount ${BUILDCHROOT_DIR}/proc 2>/dev/null ||
>>>>>>>> true
>>>>>>>> -        (exit $ret) || bb_exit_handler
>>>>>>>> -    }
>>>>>>>> -    trap '_do_build_cleanup' EXIT
>>>>>>>> -
>>>>>>>>        do_setup_mounts
>>>>>>>>    
>>>>>>>>        # Create root filesystem
>>>>>>>> @@ -79,7 +70,6 @@ do_build() {
>>>>>>>>    
>>>>>>>>        # Configure root filesystem
>>>>>>>>        sudo chroot ${BUILDCHROOT_DIR} /configscript.sh
>>>>>>>> -    _do_build_cleanup
>>>>>>>>    
>>>>>>>>        do_cleanup_mounts
>>>>>>>>    }
>>>>>>>> @@ -96,10 +86,16 @@ do_setup_mounts[stamp-extra-info] =
>>>>>>>> "${DISTRO}-${DISTRO_ARCH}"
>>>>>>>>    do_setup_mounts() {
>>>>>>>>        sudo mount --bind ${DEPLOY_DIR_APT}/${DISTRO}
>>>>>>>> ${BUILDCHROOT_DIR}/isar-apt
>>>>>>>> +    sudo mount --bind /dev ${BUILDCHROOT_DIR}/dev
>>>>>>>> +    sudo mount -t proc none ${BUILDCHROOT_DIR}/proc
>>>>>>>> +    sudo mount -t sysfs none ${BUILDCHROOT_DIR}/sys
>>>>>>>>    }
>>>>>>>>    
>>>>>>>>    addtask setup_mounts after do_build
>>>>>>>>    
>>>>>>>>    do_cleanup_mounts() {
>>>>>>>>        sudo umount ${BUILDCHROOT_DIR}/isar-apt 2>/dev/null ||
>>>>>>>> true
>>>>>>>> +    sudo umount ${BUILDCHROOT_DIR}/dev 2>/dev/null || true
>>>>>>>> +    sudo umount ${BUILDCHROOT_DIR}/proc 2>/dev/null || true
>>>>>>>> +    sudo umount ${BUILDCHROOT_DIR}/sys 2>/dev/null || true
>>>>>>>>    }
>>>>>>>> diff --git
>>>>>>>> a/meta/recipes-devtools/buildchroot/files/configscript.sh
>>>>>>>> b/meta/recipes-devtools/buildchroot/files/configscript.sh
>>>>>>>> index 9813c9a..524e50c 100644 ---
>>>>>>>> a/meta/recipes-devtools/buildchroot/files/configscript.sh +++
>>>>>>>> b/meta/recipes-devtools/buildchroot/files/configscript.sh @@
>>>>>>>> -39,10 +39,6 @@ export LC_ALL=C LANGUAGE=C LANG=C #run pre
>>>>>>>> installation script /var/lib/dpkg/info/dash.preinst install
>>>>>>>> -# apt-get http method, gpg require /dev/null
>>>>>>>> -mount -t devtmpfs -o mode=0755,nosuid devtmpfs /dev
>>>>>>>> -
>>>>>>>>    #configuring packages
>>>>>>>>    dpkg --configure -a
>>>>>>>>    apt-get update
>>>>>>>> -umount /dev
>>>>>>>> diff --git
>>>>>>>> a/meta/recipes-devtools/buildchroot/files/download_dev-random
>>>>>>>> b/meta/recipes-devtools/buildchroot/files/download_dev-random
>>>>>>>> deleted file mode 100644 index 5b5b96b..0000000 ---
>>>>>>>> a/meta/recipes-devtools/buildchroot/files/download_dev-random
>>>>>>>> +++ /dev/null @@ -1,13 +0,0 @@
>>>>>>>> -#!/bin/sh
>>>>>>>> -
>>>>>>>> -set -e
>>>>>>>> -
>>>>>>>> -readonly ROOTFS="$1"
>>>>>>>> -
>>>>>>>> -mknod "${ROOTFS}/dev/random" c 1 8
>>>>>>>> -chmod 640 "${ROOTFS}/dev/random"
>>>>>>>> -chown 0:0 "${ROOTFS}/dev/random"
>>>>>>>> -
>>>>>>>> -mknod "${ROOTFS}/dev/urandom" c 1 9
>>>>>>>> -chmod 640 "${ROOTFS}/dev/urandom"
>>>>>>>> -chown 0:0 "${ROOTFS}/dev/urandom"
>>>>>>>         
>>>>>>     
>>>>>     
>>>>      
>>>    
>>
> 

-- 
With best regards,
Alexander Smirnov

ilbers GmbH
Baierbrunner Str. 28c
D-81379 Munich
+49 (89) 122 67 24-0
http://ilbers.de/
Commercial register Munich, HRB 214197
General manager: Baurzhan Ismagulov