Address review comments for pull request #846 by padmanarayana · Pull Request #901 · sonic-net/sonic-buildimage

padmanarayana · 2017-08-21T12:15:17Z

"make target/sonic-broadcom.raw" will create the compressed dd'able image.
This will also update the grub config files (device/dell/*/nos_to_sonic_grub.cfg) with the image versions.

msftclas · 2017-08-21T12:15:22Z

@padmanarayana,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

lguohan · 2017-08-22T09:28:52Z

Hi padman, can you add an iteration on previous #846 so that I can know what changed and what not changed. In case you start a new PR, I need to do a new review again for all the changes.

lguohan · 2017-08-22T09:31:35Z

platform/broadcom/raw-image.mk

+$(SONIC_RAW_IMAGE)_MACHINE = broadcom
+$(SONIC_RAW_IMAGE)_IMAGE_TYPE = raw
+$(SONIC_RAW_IMAGE)_DEPENDS += $(BRCM_OPENNSL_KERNEL)
+$(SONIC_RAW_IMAGE)_INSTALLS += $(DELL_S6000_PLATFORM_MODULE) \


this becomes hard to manage we need to add platform to both one-image.mk and raw-image.

it is better to make raw_image depends on one_image, so that you do not need to duplicate these dependencies.

lguohan · 2017-08-22T09:47:33Z

installer/x86_64/install.sh

    done
+else
+    demo_mnt="build_raw_image_mnt"
+    demo_dev=$cur_wd/"%%OUTPUT_RAW_IMAGE%%"


in line 44, you have sourced onie-image.conf, which defines OUTPUT_RAW_IMAGE, here you should use that instead of doing sed

isn't cur_wd = target?

Though onie-image.conf is sourced, the TARGET_MACHINE will be assigned to the default ": ${TARGET_MACHINE:=generic}" resulting in an failed attempt to create sonic-generic.raw rather than sonic-broadcom.raw. The sed expansion is needed to assign the TARGET_MACHINE correctly.

lguohan · 2017-08-22T09:48:12Z

onie-mk-demo.sh

       -e "s/%%IMAGE_VERSION%%/$image_version/g" \
       -e "s/%%ONIE_IMAGE_PART_SIZE%%/$onie_image_part_size/" \
       -e "s/%%EXTRA_CMDLINE_LINUX%%/$EXTRA_CMDLINE_LINUX/" \
+       -e "s@%%OUTPUT_RAW_IMAGE%%@$output_raw_image@" \


I do not see a must you need to do sed here.

lguohan · 2017-08-22T09:49:43Z

build_image.sh

+
+            if [ "$IMAGE_TYPE" = "raw" ] && [ -f ./device/$VENDOR/$PLATFORM/nos_to_sonic_grub.cfg ]; then
+                sed -i -e "s/%%IMAGE_VERSION%%/$IMAGE_VERSION/g" ./device/$VENDOR/$PLATFORM/nos_to_sonic_grub.cfg
+                echo "IMAGE_VERSION is $IMAGE_VERSION/g"


/g? remove?

lguohan · 2017-08-22T09:52:48Z

installer/sharch_body.sh


 # Untar and launch install script in a tmpfs
 cur_wd=$(pwd)
+export cur_wd


not sure you need export this value.

cur_wd is the absolute path prefix prior to the user's target. say, /home/padman/test/sonic-buildimage. Since the installer (in a "build" mode) does not have a real device to mount, it will fallback to /tmp/tmp... (based on mktmp -d). When the final sonic-broadcom.bin is created under /tmp... build_image.sh cannot correctly extract the sonic-broadcom.bin if there are parallel builds on the same host...

One other way would be to pass the cur_wd equivalent to the installer - but that did not appear to be clean.

Exporting cur_wd will ensure that the image gets built only in the specific target dir where the build was invoked.

A correction : the cur_wd is actually /sonic (from the Makefile's -w /sonic ) and not the absolute build path.. Nevertheless, this path needs to be passed to the installer so that build_image.sh may directly retrieve it in-place after the "build" mode installation.

lguohan · 2017-08-22T09:55:59Z

files/image_config/platform/rc.local

+    if [ ! -e /mnt/onie-boot/onie/grub/grub-machine.cfg ]; then
+        echo "/mnt/onie-boot/onie/grub/grub-machine.cfg not found" >> /etc/migration.log
+    else
+        grep "=" /mnt/onie-boot/onie/grub/grub-machine.cfg > /host/machine.conf


this is fragile.

check here. https://github.com/opencomputeproject/onie/pull/227/files

+if [ -r "$grub_machine_cfg" ] ; then + . "$grub_machine_cfg" + grep = $grub_machine_cfg | sed -e 's/onie_//' -e 's/=.*$//' | while read var ; do + eval val='$'onie_$var + printf "%-20s: %s\n" "ONIE ${var}" "$val" + done +else

lguohan

as comments.

padmanarayana

Address review comments.

lguohan · 2017-08-27T02:27:45Z

retest this please

lguohan · 2017-08-28T02:44:59Z

build_image.sh

            fi
+
+            if [ "$IMAGE_TYPE" = "raw" ] && [ -f ./device/$VENDOR/$PLATFORM/nos_to_sonic_grub.cfg ]; then
+                sed -i -e "s/%%IMAGE_VERSION%%/$IMAGE_VERSION/g" ./device/$VENDOR/$PLATFORM/nos_to_sonic_grub.cfg


the IMAGE_VERSION is not replaced in the nos_to_sonic_grub.cfg, can you check?

In my local build, it is getting reset after make target/sonic-broadcom.raw:

padman@ubuntu-16:/gvob/sdchk$ diff ./sonic-buildimage/device/dell/x86_64-dell_s6100_c2538-r0/nos_to_sonic_grub.cfg.base ./sonic-buildimage/device/dell/x86_64-dell_s6100_c2538-r0/nos_to_sonic_grub.cfg
22,23c22,23
< linux /image-%%IMAGE_VERSION%%/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-%%IMAGE_VERSION%%/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor
< initrd /image-%%IMAGE_VERSION%%/boot/initrd.img-3.16.0-4-amd64

linux /image-master.0-2d3b064/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-master.0-2d3b064/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor initrd /image-master.0-2d3b064/boot/initrd.img-3.16.0-4-amd64

31,32c31,32
< linux /image-%%IMAGE_VERSION%%/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-%%IMAGE_VERSION%%/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor
< initrd /image-%%IMAGE_VERSION%%/boot/initrd.img-3.16.0-4-amd64

linux /image-master.0-2d3b064/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-master.0-2d3b064/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor initrd /image-master.0-2d3b064/boot/initrd.img-3.16.0-4-amd64

padman@ubuntu-16:/gvob/sdchk$

lguohan · 2017-08-28T03:08:41Z

device/dell/x86_64-dell_s6100_c2538-r0/nos_to_sonic_grub.cfg

+        insmod part_msdos
+        insmod ext2
+        set root='(hd0,gpt8)'
+        linux   /image-%%IMAGE_VERSION%%/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-%%IMAGE_VERSION%%/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor


please add quiet option to boot linux

[vlan] Add pytest case to add max vlan. (sonic-net#881) [badge]: add vs build badge (sonic-net#898) Fix PFC watchdog not getting lossless TC (sonic-net#876) [vstest]: skip test_AddMaxVlan as it takes almost two hours to finish (sonic-net#901) [test]: Enforce fake port-channel interfaces carrier up Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>

[vlan] Add pytest case to add max vlan. (#881) [badge]: add vs build badge (#898) Fix PFC watchdog not getting lossless TC (#876) [vstest]: skip test_AddMaxVlan as it takes almost two hours to finish (#901) [test]: Enforce fake port-channel interfaces carrier up Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>

[vlan] Add pytest case to add max vlan. (sonic-net#881) [badge]: add vs build badge (sonic-net#898) Fix PFC watchdog not getting lossless TC (sonic-net#876) [vstest]: skip test_AddMaxVlan as it takes almost two hours to finish (sonic-net#901) [test]: Enforce fake port-channel interfaces carrier up Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>

* msft_github/master: [snmpagent]: Update sonic-snmpagent submodule (sonic-net#1004) [swss]: Update the ipinip.j2 template to specify the IPv4 loopback address (sonic-net#998) Change all port_config.ini column headers from 'port' to 'index' (sonic-net#1001) [docker-platform-monitor]: Remove stale fancontrol.pid file (if exists) before starting fancontrol (sonic-net#1002) [swss]: Update swss/common submodules (sonic-net#988) [snmp]: Update py-swsssdk/snmpagent submodules (sonic-net#996) [Broadcom]: Update SAI version to 3.0.3.2-5 (sonic-net#997) [Mellanox]: Update outdated MSN2410 minigraph (sonic-net#995) Add time stamp suffix to the dirty images version string (sonic-net#958) [Mellanox]: Remove FW upgrade procedure in docker (sonic-net#989) [snmp]: Update sonic-snmpagent submodule (sonic-net#986) [Broadcom]: Update SAI version to 3.0.3.2-4 (sonic-net#983) [Ingrasys]: Add Ingrasys S8900-64XC sfputil function and update submodule (sonic-net#984) [mellanox]: Update Mellanox SAI version (sonic-net#979) [Ingrasys]: Change hwmon kernel modules installation order (sonic-net#980) [Makefile] Allowing interactive session with the slave docker-container (sonic-net#903) [snmp]: Update sonic-snmpagent submodule (sonic-net#978) Disable snmpd module disk_hw, so no syslog messages about unavailable disks (sonic-net#977) [teamd]: Remove deprecated blocking logic before starting teamd (sonic-net#976) [Broadcom]: Update libsai version to 3.0.3.2-2 (sonic-net#973) [device/accton]: Add a new supported device, AS7312-54X (sonic-net#955) [sairedis]: update sairedis submodule (sonic-net#974) lacp runner will send lacp update right after it received SIGINT (sonic-net#969) [config]: Fix management interface configuration (sonic-net#966) [Broadcom]: Update OpenNSL modules (sonic-net#970) [utilities]: Update sonic-utilities (sonic-net#968) [interfaces]: Change MTU value to 9100 (sonic-net#967) Framework to plugin Organization specific scripts during ONIE Image build (sonic-net#951) Always start with Forwarding State flag set for bgpd (sonic-net#963) Update sonic-utilities to be compatible with sonic-net#942 (sonic-net#965) [swss]: Fix the command to get HWSKU with sonic-cfggen (sonic-net#964) [bgp]: Fix the deployment_id with DEVICE_METADATA (sonic-net#962) [Ingrasys] Update Ingrasys submodule for S8900-54XC (sonic-net#954) [build/onie installer] Install grub for SONiC post migration from another NOS (sonic-net#949) [syncd]: Comment out unused docker-ptf-brcm.mk [Broadcom]: Update OpenNSL/SAI version (sonic-net#959) [swss]: Move swss/common/sairedis submodule to 201709 tag [sairedis]: update sairedis submodule head (sonic-net#956) [service template] Starting new docker when HWSKU change is detected (sonic-net#946) [config] Fix an issue that bgp asn data type is not consistent (sonic-net#953) [mellanox]: Update Mellanox SAI version ansd SDK version [Ingrasys] update port_config.ini and sfputil for ingrasys platforms (sonic-net#952) [frr]: RR client support in minigraph for FRR (sonic-net#923) [configdb] Migrate minigraph configurations to DB (sonic-net#942) [devices]: Add led plugin for Arista 7060CX-32S and 7260CX3-64 (sonic-net#945) [sonic-slave]: SLAVE_TAG should be for both Dockerfile and Dockerfile.user (sonic-net#950) [github]: add templates for submitting issues and PR (sonic-net#947) [rsyslog]: Use timegenerated instead of timestamp (sonic-net#944) [dell]: remove nos_to_sonic_grub.cfg (sonic-net#943) [slave.mk]: Apply series of patches to SONIC_PYTHON_STDEB_DEBS targets if they exist (sonic-net#941) [rsyslog]: Use SONiC template in containers (sonic-net#940) [Broadcom]: Remove BRCM_OPENNSL library and upgrade BRCM_SAI to 3.0.3.2 (sonic-net#938) [baseimage]: allocate varlog disk in the initramfs stage (sonic-net#936) [sairedis]: revert deadlock fix in sonic-sairedis submodule (sonic-net#934) Replace CRLF line endings with LF (sonic-net#932) Fix confusing comment (sonic-net#931) Update sfputil support for Ingrasys S9100 (sonic-net#929) [quagga]: Disable ipv4 over ipv6 and enable ipv6 over ipv4 peer group (sonic-net#922) [quagga] enable core dump for bgpd and zebra (sonic-net#927) [devices]: Update Dell s6100/z9100 platform modules (sonic-net#925) Revert "Migrate DEVICE_METADATA to db (sonic-net#919)" (sonic-net#928) Migrate DEVICE_METADATA to db (sonic-net#919) [devices]: Bump sonic-platform-modules-arista submodule (sonic-net#924) [image]: build sonic-broadcom.raw image for sonic conversion from ftos (sonic-net#901) [sonic-slave] Force pyangbind version to 0.5.10 (sonic-net#918) [Arista-7260CX3] Rename hwSKU Arista-7260CX3-64 to Arista-7260CX3-C64, introducing new hwSKU Arista-7260CX3-D108C8 (sonic-net#920) [devices]: modify sfputil plugins for mellanox devices for new platform API (sonic-net#916) [cavm]Update sai revision and packet driver (sonic-net#914) [translate-acl] Specify pyangbind version to not introduce new dependency (sonic-net#915) Update sfputil support for Arista platforms (sonic-net#912) Port speed (sonic-net#879) [Accton]: Add a new supported device AS5712-54X (sonic-net#898) [kernel]: update kernel submodule (sonic-net#910) [device]: Updated dell s6100 submodule to 5ab014 (sonic-net#909) [broadcom]: update broadcom sai package to 2.1.5.1-17 (sonic-net#908) IPv4 prefixes shouldn't be sent by default over IPv6 session with FRR. (sonic-net#905) [submodule]: update sonic linux kernel (sonic-net#906) [sonic-sairedis] update sairedis submodule (sonic-net#211, sonic-net#212) (sonic-net#904) [mlnx-fw-upgrade]: Define required FW version in build time. (sonic-net#902) [SAI]: Remove the SAI submodule from buildimage repo (sonic-net#893) [Submodule update]: sonic-utilities (sonic-net#888) Revert "[mellanox]: Update Mellanox SAI version" [mellanox]: Update Mellanox SAI version [utilities]: Update sonic-utilities submodule [swss-common]: Update sonic-swss-common submodule [mellanox]: Disable fsat boot mode for SX kernel [quagga]: Update sonic-quagga submodule [Broadcom]: Update Broadcom SAI/SDK version (sonic-net#883) Squash merge v1.0.3 branch onto master

…sonic-net#901) Signed-off-by: Guohan Lu <gulv@microsoft.com>

c2facd8 [show] Fix abbreviations for 'show ip bgp ...' commands (#901) cb68e7d Add support for multi-ASIC devices (#877) 44ed6e9 Improved route_check tool and adopt to 20191130 image. (#898) 6fba8db [psushow] Add a column to display LED color to show platform psustatus output (#886) e747456 ssd_mitigation_changes (#829)

Address review comments for pull request #846

487c074

msftclas added the cla-already-signed label Aug 21, 2017

padmanarayana mentioned this pull request Aug 21, 2017

Add a build mode to the installer to facilitate partition dump #846

Closed

lguohan reviewed Aug 22, 2017

View reviewed changes

lguohan requested changes Aug 22, 2017

View reviewed changes

Address review comments

b9cae67

padmanarayana commented Aug 25, 2017

View reviewed changes

stcheng added the Enhancement ➕ label Aug 27, 2017

lguohan reviewed Aug 28, 2017

View reviewed changes

lguohan approved these changes Aug 28, 2017

View reviewed changes

lguohan merged commit 2d3b064 into sonic-net:master Aug 28, 2017

lguohan mentioned this pull request Aug 29, 2017

Changes for building the 8GB d image blob #800

Closed

padmanarayana deleted the sonic_dd_update2 branch August 29, 2017 09:43

stcheng mentioned this pull request May 22, 2019

[swss]: Update sonic-swss submodule #2935

Merged

madhanmellanox pushed a commit to madhanmellanox/sonic-buildimage that referenced this pull request Mar 23, 2020

[vstest]: skip test_AddMaxVlan as it takes almost two hours to finish (…

2e3b946

…sonic-net#901) Signed-off-by: Guohan Lu <gulv@microsoft.com>

judyjoseph mentioned this pull request May 4, 2020

Advance sonic-utilities submodule #4523

Merged

stepanblyschak pushed a commit to stepanblyschak/sonic-buildimage that referenced this pull request May 10, 2021

[show] Fix abbreviations for 'show ip bgp ...' commands (sonic-net#901)

c2facd8

Conversation

padmanarayana commented Aug 21, 2017

Uh oh!

msftclas commented Aug 21, 2017

Uh oh!

lguohan commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

padmanarayana left a comment

Choose a reason for hiding this comment

Uh oh!

lguohan commented Aug 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

31,32c31,32 < linux /image-%%IMAGE_VERSION%%/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-%%IMAGE_VERSION%%/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor < initrd /image-%%IMAGE_VERSION%%/boot/initrd.img-3.16.0-4-amd64

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lguohan commented Aug 22, 2017 •

edited

Loading

lguohan Aug 22, 2017 •

edited

Loading

31,32c31,32
< linux /image-%%IMAGE_VERSION%%/boot/vmlinuz-3.16.0-4-amd64 root=/dev/sda8 rw console=tty0 console=ttyS1,9600n8 loop=image-%%IMAGE_VERSION%%/fs.squashfs loopfstype=squashfs apparmor=1 security=apparmor
< initrd /image-%%IMAGE_VERSION%%/boot/initrd.img-3.16.0-4-amd64