diff mbox series

[1/2] wic/plugins/images/direct: replace fstab entries

Message ID 20230105125223.398216-2-felix.moessbauer@siemens.com
State New
Headers show
Series wic/plugins/images/direct: replace fstab entries | expand

Commit Message

Felix Moessbauer Jan. 5, 2023, 12:52 p.m. UTC 
This patch extends the imager direct class to check if the mountpoint of
a to-be-added entry in the fstab is already there. If it is, the old
entry is removed and the new entry is appended.

This solves issues with duplicated entries that come from the rootfs
itself. One example where this happens is when generating images for
both direct kernel boot and EFI. In this case, the rootfs mountpoint
needs to be in the fstab of the rootfs and needs to be replaced in WIC.

With this logic, the reverted commit 20d43a2 can be included again.

Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 scripts/lib/wic/plugins/imager/direct.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Henning Schild Jan. 5, 2023, 1:24 p.m. UTC | #1 
Am Thu,  5 Jan 2023 12:52:22 +0000
schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:

> This patch extends the imager direct class to check if the mountpoint
> of a to-be-added entry in the fstab is already there. If it is, the
> old entry is removed and the new entry is appended.
> 
> This solves issues with duplicated entries that come from the rootfs
> itself. One example where this happens is when generating images for
> both direct kernel boot and EFI. In this case, the rootfs mountpoint
> needs to be in the fstab of the rootfs and needs to be replaced in
> WIC.
> 
> With this logic, the reverted commit 20d43a2 can be included again.
> 
> Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>  scripts/lib/wic/plugins/imager/direct.py | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/lib/wic/plugins/imager/direct.py
> b/scripts/lib/wic/plugins/imager/direct.py index
> dfaa901567..085bf8b463 100644 ---
> a/scripts/lib/wic/plugins/imager/direct.py +++
> b/scripts/lib/wic/plugins/imager/direct.py @@ -104,6 +104,14 @@ class
> DirectPlugin(ImagerPlugin): 
>      def update_fstab(self, image_rootfs):
>          """Assume partition order same as in wks"""
> +
> +        def _get_mountpoint(line):
> +            """return the mount point of an fstab entry or None"""
> +            parts = line.split()
> +            if line.startswith('#') or len(parts) != 6:
> +                return None
> +            return parts[1]
> +
>          if not image_rootfs:
>              return
>  
> @@ -142,6 +150,8 @@ class DirectPlugin(ImagerPlugin):
>              line = "\t".join([device_name, part.mountpoint,
> part.fstype, opts, "0", passno]) + "\n"
>  
> +            # if this mountpoint is already in the fstab, replace it
> +            fstab_lines = [item for item in fstab_lines if
> _get_mountpoint(item) != part.mountpoint] fstab_lines.append(line)

please bbwarn here, while this seems ok it indicates that two places
write different things and users might want to drop the offending line
from their template

Henning

>              updated = True
>
Florian Bezdeka Jan. 6, 2023, 8:56 a.m. UTC | #2 
On 05.01.23 14:24, Henning Schild wrote:
> Am Thu,  5 Jan 2023 12:52:22 +0000
> schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:
> 
>> This patch extends the imager direct class to check if the mountpoint
>> of a to-be-added entry in the fstab is already there. If it is, the
>> old entry is removed and the new entry is appended.
>>
>> This solves issues with duplicated entries that come from the rootfs
>> itself. One example where this happens is when generating images for
>> both direct kernel boot and EFI. In this case, the rootfs mountpoint
>> needs to be in the fstab of the rootfs and needs to be replaced in
>> WIC.
>>
>> With this logic, the reverted commit 20d43a2 can be included again.
>>
>> Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
>> ---
>>  scripts/lib/wic/plugins/imager/direct.py | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/scripts/lib/wic/plugins/imager/direct.py
>> b/scripts/lib/wic/plugins/imager/direct.py index
>> dfaa901567..085bf8b463 100644 ---
>> a/scripts/lib/wic/plugins/imager/direct.py +++
>> b/scripts/lib/wic/plugins/imager/direct.py @@ -104,6 +104,14 @@ class
>> DirectPlugin(ImagerPlugin): 
>>      def update_fstab(self, image_rootfs):
>>          """Assume partition order same as in wks"""
>> +
>> +        def _get_mountpoint(line):
>> +            """return the mount point of an fstab entry or None"""
>> +            parts = line.split()
>> +            if line.startswith('#') or len(parts) != 6:
>> +                return None
>> +            return parts[1]
>> +
>>          if not image_rootfs:
>>              return
>>  
>> @@ -142,6 +150,8 @@ class DirectPlugin(ImagerPlugin):
>>              line = "\t".join([device_name, part.mountpoint,
>> part.fstype, opts, "0", passno]) + "\n"
>>  
>> +            # if this mountpoint is already in the fstab, replace it
>> +            fstab_lines = [item for item in fstab_lines if
>> _get_mountpoint(item) != part.mountpoint] fstab_lines.append(line)
> 
> please bbwarn here, while this seems ok it indicates that two places
> write different things and users might want to drop the offending line
> from their template

