Enable P4RT at build time and disable at startup #10499

ntoorchi · 2022-04-07T22:42:06Z

Why I did it

Currently at the Azure build system, the P4RT container is disabled by default at the build time. Here the goal is to include the P4RT container at the build time while disabling it at the runtime. The user can enable/disable the p4rt app through the config based on the preference.

How I did it

Changed the config in rules/config and init-cfg.json.j2

How to verify it

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

Description for the changelog

Link to config_db schema for YANG module changes

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

bocon13 · 2022-04-19T21:33:35Z

@ntoorchi can you add some time and space comparisons?

azure-pipelines · 2022-07-07T18:54:13Z

Commenter does not have sufficient privileges for PR 10499 in repo Azure/sonic-buildimage

ntoorchi · 2022-07-07T21:39:20Z

/azpw run

mssonicbld · 2022-07-07T21:39:21Z

/AzurePipelines run

azure-pipelines · 2022-07-07T21:39:26Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

ntoorchi · 2022-07-07T21:43:59Z

/azp run broadcom

azure-pipelines · 2022-07-07T21:44:04Z

Commenter does not have sufficient privileges for PR 10499 in repo Azure/sonic-buildimage

ntoorchi · 2022-07-07T21:44:33Z

/azpw run broadcom

mssonicbld · 2022-07-07T21:44:34Z

/AzurePipelines run broadcom

azure-pipelines · 2022-07-07T21:44:39Z

No pipelines are associated with this pull request.

ntoorchi · 2022-07-21T19:09:39Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-07-21T19:09:41Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-07-21T19:09:53Z

Azure Pipelines successfully started running 1 pipeline(s).

rhalstea · 2022-08-15T17:48:23Z

@ntoorchi can we rebuild this, or do we know why the build failed?

ntoorchi · 2022-08-16T00:24:21Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-08-16T00:24:26Z

Commenter does not have sufficient privileges for PR 10499 in repo sonic-net/sonic-buildimage

ntoorchi · 2022-08-16T00:25:21Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-08-16T00:25:22Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-08-16T00:25:34Z

Azure Pipelines successfully started running 1 pipeline(s).

ntoorchi · 2022-08-16T04:47:49Z

@ntoorchi can we rebuild this, or do we know why the build failed?

@rhalstea marvell_armhf fails. It's due to this block:

sonic-buildimage/slave.mk

Line 178 in 485edbf

ifeq ($(CONFIGURED_ARCH),armhf)

The error is recipe commences before first target. Stop. so it might be due to @echo.

rhalstea · 2022-08-16T17:56:39Z

Thanks @ntoorchi,

Was this re-based to be at the most recent sonic-buildimage? I'm not sure how to tell if it was, but marvell_armhf doesn't seem to have an issue on the mainline.

ntoorchi · 2022-08-17T02:04:27Z

@rhalstea no this is not rebased recently, but I tried on a locally rebased sonic-buildimage too. This looks like a syntax error in the slave.mk, happens when INCLUDE_P4RT=y and CONFIGURED_ARCH=armhf (running on arm processor). I guess the reason is the tab before @echo when it is not under any target. I tried using $(info ...) instead of echo and it worked. Should I add the fix to this PR or open a separate PR or issue?

rhalstea · 2022-08-17T15:58:42Z

Thanks,
We'll need this PR to pass all the "Required" tests before we can submit so I think it has to go here.

mssonicbld · 2022-08-21T03:22:53Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-08-21T03:23:05Z

Azure Pipelines successfully started running 1 pipeline(s).

ntoorchi · 2022-08-21T16:42:36Z

@rhalstea all checks passed!

rhalstea · 2022-08-22T18:09:37Z

Thanks @ntoorchi!

@prsunny would you be able to review this? Asking since you seem to be who we mainly send PINS PRs to :)

CC'ing: @bhagatyj and @bocon13

bhagatyj · 2022-08-29T17:33:48Z

LGTM

ntoorchi · 2022-08-30T21:59:13Z

@qiluo-msft, @zhangyanzhao can you please review this PR or assign someone to review? The goal is to include the P4RT at the build time. However it is disabled by default at the runtime. The PR also includes a fix to the slave.mk in order to pass marvell_armhf build.

qiluo-msft · 2022-09-17T01:05:04Z

Share the same concern: can you add some time and space comparisons?

ntoorchi · 2022-09-19T17:00:09Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-09-19T17:00:11Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-09-19T17:00:23Z

Azure Pipelines successfully started running 1 pipeline(s).

ntoorchi · 2022-09-20T18:20:04Z

Share the same concern: can you add some time and space comparisons?

Thanks @qiluo-msft for reviewing. I am trying to use Azure Pipeline artifacts as references but build time and size usually varies by time. In general, I have noticed build time is quite comparable, and there is some increase in size due to adding P4RT.

Pipeline it is taking a long time (or failing) at this moment. But the sonic-buildimage.broadcom was successful and it took around 6h while Master build has taken around 8h. But at the normal condition, the PR build time was quite comparable with other builds, e.g. around 2h 30m.

Comparing size of downloaded artifact with the one from the commit 9750cb4, there is slightly increase in size due to adding P4RT. We have disabled some of the debugging tools to minimize the overhead.

commit	size sonic-buildimage.broadcom	size sonic-aboot-broadcom.swi
PR 10499	10.27G	1.13G
commit 9750cd4	9.88G	1.12 G

Please let me know if you have any specific comparison in mind.

qiluo-msft · 2022-09-20T20:17:14Z

The image sizes increases by ~10M.
Since most users do not use P4RT, could you maintain private branches/patches on top of the public branches, so there is no impact to unrelated users. We have to support various platforms and some have super small disks.

bocon13 · 2022-09-22T19:22:38Z

I'm trying to understand the requirement here.

From my perspective, it looks like enabling PINS adds ~10MB to the switch image that we deploy. This seems like a very small impact to users, and it will be great to have PINS in the image to enable more users to try it out without rebuilding.

The ~400MB is the build time artifacts on a machine that likely has 1TB+ available. Personally, I don't see a problem with this, but if there is concern, we can just add a step to remove intermediate artifacts from the PINS build before they are stored in the CI system.

What do you think?

qiluo-msft · 2022-09-22T20:41:04Z

I see your point and refine my last comment. The size of build time artifacts is not a concern. ~10M of image size increasing is still a valid concern. @lguohan do you have any comment?

ntoorchi · 2022-09-30T19:44:36Z

I see your point and refine my last comment. The size of build time artifacts is not a concern. ~10M of image size increasing is still a valid concern. @lguohan do you have any comment?

Thanks @qiluo-msft for your comment. As @bocon13 mentioned, enabling PINS would be valuable as currently many vendors are trying it out. It will be more convenient if it's present in the image, encouraging more users to trying it.

cc @bhagatyj @rhalstea

bhagatyj · 2022-10-01T15:19:24Z

Since PINS introduction last year, multiple organizations have been working on different use cases. We are also in the path of upstreaming tests. As a percentage of image size, the 10M is quite small. For this size, this container brings in a critical functionality of SDN and remote controllability.

qiluo-msft · 2022-10-20T23:07:34Z

Please solve the conflicts.

Why I did it The extra spaces cause the error "recipe commences before first target. Stop."

ntoorchi · 2022-10-26T17:39:21Z

Please solve the conflicts.

Thanks Qi, I rebased and resolved the conflict but the pipeline builds are failing at this moment.

ntoorchi · 2022-10-27T17:41:06Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-10-27T17:41:08Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-10-27T17:41:21Z

Azure Pipelines successfully started running 1 pipeline(s).

rhalstea · 2022-10-31T17:36:11Z

Thanks @ntoorchi @qiluo-msft and @xumia,

The build and test look good. Can we merge this?

ntoorchi force-pushed the enable-p4rt branch 2 times, most recently from a309172 to 3ba6bff Compare May 5, 2022 19:07

ntoorchi marked this pull request as ready for review May 9, 2022 23:02

ntoorchi requested review from qiluo-msft, xumia and lguohan as code owners May 9, 2022 23:02

ntoorchi mentioned this pull request Jun 21, 2022

Feature request to enable P4RT App at the build time and allow P4RT App to read configs from CONFIG_DB #11211

Closed

xumia approved these changes Sep 16, 2022

View reviewed changes

qiluo-msft approved these changes Oct 20, 2022

View reviewed changes

ntoorchi and others added 3 commits October 24, 2022 12:43

Enable P4RT at build time and disable at startup

d677d17

disabling p4rt dbg from docker dbg image.

ce3f2be

TODO comment added.

623e17a

ntoorchi force-pushed the enable-p4rt branch from 07396eb to 623e17a Compare October 24, 2022 20:01

Removing some spaces in slave.mk to avoid target error.

1528090

Why I did it The extra spaces cause the error "recipe commences before first target. Stop."

qiluo-msft merged commit 45d1746 into sonic-net:master Oct 31, 2022

Enable P4RT at build time and disable at startup #10499

Enable P4RT at build time and disable at startup #10499

Conversation

ntoorchi commented Apr 7, 2022 • edited Loading

Why I did it

How I did it

How to verify it

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

Description for the changelog

Link to config_db schema for YANG module changes

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

bocon13 commented Apr 19, 2022

azure-pipelines bot commented Jul 7, 2022

ntoorchi commented Jul 7, 2022

mssonicbld commented Jul 7, 2022

azure-pipelines bot commented Jul 7, 2022

ntoorchi commented Jul 7, 2022

azure-pipelines bot commented Jul 7, 2022

ntoorchi commented Jul 7, 2022

mssonicbld commented Jul 7, 2022

azure-pipelines bot commented Jul 7, 2022

ntoorchi commented Jul 21, 2022

mssonicbld commented Jul 21, 2022

azure-pipelines bot commented Jul 21, 2022

rhalstea commented Aug 15, 2022

ntoorchi commented Aug 16, 2022

azure-pipelines bot commented Aug 16, 2022

ntoorchi commented Aug 16, 2022

mssonicbld commented Aug 16, 2022

azure-pipelines bot commented Aug 16, 2022

ntoorchi commented Aug 16, 2022

rhalstea commented Aug 16, 2022

ntoorchi commented Aug 17, 2022

rhalstea commented Aug 17, 2022 • edited Loading

mssonicbld commented Aug 21, 2022

azure-pipelines bot commented Aug 21, 2022

ntoorchi commented Aug 21, 2022 • edited Loading

rhalstea commented Aug 22, 2022

bhagatyj commented Aug 29, 2022 • edited Loading

ntoorchi commented Aug 30, 2022

qiluo-msft commented Sep 17, 2022

ntoorchi commented Sep 19, 2022

mssonicbld commented Sep 19, 2022

azure-pipelines bot commented Sep 19, 2022

ntoorchi commented Sep 20, 2022

qiluo-msft commented Sep 20, 2022 • edited Loading

bocon13 commented Sep 22, 2022

qiluo-msft commented Sep 22, 2022

ntoorchi commented Sep 30, 2022

bhagatyj commented Oct 1, 2022

qiluo-msft commented Oct 20, 2022

ntoorchi commented Oct 26, 2022

ntoorchi commented Oct 27, 2022

mssonicbld commented Oct 27, 2022

azure-pipelines bot commented Oct 27, 2022

rhalstea commented Oct 31, 2022

ntoorchi commented Apr 7, 2022 •

edited

Loading

rhalstea commented Aug 17, 2022 •

edited

Loading

ntoorchi commented Aug 21, 2022 •

edited

Loading

bhagatyj commented Aug 29, 2022 •

edited

Loading

qiluo-msft commented Sep 20, 2022 •

edited

Loading