[PATCH v2 0/2] Fix sporadic failures in do_wic_image

[PATCH v2 0/2] Fix sporadic failures in do_wic_image

[Felix Moessbauer]

Changes since v1:
- remove patches 3 and 4 (refactoring and breakup of do_rootfs_finalize task)
- use exclusive lock in do_wic_image

Best regards,
Felix

Felix Moessbauer (2):
  fix typo in do_rootfs_finalize
  execute do_wic_image under a lock to ensure mountpoints remain mounted

 meta/classes/image.bbclass   |  2 +-
 meta/classes/wic-img.bbclass | 34 +++++++++++++++++++++-------------
 2 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] fix typo in do_rootfs_finalize

[Felix Moessbauer]

This fixes a typo to make sure the same mountpoint is checked for
existence and unmounted.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta/classes/image.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index ec93cab..1f39690 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -200,7 +200,7 @@ do_rootfs_finalize() {
 
         mountpoint -q '${ROOTFSDIR}/dev' && \
             umount -l ${ROOTFSDIR}/dev
-        mountpoint -q '${ROOTFSDIR}/sys' && \
+        mountpoint -q '${ROOTFSDIR}/proc' && \
             umount -l ${ROOTFSDIR}/proc
         mountpoint -q '${ROOTFSDIR}/sys' && \
             umount -l ${ROOTFSDIR}/sys
-- 
2.30.2

[PATCH v2 2/2] execute do_wic_image under a lock to ensure mountpoints remain mounted

[Felix Moessbauer]

This patch adds the isar.lock to the do_wic_image task to ensure that
the mountpoints are not unmounted by a simultaneously running wic task.
Further, it makes mounting more robust by executing the
wic task in a try / finally handler so that the mount points are
unmounted even if the task itself fails.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta/classes/wic-img.bbclass | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
index d849ad9..cd97fa9 100644
--- a/meta/classes/wic-img.bbclass
+++ b/meta/classes/wic-img.bbclass
@@ -138,23 +138,40 @@ python check_for_wic_warnings() {
 
 do_wic_image[file-checksums] += "${WKS_FILE_CHECKSUM}"
 python do_wic_image() {
-    bb.build.exec_func("generate_wic_image", d)
+    lock = bb.utils.lockfile(d.getVar("REPO_ISAR_DIR") + "/isar.lock", shared=False)
+    bb.build.exec_func("wic_do_mounts", d)
+    try:
+        bb.build.exec_func("generate_wic_image", d)
+    finally:
+        bb.build.exec_func("wic_undo_mounts", d)
+        bb.utils.unlockfile(lock)
     bb.build.exec_func("check_for_wic_warnings", d)
 }
 addtask wic_image before do_image after do_image_tools
 
-generate_wic_image() {
+wic_do_mounts() {
     buildchroot_do_mounts
     sudo -s <<'EOSUDO'
-        ( flock 9
         for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
             mkdir -p ${BUILDCHROOT_DIR}/$dir
             if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
                 mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
             fi
         done
-        ) 9>${MOUNT_LOCKFILE}
 EOSUDO
+}
+
+wic_undo_mounts() {
+    sudo -s <<'EOSUDO'
+        for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
+            if mountpoint -q ${BUILDCHROOT_DIR}/$dir; then
+                umount ${BUILDCHROOT_DIR}/$dir
+            fi
+        done
+EOSUDO
+}
+
+generate_wic_image() {
     export FAKEROOTCMD=${FAKEROOTCMD}
     export BUILDDIR=${BUILDDIR}
     export MTOOLS_SKIP_CHECK=1
@@ -200,13 +217,4 @@ EOSUDO
     done
     rm -rf ${BUILDCHROOT_DIR}/${WICTMP}
     rm -rf ${IMAGE_ROOTFS}/../pseudo
-    sudo -s <<'EOSUDO'
-        ( flock 9
-        for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
-            if mountpoint -q ${BUILDCHROOT_DIR}/$dir; then
-                umount ${BUILDCHROOT_DIR}/$dir
-            fi
-        done
-        ) 9>${MOUNT_LOCKFILE}
-EOSUDO
 }
-- 
2.30.2

Re: [PATCH v2 2/2] execute do_wic_image under a lock to ensure mountpoints remain mounted

[Jan Kiszka]

On 23.09.21 15:12, Felix Moessbauer wrote:
> This patch adds the isar.lock to the do_wic_image task to ensure that
> the mountpoints are not unmounted by a simultaneously running wic task.

As long as we are not bookkeeping mount and umounts, it's better to
avoid locking of the whole task, only synchronize mounting against
competing runs, and leave umount to the cleanup handler.

Also, the lock is way too broad when it is repo-anchored. Only users of
the same buildchroot compete here.

Jan

> Further, it makes mounting more robust by executing the
> wic task in a try / finally handler so that the mount points are
> unmounted even if the task itself fails.
>
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>  meta/classes/wic-img.bbclass | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
> index d849ad9..cd97fa9 100644
> --- a/meta/classes/wic-img.bbclass
> +++ b/meta/classes/wic-img.bbclass
> @@ -138,23 +138,40 @@ python check_for_wic_warnings() {
>
>  do_wic_image[file-checksums] += "${WKS_FILE_CHECKSUM}"
>  python do_wic_image() {
> -    bb.build.exec_func("generate_wic_image", d)
> +    lock = bb.utils.lockfile(d.getVar("REPO_ISAR_DIR") + "/isar.lock", shared=False)
> +    bb.build.exec_func("wic_do_mounts", d)
> +    try:
> +        bb.build.exec_func("generate_wic_image", d)
> +    finally:
> +        bb.build.exec_func("wic_undo_mounts", d)
> +        bb.utils.unlockfile(lock)
>      bb.build.exec_func("check_for_wic_warnings", d)
>  }
>  addtask wic_image before do_image after do_image_tools
>
> -generate_wic_image() {
> +wic_do_mounts() {
>      buildchroot_do_mounts
>      sudo -s <<'EOSUDO'
> -        ( flock 9
>          for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
>              mkdir -p ${BUILDCHROOT_DIR}/$dir
>              if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
>                  mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
>              fi
>          done
> -        ) 9>${MOUNT_LOCKFILE}
>  EOSUDO
> +}
> +
> +wic_undo_mounts() {
> +    sudo -s <<'EOSUDO'
> +        for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
> +            if mountpoint -q ${BUILDCHROOT_DIR}/$dir; then
> +                umount ${BUILDCHROOT_DIR}/$dir
> +            fi
> +        done
> +EOSUDO
> +}
> +
> +generate_wic_image() {
>      export FAKEROOTCMD=${FAKEROOTCMD}
>      export BUILDDIR=${BUILDDIR}
>      export MTOOLS_SKIP_CHECK=1
> @@ -200,13 +217,4 @@ EOSUDO
>      done
>      rm -rf ${BUILDCHROOT_DIR}/${WICTMP}
>      rm -rf ${IMAGE_ROOTFS}/../pseudo
> -    sudo -s <<'EOSUDO'
> -        ( flock 9
> -        for dir in ${BBLAYERS} ${STAGING_DIR} ${SCRIPTSDIR} ${BITBAKEDIR}; do
> -            if mountpoint -q ${BUILDCHROOT_DIR}/$dir; then
> -                umount ${BUILDCHROOT_DIR}/$dir
> -            fi
> -        done
> -        ) 9>${MOUNT_LOCKFILE}
> -EOSUDO
>  }
>