Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix initrd regeneration on s390x and Fedora/RHEL #2873

Merged
merged 1 commit into from Oct 12, 2022

Conversation

lzaoral
Copy link
Contributor

@lzaoral lzaoral commented Sep 29, 2022

Relax-and-Recover (ReaR) Pull Request Template

Please fill in the following items before submitting a new pull request:

Pull Request Details:
  • Type: Bug Fix

  • Impact: Normal

  • How was this pull request tested? RHEL 9 on s390x z/VM

  • Brief description of the changes in this pull request:

For some reason, the 550_rebuild_initramfs.sh script was not included for s390x on Fedora/RHEL so the initrd was not regenerated after backup restore on this architecture.

Since all other architectures were actually using the same script, let's just move it one level up to fix this bug and to also simplify the directory structure a bit.

For some reason, the 550_rebuild_initramfs.sh script was not included
for s390x on Fedora/RHEL so the initrd was not regenerated after backup
restore on this architecture.

Since all other architectures were actually using the same script,
let's just move it one level up to fix this bug and to also simplify
the directory structure a bit.
@pcahyna
Copy link
Member

pcahyna commented Sep 29, 2022

The symlinks ( ppc, ppc64le ) to the script have been introduced in #1311 and s390 was then forgotten, because support for that arch got introduced later. I have read #1311 (comment) and I don't think that the issue described there prevents moving the file up and removing the symlinks, and it is IMO the right thing to do, because there is nothing architecture-dependent in initrd regeneration. It will prevent this from being forgotten again when a new architecture support is added (think ARM).

@pcahyna pcahyna added this to the ReaR v2.8 milestone Sep 29, 2022
@pcahyna pcahyna requested a review from a team September 29, 2022 14:20
Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree with the change - good finding by the way.

@jsmeix
Copy link
Member

jsmeix commented Sep 30, 2022

Regarding my
#1311 (comment)
What I had meant was that we cannot have specific scripts
that replace "default" scripts because all scripts will be run.

I think the crucial point is whether or not
there is nothing architecture-dependent in initrd regeneration.

The problem is when there are already architecture-dependent
sub-directories it indicates that there is something
architecture-dependent in initrd regeneration
or that likely ther can be something
architecture-dependent in initrd regeneration
so a contributor for some specific architecture
will follow what is already there.
As far as I understand
https://github.com/rear/rear/pull/1311/files
it means that a architecture-dependent sub-directory
usr/share/rear/finalize/Fedora/i386/
had been already there.

@jsmeix
Copy link
Member

jsmeix commented Sep 30, 2022

By the way:
We may clean up all of those symlinks
for the three different cases:

# find usr/share/rear -ls | grep 550_rebuild_initramfs

... -rw-r--r-- ... 5397 ...usr/share/rear/finalize/Fedora/i386/550_rebuild_initramfs.sh
... lrwxrwxrwx ...   32 ... usr/share/rear/finalize/Fedora/ppc64le/550_rebuild_initramfs.sh -> ../i386/550_rebuild_initramfs.sh
... lrwxrwxrwx ...   32 ... usr/share/rear/finalize/Fedora/ppc64/550_rebuild_initramfs.sh -> ../i386/550_rebuild_initramfs.sh

... -rw-r--r-- ... 5660 ... usr/share/rear/finalize/SUSE_LINUX/i386/550_rebuild_initramfs.sh
... lrwxrwxrwx ...   32 ... usr/share/rear/finalize/SUSE_LINUX/ppc64le/550_rebuild_initramfs.sh -> ../i386/550_rebuild_initramfs.sh
... lrwxrwxrwx ...   32 ... usr/share/rear/finalize/SUSE_LINUX/ppc64/550_rebuild_initramfs.sh -> ../i386/550_rebuild_initramfs.sh

... -rw-r--r-- ... 4454 ... usr/share/rear/finalize/Debian/i386/550_rebuild_initramfs.sh
... lrwxrwxrwx ...   32 ... usr/share/rear/finalize/Debian/ppc64le/550_rebuild_initramfs.sh -> ../i386/550_rebuild_initramfs.sh

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From plain looking at the changes it looks good to me

@pcahyna
Copy link
Member

pcahyna commented Sep 30, 2022

Yes, it had been already there, so I don't blame the authors of #1311 for following the example. It has been since the beginning when it was introduced (as usr/share/rear/finalize/SUSE_LINUX/i386/17_rebuild_initramfs.sh) and I believe that even then it was not necessary, because it did not contain anything architecture-specific.

@jsmeix
Copy link
Member

jsmeix commented Sep 30, 2022

Further unifying the actual scripts

finalize/Fedora/i386/550_rebuild_initramfs.sh
finalize/SUSE_LINUX/i386/550_rebuild_initramfs.sh
finalize/Debian/i386/550_rebuild_initramfs.sh

into one or two (as far as possible with reasonable effort)
should be a separated cleanup task (as time permits).

@jsmeix
Copy link
Member

jsmeix commented Sep 30, 2022

In general I am against the Linux distribution specific
sub-directories which we currently have in ReaR
because usually same kind of things are
rather same on different Linux distributions
so in general I prefer one generic script
that has Linux distribution specific differences
only where needed.

In contrast when different kind of things are used
(e.g. legacy GRUB versus GRUB2 or BIOS versus UEFI)
then usually separated different scripts should be used.

@jsmeix
Copy link
Member

jsmeix commented Oct 5, 2022

@pcahyna
I assigned this pull request to you
because in its current form it belongs only
to things in .../finalize/Fedora/...
so you could merge it as you like.

@pcahyna
Copy link
Member

pcahyna commented Oct 12, 2022

Hi @jsmeix , thanks for looking. We can check the differences between Fedora and SUSE scripts and determine if they are needed or not (some tests on both sides will be required).

@pcahyna pcahyna merged commit 56800d6 into rear:master Oct 12, 2022
12 checks passed
@lzaoral lzaoral deleted the fedora-s390x-initrd branch October 12, 2022 15:03
pcahyna added a commit to pcahyna/rear that referenced this pull request Feb 17, 2023
Fix initrd regeneration on s390x and Fedora/RHEL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants