RE: [RFC PATCH 5/5] meta: refactor to use the new mounting mechanism

["Schmidt, Adriaan" <adriaan.schmidt@siemens.com>]

On 2021-10-13 12:42, Jan Kiszka wrote:
> > -buildchroot_do_mounts() {
> > +buildchroot_task_prefunc() {
> >      sudo -s <<'EOSUDO'
> > -        ( flock 9
> >          set -e
> > -
> > -        mountpoint -q '${BUILDCHROOT_DIR}/isar-apt' ||
> > -            mount --bind '${REPO_ISAR_DIR}/${DISTRO}'
> '${BUILDCHROOT_DIR}/isar-apt'
> > -        mountpoint -q '${BUILDCHROOT_DIR}/downloads' ||
> > -            mount --bind '${DL_DIR}' '${BUILDCHROOT_DIR}/downloads'
> > -        mountpoint -q '${BUILDCHROOT_DIR}/dev' ||
> > -            mount --rbind /dev '${BUILDCHROOT_DIR}/dev'
> > -        mount --make-rslave '${BUILDCHROOT_DIR}/dev'
> > -        mountpoint -q '${BUILDCHROOT_DIR}/proc' ||
> > -            mount -t proc none '${BUILDCHROOT_DIR}/proc'
> > -        mountpoint -q '${BUILDCHROOT_DIR}/sys' ||
> > -            mount --rbind /sys '${BUILDCHROOT_DIR}/sys'
> > -        mount --make-rslave '${BUILDCHROOT_DIR}/sys'
> > -
> > -        # Mount base-apt if 'ISAR_USE_CACHED_BASE_REPO' is set
> > -        if [
> "${@repr(bb.utils.to_boolean(d.getVar('ISAR_USE_CACHED_BASE_REPO')))}" =
> 'True' ]
> > -        then
> > -            mkdir -p '${BUILDCHROOT_DIR}/base-apt'
> > -            mountpoint -q '${BUILDCHROOT_DIR}/base-apt' || \
> > -                mount --bind '${REPO_BASE_DIR}' '${BUILDCHROOT_DIR}/base-
> apt'
> > -        fi
> > -
> > -        # Refresh or remove /etc/resolv.conf at this chance
> > +        # Refresh or remove /etc/resolv.conf
> >          if [ "${@repr(bb.utils.to_boolean(d.getVar('BB_NO_NETWORK')))}" =
> 'True' ]
> >          then
> > -            rm -rf '${BUILDCHROOT_DIR}/etc/resolv.conf'
> > -        else
> > +            rm -f '${BUILDCHROOT_DIR}/etc/resolv.conf'
> > +        elif [ -d '${BUILDCHROOT_DIR}/etc' ]; then
> >              cp -L /etc/resolv.conf '${BUILDCHROOT_DIR}/etc'
> >          fi
> > -
> > -        ) 9>'${MOUNT_LOCKFILE}'
> 
> Which lock is protecting the remaining bits? If none, we need a word why
> that is fine.

The only remaining bit is writing resolv.conf, which I think is safe.
Will add a comment.


> > @@ -190,21 +187,6 @@ do_rootfs_finalize() {
> >              find "${ROOTFSDIR}/usr/bin" \
> >                  -maxdepth 1 -name 'qemu-*-static' -type f -delete
> >
> > -        mountpoint -q '${ROOTFSDIR}/isar-apt' && \
> > -            umount -l ${ROOTFSDIR}/isar-apt
> > -        rmdir --ignore-fail-on-non-empty ${ROOTFSDIR}/isar-apt
> > -
> > -        mountpoint -q '${ROOTFSDIR}/base-apt' && \
> > -            umount -l ${ROOTFSDIR}/base-apt
> > -        rmdir --ignore-fail-on-non-empty ${ROOTFSDIR}/base-apt
> 
> Who is deleting the empty isar-apt base-apt dirs?

No-one... The next version will add a feature, so we can define a mount as:

"bind,rmdir:${REPO_ISAR_DIR}/${DISTRO}:${BUILDCHROOT_DIR}/isar-apt"

and then the rmdir will run as part of unmounting.


> > @@ -54,7 +52,7 @@ python build_completed() {
> >      with open('/proc/mounts') as f:
> >          for line in f.readlines():
> >              if basepath in line:
> > -                bb.debug(1, '%s left mounted, unmounting...' %
> line.split()[1])
> > +                bb.warn('%s left mounted, unmounting...' %
> line.split()[1])
> 
> Ah, you are brave and make this a real warning again. Means you've never
> seen it?

The new mounts class comes with its own cleanup, so once that is finished, this one should not find anything.


> > @@ -153,13 +132,15 @@ do_rootfs_install[root_cleandirs] = "${ROOTFSDIR}"
> >  do_rootfs_install[vardeps] += "${ROOTFS_CONFIGURE_COMMAND}
> ${ROOTFS_INSTALL_COMMAND}"
> >  do_rootfs_install[depends] = "isar-bootstrap-${@'target' if
> d.getVar('ROOTFS_ARCH') == d.getVar('DISTRO_ARCH') else 'host'}:do_build"
> >  do_rootfs_install[deptask] = "do_deploy_deb"
> > +do_rootfs_install[mounts] = "${ROOTFS_MOUNTS}"
> > +do_rootfs_install[mounts-noauto] = "1"
> 
> Why "noauto"? How is it done otherwise?

"normally" the mount and unmount functions are added to do_task[prefuncs/postfuncs] and run automatically, with some additional code to catch task-failed events and do the unmounting in that case.

The mounts-noauto flag is in case we need mounting/unmounting to happen at different times. Then the functions need to be called explicitly by the task (cf. below: do_rootfs_install adds them to its list of cmds).
If the task fails, there is no way to know if unmounting still needs to happen, but this case should be rare, and the final cleanup handler will catch that.

> >  python do_rootfs_install() {
> >      configure_cmds = (d.getVar("ROOTFS_CONFIGURE_COMMAND", True) or "").split()
> >      install_cmds = (d.getVar("ROOTFS_INSTALL_COMMAND", True) or "").split()
> >
> >      # Mount after configure commands, so that they have time to copy
> >      # 'isar-apt' (sdkchroot):
> > -    cmds = ['rootfs_prepare'] + configure_cmds + ['rootfs_do_mounts'] + install_cmds
> > +    cmds = ['rootfs_prepare'] + configure_cmds + ['mounts_task_prefunc'] + install_cmds + ['mounts_task_postfunc']


> Maybe split this up into multiple conversions of the same type?

Yes, V2 will split this commit to make review easier.

Adriaan