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

[arm] Refactor installer and build to allow arm builds targeted at grub platforms #11341

Merged
merged 5 commits into from
Jul 12, 2022

Conversation

alexrallen
Copy link
Contributor

Refactors the SONiC Installer to support greater flexibility in building for a given architecture and bootloader.

Why I did it

Currently the SONiC installer assumes that if a platform is ARM based that it uses the uboot bootloader and uses the grub bootloader otherwise. This is not a correct assumption to make as ARM is not strictly tied to uboot and x86 is not strictly tied to grub.

How I did it

To implement this I introduce the following changes:

  • Remove the different arch folders from the installer/ directory
  • Merge the generic components of the ARM and x86 installer into installer/installer.sh
  • Refactor x86 + grub specific functions into installer/default_platform.conf
  • Modify installer to call default_platform.conf file and also call platform/[platform]/patform.conf file as well to override as needed
  • Update references to the installer in the build_image.sh script
  • Add TARGET_BOOTLOADER variable that is by default uboot for ARM devices and grub for x86 unless overridden in platform/[platform]/rules.mk
  • Update bootloader logic in build_debian.sh to be based on TARGET_BOOTLOADER instead of TARGET_ARCH and to reference the grub package in a generic manner

How to verify it

This has been tested on a ARM test platform as well as on Mellanox amd64 switches as well to ensure there was no impact.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205

Description for the changelog

[arm] Refactor installer and build to allow arm builds targeted at grub platforms

Link to config_db schema for YANG module changes

N/A

A picture of a cute animal (not mandatory but encouraged)

possum

@liat-grozovik
Copy link
Collaborator

@stepanblyschak could you please help to review sonic-installer part?
@qiluo-msft could you please help to review or suggest someone to review?

installer/install.sh Outdated Show resolved Hide resolved
@qiluo-msft qiluo-msft requested a review from wen587 July 6, 2022 18:53
wen587
wen587 previously requested changes Jul 7, 2022
installer/default_platform.conf Show resolved Hide resolved
build_image.sh Show resolved Hide resolved
if [ -r ./onie-image-arm64.conf ]; then
. ./onie-image-arm64.conf
if [ -r ./onie-image*.conf ]; then
. ./onie-image*.conf
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 8, 2022

Choose a reason for hiding this comment

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

If there are multiple files matching the wildcard, only one is executed.

$ . a.sh
a
$ . b.sh
b
$ . *.sh
a
``` #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems there is a case of onie-image.conf and onie-image-*.conf both exist for one platform.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still applicable.

Copy link
Contributor Author

@alexrallen alexrallen Jul 12, 2022

Choose a reason for hiding this comment

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

Still applicable.

Not sure the reasoning of this comment @qiluo-msft no changes went in between your two comments, maybe I am misunderstanding.

Anyway, I think this should be resolved now. I have made a couple changes elsewhere (that should have been there in the first place) that make it such that only the needed onie-image files are loaded into an image given the target architecture. Then I load in the generic and the arch-specific version in the installer. This should resolve any concerns.

@wen587 wen587 dismissed their stale review July 8, 2022 01:08

LGTM

@qiluo-msft qiluo-msft merged commit 429254c into sonic-net:master Jul 12, 2022
@xumia
Copy link
Collaborator

xumia commented Jul 27, 2022

@alexrallen looks like one of the template variable (%%EXTRA_CMDLINE_LINUX%%) is not replaced. Is it relative to the change? The grub.cfg in the latest vs image may be not correct.

log:

EXTRA_CMDLINE_LINUX=
ONIE: Unable to find 'Part Number' TLV in EEPROM data.
Success: Support tarball created: /tmp/onie-support-kvm_x86_64.tar.bz2
Installing for i386-pc platform.
Installation finished. No error reported.
Switch CPU vendor is: GenuineIntel
Switch CPU cstates are: disabled
EXTRA_CMDLINE_LINUX=%%EXTRA_CMDLINE_LINUX%%
Installed SONiC base image SONiC-OS successfully
ONIE: NOS install successful: file://dev/vdb/onie-installer.bin

And Grub settings: see %%EXTRA_CMDLINE_LINUX%% in linux.

menuentry 'SONiC-OS-master-10946.127441-ab961b5e3' {
        search --no-floppy --label --set=root SONiC-OS
        echo    'Loading SONiC-OS OS kernel ...'
        insmod gzio
        if [ x = xxen ]; then insmod xzio; insmod lzopio; fi
        insmod part_msdos
        insmod ext2
        linux   /image-master-10946.127441-ab961b5e3/boot/vmlinuz-5.10.0-12-2-amd64 root=UUID=0f1b4b07-9742-426d-bd3a-a1e789401536 rw console=tty0 console=ttyS0,115200n8 quiet intel_idle.max_cstate=0 %%EXTRA_CMDLINE_LINUX%%                  net.ifnames=0 biosdevname=0                 loop=image-master-10946.127441-ab961b5e3/fs.squashfs loopfstype=squashfs                                       systemd.unified_cgroup_hierarchy=0                 apparmor=1 security=apparmor varlog_size=4096 usbcore.autosuspend=-1 
        echo    'Loading SONiC-OS OS initial ramdisk ...'
        initrd  /image-master-10946.127441-ab961b5e3/boot/initrd.img-5.10.0-12-2-amd64
}

@@ -1,119 +1,5 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not ideal that we have .conf file with so complex bash code. And there are many variables need replacement, like %%SONIC_ROOT%%.

ZhaohuiS pushed a commit that referenced this pull request Oct 26, 2022
Why I did it
Issue was caused by this #11341

*.bin image structure in 202205:

vkarri@19d5638dde2d:/sonic$ ls -l /tmp/tmp.9ibWSipeRw/installer/x86_64/
total 12
drwxr-xr-x 2 vkarri dip 12288 Oct 14 13:16 platforms
However install.sh which runs on ONiE parition expects the platform specific kernel cmd line conf file under platform/$onie_platform_string file https://github.com/sonic-net/sonic-buildimage/blob/master/installer/install.sh#L102

Thus, any platform which defines and depends on these params might be broken on master label.

How I did it
Since we are already filtering the conf files based on TARGET_PLATFORM in build_image.sh, i've just updated the location to installer/platforms instead of installer/$arch/platforms

Signed-off-by: Vivek Reddy Karri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants