#11662 - wic should mount /boot

wic updates /etc/fstab on root partition if there are valid mount points in .wks

When wic runs from bitbake this can cause incorrect results or even breakage of other tasks working with rootfs directory in parallel with do_image_wic.

Implemented copying rootfs directory to a temporary location using copyhardlinktree before updating fstab to avoid conflicts with other tasks working with rootfs.

index aa9cc9f..f707365 100644

scripts/lib/wic/plugins/imager/direct.py +2 -0

@@ -32,6 +32,8 @@ import uuid
 
 from time import strftime
 
+from oe.path import copyhardlinktree
+
 from wic import WicError
 from wic.filemap import sparse_copy
 from wic.ksparser import KickStart, KickStartError

scripts/lib/wic/plugins/imager/direct.py +3 -1

+            copyhardlinktree(image_rootfs, new_rootfs)
+            fstab_path = os.path.join(new_rootfs, 'etc/fstab')
 
             with open(fstab_path, "w") as fstab:
                 fstab.writelines(fstab_lines)
 
-            return fstab_path
+            return new_rootfs
 
     def _update_fstab(self, fstab_lines, parts):
         """Assume partition order same as in wks"""

@@ -156,7 +162,10 @@ class DirectPlugin(ImagerPlugin):
         filesystems from the artifacts directly and combine them into

scripts/lib/wic/plugins/imager/direct.py +1 -7

@@ -172,12 +181,7 @@ class DirectPlugin(ImagerPlugin):
                     if rsize_bb:
                         part.size = int(round(float(rsize_bb)))
 
-        try:
-            self._image.prepare(self)
-        finally:
-            if fstab_path:
-                shutil.move(fstab_path + ".orig", fstab_path)
-
+        self._image.prepare(self)
         self._image.layout_partitions()
         self._image.create()
 
--

2.1.4

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Bad news: the failure this was meant to fix happened again.

| tar: ./opt/ltp/testcases/bin/dup01: file changed as we read it
| ERROR: Function failed: do_image_tar (log file is located at /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-arm/build/build/tmp/work/beaglebone-poky-linux-gnueabi/core-image-sato-sdk-ptest/1.0-r0/temp/log.do_image_tar.5535)

As before, image_wic was also running.

Is there any chance this could still be wic, or should we look at the jffs2 image generator?

Ross

Quoted 12 lines · Ed Bartosh

wic updates /etc/fstab on root partition if there are valid mount points in .wks

When wic runs from bitbake this can cause incorrect results or even breakage of other tasks working with rootfs directory in parallel with do_image_wic.

Implemented copying rootfs directory to a temporary location using copyhardlinktree before updating fstab to avoid conflicts with other tasks working with rootfs.

Review

Quoted 6 lines

Signed-off-by Ed Bartosh <[email protected]>

---
 scripts/lib/wic/plugins/imager/direct.py | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/scripts/lib/wic/plugins/imager/direct.py

Quoted 13 lines

index aa9cc9f..f707365 100644

scripts/lib/wic/plugins/imager/direct.py +2 -0

@@ -32,6 +32,8 @@ import uuid
 
 from time import strftime
 
+from oe.path import copyhardlinktree
+
 from wic import WicError
 from wic.filemap import sparse_copy
 from wic.ksparser import KickStart, KickStartError

scripts/lib/wic/plugins/imager/direct.py class DirectPlugin(ImagerPlugin): · line 122

Quoted 7 lines

scripts/lib/wic/plugins/imager/direct.py +3 -1

@@ -115,12 +117,16 @@ class DirectPlugin(ImagerPlugin):
             fstab_lines = fstab.readlines()
 
         if self._update_fstab(fstab_lines, self.parts):
-            shutil.copyfile(fstab_path, fstab_path + ".orig")
+            # copy rootfs dir to workdir to update fstab
+            # as rootfs can be used by other tasks and can't be modified
+            new_rootfs = os.path.realpath(os.path.join(self.workdir,

"rootfs_copy"))

Quoted 13 lines

scripts/lib/wic/plugins/imager/direct.py +3 -1

+            copyhardlinktree(image_rootfs, new_rootfs)
+            fstab_path = os.path.join(new_rootfs, 'etc/fstab')
 
             with open(fstab_path, "w") as fstab:
                 fstab.writelines(fstab_lines)
 
-            return fstab_path
+            return new_rootfs
 
     def _update_fstab(self, fstab_lines, parts):
         """Assume partition order same as in wks"""

@@ -156,7 +162,10 @@ class DirectPlugin(ImagerPlugin):
         filesystems from the artifacts directly and combine them into

scripts/lib/wic/plugins/imager/direct.py class DirectPlugin(ImagerPlugin): · line 171

Quoted 10 lines

