RE: [RFC PATCH 4/5] meta: add mounts class

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

On 2021-10-13 12:31, Jan Kiszka wrote:
> On 12.10.21 15:04, Adriaan Schmidt wrote:
> > +def get_requested_mounts(d, task=None):
> > +    if task is None:
> > +        task = d.getVar('BB_CURRENTTASK')
> > +        if not task:
> > +            bb.fatal("mount code running without task context!?")
> > +    if task.startswith("do_"):
> > +        task = task[3:]
> > +    mounts = (d.getVarFlag("do_" + task, 'mounts') or "").split()
> 
> You first strip the do_ prefix, only to prepend it again. Is that
> indended? If so, maybe flip it around and prepend this if missing.

This is how OE's sstate.bbclass does it. I agree it's ugly, but turning it around does not look much better either.
It's about accepting task names with and without "do_", but I'm not even sure both cases can happen here. I'll have another look an clean this up.

> > +def shorten_path(x, n=3):
> > +    xs = x.split('/')
> > +    if len(xs) <= n:
> > +        return '/'.join(xs)
> > +    return '.../'+'/'.join(xs[-3:])
> 
> Hope this does not cut off any differentiating information, even when
> just targeting logs.

It might. During development it makes reading the prints much easier, but for proper logs we want the complete paths (will change for next version).

> > +python mounts_task_prefunc () {
> > +    from collections import namedtuple
> > +    Mount = namedtuple('Mount', 'cmd source target count')
> > +    task = d.getVar('PN') + ':' + d.getVar('BB_CURRENTTASK')
> > +    lock = bb.utils.lockfile(d.getVar("MOUNTS_LOCK"))
> > +    mounts = get_requested_mounts(d)
> > +    mtab = read_mtab(d)
> > +    for cmd, source, target in mounts:
> > +        mt = mtab.get(target)
> > +        if mt:
> > +            count = mt.count + 1
> > +            bb.debug(1, f"mount({task}): already mounted
> {shorten_path(mt.source)} at {shorten_path(mt.target)}, cnt={count}")
> > +            mtab[target] = mt._replace(count=count)
> > +            continue
> > +        bb.debug(1, f"mount({task}): mounting {shorten_path(source)} at
> {shorten_path(target)}, cnt=1")
> > +        d.setVar('MOUNT_ARG_SOURCE', source)
> > +        d.setVar('MOUNT_ARG_TARGET', target)
> > +        bb.build.exec_func('mount_' + cmd, d)
> 
> Could this fail and leave the lock blocked behind?

Oh yes, good point! This also needs a try/catch, just like the umounts.

> > +addhandler mounts_cleanup
> > +python mounts_cleanup() {
> > +    # look through MOUNTS_DB for contexts
> > +    import glob
> > +    import time
> > +    base = d.getVar('MOUNTS_DB')
> > +    locks = glob.glob(base + "/*.mountlock")
> > +    tabs = glob.glob(base + "/*.mounttab")
> > +
> > +    # there should not be any locks?
> > +    if len(locks) > 0:
> > +        bb.error(f"mounts_cleanup: someone still holding lock?
> ({str(locks)})")
> > +
> > +    # cleanup any existing contexts
> > +    for mtab_file in tabs:
> > +        mtab = read_mtab(d, mtab_file)
> > +        if len(mtab) > 0:
> > +            bb.note(f"mounts_cleanup: {mtab_file.split('/')[-1]}")
> > +
> > +        done = []
> > +        for target, mt in mtab.items():
> > +            if mt.count < 0:
> > +                bb.error("count on {target} < 0. BUG!?!")
> > +                continue
> > +            if mt.count > 0:
> > +                bb.error(f"cound on {target} > 0. BUG!?!")
> > +
> > +            bb.note(f"mounts_cleanup: unmounting {target}")
> > +            for i in range(10):
> > +                try:
> > +                    d.setVar('UMOUNT_ARG_TARGET', target)
> > +                    bb.build.exec_func('umount_' + mt.cmd, d)
> > +                    done.append(target)
> > +                    break
> > +                except bb.process.ExecutionError as e:
> > +                    if e.exitcode == 32:
> > +                        # target busy
> > +                        time.sleep(1)
> > +                        continue
> > +                    else:
> > +                        bb.error(f"umount({task}): {str(e)}")
> > +                        done.append(target)
> > +                        break
> > +                bb.warn(f"mounts_cleanup: failed to umount {target}")
> > +                done.append(target)
> > +
> > +        for target in done:
> > +            del mtab[target]
> > +        write_mtab(d, mtab, mtab_file)
> > +}
> > +
> > +mounts_cleanup[eventmask] = "bb.event.BuildCompleted"
> > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> > index 7f5901d..4726eaf 100644
> > --- a/meta/conf/bitbake.conf
> > +++ b/meta/conf/bitbake.conf
> > @@ -113,7 +113,7 @@ PARALLEL_MAKE ?= "-j ${@bb.utils.cpu_count()}"
> >  BBINCLUDELOGS ??= "yes"
> >
> >  # Add event handlers for bitbake
> > -INHERIT += "isar-events"
> > +INHERIT += "mounts isar-events"
> >
> >  include conf/local.conf
> >  include conf/multiconfig/${BB_CURRENT_MC}.conf
> >
> 
> With all this in place, did you see warning in build_completed() triggering?

In "normal" cases, the cleanup code of the new mounts class is already doing it's job. I have still seen issues in the ci tests (at least in the pre-avocado variant), which may be due to multiconfig and the concurrency/sharing it brings. This needs some more work on my side.

Adriaan