Skip to content

[Marvell] Build infrastructure enhancements #18143

Merged
yxieca merged 10 commits intosonic-net:masterfrom
pavannaregundi:master
Apr 15, 2024
Merged

[Marvell] Build infrastructure enhancements #18143
yxieca merged 10 commits intosonic-net:masterfrom
pavannaregundi:master

Conversation

@pavannaregundi
Copy link
Copy Markdown
Contributor

@pavannaregundi pavannaregundi commented Feb 21, 2024

Why I did it

This change is about marvell build infrastructure enhancement by merging architecture specific marvell platform(marvell-arm64 and marvell-armhf) folders under 'marvell'.

Work item tracking
  • Microsoft ADO (number only):

How I did it

This is archived with following changes divided into following commits,

  • Move mrvl-prestera and sonic-platform-marvell submodule
    'mrvl-prestera' and 'sonic-platform-marvell' submodules are moved from 'marvell-arm64'to 'marvell'. Additionaly submodules are changed to support multi-architecture build.

  • Create sonic-platform-nokia from marvell-arm64/armhf
    'sonic-platform-nokia' is created from merge of 'marvell-armhf' and 'marvell-arm64' in 'marvell'. Additionally changes are done in debian/rules to support multi-architecture build.

  • Change marvell platform makefile to support all arch
    Combine all the architecture targets in makefiles. Added CONFIGURED_ARCH check to build for specific architecture.
    Updated sai deb which matches latest SAI commit.

  • Add support for architecture specific platform.conf
    Changed build_image.sh and onie-mk-demo.sh to read architecture specific platform.conf.
    Moved marvell-arm64/platform.conf to marvell/platform_arm64.conf.
    Moved marvell-armhf/platform.conf to marvell/platform_armhf.conf.

  • [arm64] Adjust the path to read sonic_fit.its
    Use CONFIGURED_PLATFORM based path while reading sonic_fit.its.

  • [arm64/armhf] Add architecure suffix for output image
    This change is needed to generate the output image with architecture suffix for arm64/armhf platform.
    For arm64, defined a marvell specific file to avoid affect on other vendors.

  • Change platform_asic name to marvell
    Change 'marvell-arm64' and 'marvell-armhf' platform_asic name to 'marvell'.

  • Remove marvell-armhf and marvell-arm64 folders
    Removed 'marvell-armhf' and 'marvell-arm64' as infra is combined with 'marvell' folder.

  • Update readme to reflect new build commands

  • Change azure pipeline commands for marvell

New build commands:
AMD64 - make configure PLATFORM=marvell && make target/sonic-marvell.bin
ARM64 - make configure PLATFORM=marvell PLATFORM_ARCH=arm64 && make target/sonic-marvell-arm64.bin
ARMHF - make configure PLATFORM=marvell PLATFORM_ARCH=armhf && make target/sonic-marvell-armhf.bin

How to verify it

Built images for all the platform and installed on hardware to verify the port up.
Verified onie and sonic-sonic install.

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

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

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

@Pavan-Nokia please review.

@pavannaregundi pavannaregundi force-pushed the master branch 3 times, most recently from 90b6ff5 to 2174f65 Compare February 22, 2024 02:50
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of the if-checks here, can the top-level pipeline build files be updated to point to marvell instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@saiarcot895 If we look at the changes in the build commands, only configure command is different in "PLATFORM=marvell" string. Other commands are same. So I opted this change.
If we change name in top-level pipeline( ex: name: marvell-armhf) we need to change or checks multiple places such as how the build pipelines are fetched in "Azure.sonic-buildimage.official.marvell-armhf?branchName=master&label=Marvell-armhf" or while running a make command(make target/sonic-marvell-armhf.bin) for build. I thought this is less change, and does not break other arm64 platform builds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm, @xumia, thoughts?

Copy link
Copy Markdown
Collaborator

@xumia xumia Mar 1, 2024

Choose a reason for hiding this comment

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

How about define a new parameter in azure-pipelines-build.yml for marvell-armhf and marvell-amd64.
https://github.com/sonic-net/sonic-buildimage/blob/7f454a778a3f324304c241ffcbb4fc012bf75c90/.azure-pipelines/azure-pipelines-build.yml#L100
For example:

           PLATFORM: marvell

If the variable PLATFORM is not defined (empty), then use PLATFORM_AZP

[ -z "$PLATFORM" ] && PLATFORM=$(PLATFORM_AZP)
ENABLE_DOCKER_BASE_PULL=y make PLATFORM=$PLATFORM PLATFORM_ARCH=$(PLATFORM_ARCH) $(BUILD_OPTIONS) configure
// There is a little different $PLATFORM and $(PLATFORM), be careful.

Any other platforms have the similar requirements, only need to override the variable, no logic change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@xumia Have pushed the changes as recommended by you. Please review.

Comment on lines 85 to 89
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The file has been deprecated, and it is no use. The change can be skipped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. Reverting this change.

@pavannaregundi pavannaregundi force-pushed the master branch 3 times, most recently from 6d0f3b5 to 56d0df8 Compare March 4, 2024 03:38
@pavannaregundi
Copy link
Copy Markdown
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

Pull request contains merge conflicts.

@Blueve
Copy link
Copy Markdown
Contributor

Blueve commented Mar 6, 2024

@Blueve Blueve requested a review from liushilongbuaa March 6, 2024 12:55
@Blueve
Copy link
Copy Markdown
Contributor

Blueve commented Mar 7, 2024

Hi @pavannaregundi , can you trigger a new build by typing /azpw run Azure.sonic-buildimage in comments again?

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