scripts/lib/wic/plugins/imager/direct.py +4 -1

         a partitioned image.
         """
-        fstab_path = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
+        new_rootfs = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
+        if new_rootfs:
+            # rootfs was copied to update fstab
+            self.rootfs_dir['ROOTFS_DIR'] = new_rootfs
 
         for part in self.parts:
             # get rootfs size from bitbake variable if it's not set in

.ks file

Quoted 22 lines

scripts/lib/wic/plugins/imager/direct.py +1 -7

@@ -172,12 +181,7 @@ class DirectPlugin(ImagerPlugin):
                     if rsize_bb:
                         part.size = int(round(float(rsize_bb)))
 
-        try:
-            self._image.prepare(self)
-        finally:
-            if fstab_path:
-                shutil.move(fstab_path + ".orig", fstab_path)
-
+        self._image.prepare(self)
         self._image.layout_partitions()
         self._image.create()
 
--

2.1.4

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

There is always a chance. Which commit is this? I'll try to reproduce this.

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

Quoted 5 lines · Ed Bartosh

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

As you is working on this, please also include Fabio's patch on the patchset. It includes a command like option to disable fstab change at all. For delta-based updates this is imperative.

Fabio, could you point him the last patch revision?

The last patch I sent is here:
https://patchwork.openembedded.org/patch/139252/

We're using this patch internally with Pyro branch. I can rework to apply on master.

Quoted 13 lines · Otavio Salvador

Quoted 5 lines · Ed Bartosh

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

As you is working on this, please also include Fabio's patch on the patchset. It includes a command like option to disable fstab change at all. For delta-based updates this is imperative.

Fabio, could you point him the last patch revision?

Quoted 18 lines · Fabio Berton

The last patch I sent is here:
https://patchwork.openembedded.org/patch/139252/

We're using this patch internally with Pyro branch. I can rework to apply on master.

Quoted 11 lines · Otavio Salvador

Quoted 5 lines · Ed Bartosh

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

As you is working on this, please also include Fabio's patch on the patchset. It includes a command like option to disable fstab change at all. For delta-based updates this is imperative.

Quoted 2 lines

Quoted 2 lines

Fabio, could you point him the last patch revision?

Do we really need that?

JFYI: Mount point in .wks is an optional field. It makes sense to use it only if partition needs to be mounted on boot. fstab will not be updated unless it's explicitly requested by specifying mount points in .wks

Quoted 24 lines · Ed Bartosh

Quoted 20 lines · Fabio Berton

The last patch I sent is here:
https://patchwork.openembedded.org/patch/139252/

We're using this patch internally with Pyro branch. I can rework to apply on master.

Quoted 13 lines · Otavio Salvador

Quoted 5 lines · Ed Bartosh

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

As you is working on this, please also include Fabio's patch on the patchset. It includes a command like option to disable fstab change at all. For delta-based updates this is imperative.

Fabio, could you point him the last patch revision?

Do we really need that?

Quoted 3 lines

JFYI: Mount point in .wks is an optional field. It makes sense to use it only if partition needs to be mounted on boot. fstab will not be updated unless it's explicitly requested by specifying mount points in .wks

It should have support to not touch it. For images which we intend to do delta updates, the content cannot be changed besides the original rootfs generation. So yes, we need that.

Quoted 25 lines · Otavio Salvador

Quoted 24 lines · Ed Bartosh

Quoted 20 lines · Fabio Berton

The last patch I sent is here:
https://patchwork.openembedded.org/patch/139252/

We're using this patch internally with Pyro branch. I can rework to apply on master.

Quoted 13 lines · Otavio Salvador

Quoted 5 lines · Ed Bartosh

The patchset also fixes long standing bug: wic updated fstab inplace in rootfs directory. This causes other tasks working with rootfs directory to produce incorrect results or crash. This is fixed by copying rootfs to the temporary directory before updating fstab.

As you is working on this, please also include Fabio's patch on the patchset. It includes a command like option to disable fstab change at all. For delta-based updates this is imperative.

Fabio, could you point him the last patch revision?

Do we really need that?

Quoted 7 lines

Quoted 3 lines

JFYI: Mount point in .wks is an optional field. It makes sense to use it only if partition needs to be mounted on boot. fstab will not be updated unless it's explicitly requested by specifying mount points in .wks

It should have support to not touch it. For images which we intend to do delta updates, the content cannot be changed besides the original rootfs generation. So yes, we need that.

I'm not sure I understand this. If you don't want fstab to be changed you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic to change /etc/fstab, which you apparently don't want. So, don't specify mount points and you'll have what you want.

Having additional option for this looks redundand to me.

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Quoted 11 lines · Ed Bartosh

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

Quoted 7 lines

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Why does --exclude-path conflict with --no-rootfs-update? Is that a conceptual problem or an implementation problem?

If I'm not mistaken, --exclude-path merely means "take this rootfs, but exclude certain parts". That's in line with --no-rootfs-update == "do not modify the content of the rootfs", as it just helps with choosing where content goes (the "single rootfs" -> "different partitions" approach).

Quoted 20 lines · Patrick Ohly

Quoted 18 lines · Ed Bartosh

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Quoted 3 lines

Why does --exclude-path conflict with --no-rootfs-update? Is that a conceptual problem or an implementation problem?

I thought that removing directories from original rootfs is a modification.

Quoted 25 lines · Ed Bartosh

Quoted 23 lines · Patrick Ohly

Quoted 11 lines · Ed Bartosh

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

Quoted 7 lines

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Why does --exclude-path conflict with --no-rootfs-update? Is that a conceptual problem or an implementation problem?

Quoted 2 lines

I thought that removing directories from original rootfs is a modification.

But it's not actually removed from the original roofs directory, right? For me, "not modified" refers to that and the files in it.

Quoted 29 lines · Patrick Ohly

Quoted 27 lines · Ed Bartosh

Quoted 20 lines · Patrick Ohly

Quoted 11 lines · Ed Bartosh

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

Quoted 7 lines

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Quoted 3 lines

Why does --exclude-path conflict with --no-rootfs-update? Is that a conceptual problem or an implementation problem?

I thought that removing directories from original rootfs is a modification.

Quoted 2 lines

But it's not actually removed from the original roofs directory, right? For me, "not modified" refers to that and the files in it.

My problem is with the fstab change. If I explicitly ask wic to drop something I know it is doing it so it is under my control.

Adding --no-fstab-change solves in an elegant way my problem.

Quoted 25 lines · Ed Bartosh

Quoted 23 lines · Patrick Ohly

Quoted 11 lines · Ed Bartosh

Quoted 1 line

I'm not sure I understand this. If you don't want fstab to be

changed

Quoted 2 lines

you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic

to

Quoted 1 line

change /etc/fstab, which you apparently don't want. So, don't

specify

Quoted 3 lines

mount points and you'll have what you want.

Having additional option for this looks redundand to me.

Quoted 7 lines

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks

Why does --exclude-path conflict with --no-rootfs-update? Is that a conceptual problem or an implementation problem?

Quoted 2 lines

I thought that removing directories from original rootfs is a modification.

But it's not actually removed from the original roofs directory, right? For me, "not modified" refers to that and the files in it.

We're talking about different things here, I guess. For me if target rootfs content in the image is different from rootfs produced by bitbake it means it's modified.

BTW, if this patchset is accepted wic will not be changing fstab in original rootfs anymore:
http://lists.openembedded.org/pipermail/openembedded-core/2017-June/138803.html
However, it would not mean that rootfs is not changed.

Other possible rootfs changes also should be possible to be disabled but IMO it should be per-feature (one for fstab, one for exclude, ...).

Quoted 3 lines · Otavio Salvador

Other possible rootfs changes also should be possible to be disabled but IMO it should be per-feature (one for fstab, one for exclude, ...).

I also think it should be per-feature, it necessary at all.

I still do not fully understand under which circumstances wic modifies the rootfs. If that happens only when explicitly requested in the wks file as Ed said, then I don't see a need for any additional flags.

then I don't see a need for any additional flags. Just don't use the features which result in a rootfs modification.

Quoted 2 lines · Patrick Ohly

then I don't see a need for any additional flags. Just don't use the features which result in a rootfs modification.

I also didn't see it till last message from Otavio. Now I do - they don't want to change .wks files. They're using standard wks from scripts/lib/wic/canned-wks or from standard layers and they don't want to duplicate them when they don't want rootfs modifications.

It could be a valid reason to have --no-fstab-update option I think. However, I'm still not 100% convinced I'm ok with this if nobody else objects.

Quoted 4 lines · Ed Bartosh

Quoted 2 lines · Patrick Ohly

then I don't see a need for any additional flags. Just don't use the features which result in a rootfs modification.

Quoted 8 lines

I also didn't see it till last message from Otavio. Now I do - they don't want to change .wks files. They're using standard wks from scripts/lib/wic/canned-wks or from standard layers and they don't want to duplicate them when they don't want rootfs modifications.

It could be a valid reason to have --no-fstab-update option I think. However, I'm still not 100% convinced I'm ok with this if nobody else objects.

Okay, now I see what the purpose is.

I prefer a --no-fstab-update over a general --no-rootfs-update because for each case where wic would normally modify the rootfs, some other mechanism must be in place which makes that modification redundant (like using PARTUUID). Having separate parameters forces the developers to think about it. Just my 2 cents...