That is the expected behavior. wic has to update the entry for / and
that is what it does. I can't find a reason why we should warn here.

> 
> Henning
> 
>>              updated = True
>>  
>
Henning Schild Jan. 6, 2023, 4:14 p.m. UTC | #3 
Am Fri, 6 Jan 2023 09:56:01 +0100
schrieb Florian Bezdeka <florian.bezdeka@siemens.com>:

> On 05.01.23 14:24, Henning Schild wrote:
> > Am Thu,  5 Jan 2023 12:52:22 +0000
> > schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:
> >   
> >> This patch extends the imager direct class to check if the
> >> mountpoint of a to-be-added entry in the fstab is already there.
> >> If it is, the old entry is removed and the new entry is appended.
> >>
> >> This solves issues with duplicated entries that come from the
> >> rootfs itself. One example where this happens is when generating
> >> images for both direct kernel boot and EFI. In this case, the
> >> rootfs mountpoint needs to be in the fstab of the rootfs and needs
> >> to be replaced in WIC.
> >>
> >> With this logic, the reverted commit 20d43a2 can be included again.
> >>
> >> Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> >> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> >> ---
> >>  scripts/lib/wic/plugins/imager/direct.py | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/scripts/lib/wic/plugins/imager/direct.py
> >> b/scripts/lib/wic/plugins/imager/direct.py index
> >> dfaa901567..085bf8b463 100644 ---
> >> a/scripts/lib/wic/plugins/imager/direct.py +++
> >> b/scripts/lib/wic/plugins/imager/direct.py @@ -104,6 +104,14 @@
> >> class DirectPlugin(ImagerPlugin): 
> >>      def update_fstab(self, image_rootfs):
> >>          """Assume partition order same as in wks"""
> >> +
> >> +        def _get_mountpoint(line):
> >> +            """return the mount point of an fstab entry or None"""
> >> +            parts = line.split()
> >> +            if line.startswith('#') or len(parts) != 6:
> >> +                return None
> >> +            return parts[1]
> >> +
> >>          if not image_rootfs:
> >>              return
> >>  
> >> @@ -142,6 +150,8 @@ class DirectPlugin(ImagerPlugin):
> >>              line = "\t".join([device_name, part.mountpoint,
> >> part.fstype, opts, "0", passno]) + "\n"
> >>  
> >> +            # if this mountpoint is already in the fstab, replace
> >> it
> >> +            fstab_lines = [item for item in fstab_lines if
> >> _get_mountpoint(item) != part.mountpoint] fstab_lines.append(line)
> >>  
> > 
> > please bbwarn here, while this seems ok it indicates that two places
> > write different things and users might want to drop the offending
> > line from their template  
> 
> That is the expected behavior. wic has to update the entry for / and
> that is what it does. I can't find a reason why we should warn here.

maybe because we turned "auto" to "ext4" and "1 1" to "0 0" ... see
cover letter. And all pretty much without leaving a trace how that
could happen.

We see that wic messing with fstab / is useful but can have fun
effects, that is why that initial patch was reverted.

I say we only bring it back in a way that uses can not again see fun
effects, maybe by warning them and or requiring them to be explicit by
introducing a new arg for part.

Otherwise we might just be adding the next patch to revert later, and
we have to poll again because we are not on CC for that revert.

Henning

> > 
> > Henning
> >   
> >>              updated = True
> >>    
> >   
>
diff mbox series

Patch

diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index dfaa901567..085bf8b463 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -104,6 +104,14 @@  class DirectPlugin(ImagerPlugin):
 
     def update_fstab(self, image_rootfs):
         """Assume partition order same as in wks"""
+
+        def _get_mountpoint(line):
+            """return the mount point of an fstab entry or None"""
+            parts = line.split()
+            if line.startswith('#') or len(parts) != 6:
+                return None
+            return parts[1]
+
         if not image_rootfs:
             return
 
@@ -142,6 +150,8 @@  class DirectPlugin(ImagerPlugin):
             line = "\t".join([device_name, part.mountpoint, part.fstype,
                               opts, "0", passno]) + "\n"
 
+            # if this mountpoint is already in the fstab, replace it
+            fstab_lines = [item for item in fstab_lines if _get_mountpoint(item) != part.mountpoint]
             fstab_lines.append(line)
             updated = True