sudo apt-get install -y acl
sudo bash -c "echo 1 > /proc/sys/vm/compact_memory"
ENABLE_DOCKER_BASE_PULL=y make PLATFORM=$(PLATFORM_AZP) PLATFORM_ARCH=$(PLATFORM_ARCH) $(BUILD_OPTIONS) configure
[ -z "$PLATFORM" ] && PLATFORM=$(PLATFORM_AZP)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why? When PLATFORM is set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@liushilongbuaa This is a new optional variable introduced in 'azure-pipelines-build.yml' to select the platform if it is different from default PLATFORM_AZP. Basically, pick platform name from PLATFORM_AZP when PLATFORM variable is not set.

Details of this change are already present in comments above.

sudo LANG=C chroot $FILESYSTEM_ROOT mv /boot/u${INITRD_FILE} /boot/$INITRD_FILE
else
sudo cp -v $PLATFORM_DIR/${sonic_asic_platform}-${CONFIGURED_ARCH}/sonic_fit.its $FILESYSTEM_ROOT/boot/
sudo cp -v $PLATFORM_DIR/$CONFIGURED_PLATFORM/sonic_fit.its $FILESYSTEM_ROOT/boot/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

centec-arm64 is also impacted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. But $CONFIGURED_PLATFORM for "centec-arm64" is "centec-arm64". So change will work.

@liushilongbuaa
Copy link
Copy Markdown
Contributor

can you trigger a new build by typing /azpw run Azure.sonic-buildimage in comments again?

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@liushilongbuaa
Copy link
Copy Markdown
Contributor

can you trigger a new build by typing /azpw run Azure.sonic-buildimage in comments again?

Done. Build still failing.

Can you debug locally to fix sonic-config-engine UT?

@liushilongbuaa
Copy link
Copy Markdown
Contributor

Hi @neethajohn , can you help on the UT?

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

can you trigger a new build by typing /azpw run Azure.sonic-buildimage in comments again?

Done. Build still failing.

Can you debug locally to fix sonic-config-engine UT?

@liushilongbuaa We will try looking at this. However, someone who has experience in this area can help us will be good.

@radha-danda
Copy link
Copy Markdown

/azpw run Azure.sonic-buildimage

@Blueve
Copy link
Copy Markdown
Contributor

Blueve commented Apr 8, 2024

Potential fix of build ut issue: #18566

The marvell build pipeline continuously fail, seems related to the new added PLATFORM variable. And above PR can clean up the variable before build py-wheel

@liushilongbuaa
Copy link
Copy Markdown
Contributor

@pavannaregundi , can you rerun PR checker by comment '/azpw run Azure.sonic-buildimage'?

'mrvl-prestera' and 'sonic-platform-marvell' submodules are
moved from 'marvell-arm64'to 'marvell'.
Additionaly submodules are changed to support multi-architecture build.

Signed-off-by: Pavan Naregundi <[email protected]>
sonic-platform-nokia is created from merge of 'marvell-armhf'
and 'marvell-arm64' in 'marvell'.
Additionaly changes are done to support multi-arch build.

Signed-off-by: Pavan Naregundi <[email protected]>
Combine all the architecture targets in makefiles.
Added CONFIGURED_ARCH check to build for specific architecture.
Updated sai deb which matches latest SAI commit.

Signed-off-by: Pavan Naregundi <[email protected]>
Changed build_image.sh and onie-mk-demo.sh to read architecture
specific platform.conf.
Moved marvell-arm64/platform.conf to marvell/platform_arm64.conf.
Moved marvell-armhf/platform.conf to marvell/platform_armhf.conf.

Signed-off-by: Pavan Naregundi <[email protected]>
Use CONFIGURED_PLATFORM based path while reading sonic_fit.its.

Signed-off-by: Pavan Naregundi <[email protected]>
This change is needed to generate the output image with
architecture suffix for arm64/armhf platform.
For arm64, defined a marvell specific file to avoid affect
on other vendors.

Signed-off-by: Pavan Naregundi <[email protected]>
Change 'marvell-arm64' and 'marvell-armhf' platform_asic name
to 'marvell'.

Signed-off-by: Pavan Naregundi <[email protected]>
Removed 'marvell-armhf' and 'marvell-arm64' as infra is
combined with 'marvell' folder.

Signed-off-by: Pavan Naregundi <[email protected]>
@pavannaregundi pavannaregundi force-pushed the master branch 3 times, most recently from 793f219 to 44e9f1a Compare April 9, 2024 04:58
make configure is changed as below for 'marvell-arm64' and 'marvell-armhf'.
arm64 - make configure PLATFORM=marvell PLATFORM_ARCH=arm64
armhf - make configure PLATFORM=marvell PLATFORM_ARCH=armhf

Signed-off-by: Pavan Naregundi <[email protected]>
@pavannaregundi
Copy link
Copy Markdown
Contributor Author

@Blueve @liushilongbuaa I have pushed a change by changing the azure new variable for platform to "PLATFORM_NAME", instead of "PLATFORM". It could be that yml is setting this as env variable.

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

@Blueve @liushilongbuaa I have pushed a change by changing the azure new variable for platform to "PLATFORM_NAME", instead of "PLATFORM". It could be that yml is setting this as env variable.

Builds are passing now.

Copy link
Copy Markdown
Contributor

@liushilongbuaa liushilongbuaa left a comment

Choose a reason for hiding this comment

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

LGTM

@pavannaregundi
Copy link
Copy Markdown
Contributor Author

@prgeor All comments are addressed. Can we merge this PR.

@prgeor
Copy link
Copy Markdown
Contributor

prgeor commented Apr 14, 2024

@prgeor All comments are addressed. Can we merge this PR.

@lguohan @yxieca can you merge?

@yxieca yxieca merged commit 006f4b2 into sonic-net:master Apr 15, 2024
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.

9 participants