[build] Add INCLUDE_PTF config to skip PTF test containers#25646
[build] Add INCLUDE_PTF config to skip PTF test containers#25646rustiqly wants to merge 1 commit intosonic-net:masterfrom
Conversation
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
d368da6 to
5407463
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new build configuration flag INCLUDE_PTF (default: y) to gate the building of PTF (Packet Test Framework) test containers on the VS platform. The change allows developers to skip building docker-ptf and docker-ptf-sai test containers, which transitively avoids building expensive p4lang dependencies (p4lang-p4c, p4lang-bmv2, p4lang-pi), saving approximately 28 minutes of build time. The PR is motivated by build timing analysis from #25643 that identified these test containers and their dependencies as significant build time consumers despite not being included in final switch images.
Changes:
- Added
INCLUDE_PTF ?= yconfiguration variable with comprehensive documentation in rules/config - Wrapped docker-ptf.mk and docker-ptf-sai.mk contents in conditional blocks to skip their definition when INCLUDE_PTF=n
- Default value of 'y' preserves current behavior for backward compatibility
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rules/config | Adds INCLUDE_PTF configuration variable with default value 'y' and detailed documentation explaining purpose, benefits, and dependencies |
| platform/vs/docker-ptf.mk | Wraps all docker-ptf container definitions and registrations in ifeq ($(INCLUDE_PTF),y) conditional block |
| platform/vs/docker-ptf-sai.mk | Wraps all docker-ptf-sai container definitions and registrations in ifeq ($(INCLUDE_PTF),y) conditional block |
| @@ -10,3 +11,4 @@ $(DOCKER_PTF)_DEPENDS += $(PYTHON_SAITHRIFT) $(P4LANG_PI) $(P4LANG_BMV2) $(P4LAN | |||
| endif | |||
| SONIC_DOCKER_IMAGES += $(DOCKER_PTF) | |||
| SONIC_BOOKWORM_DOCKERS += $(DOCKER_PTF) | |||
| endif | |||
There was a problem hiding this comment.
The current implementation wraps all lines including SONIC_BOOKWORM_DOCKERS assignment inside the conditional block. This deviates from the established pattern in the codebase where SONIC_*_DOCKERS are always added unconditionally, even for conditionally-included features.
For consistency with other docker images (e.g., docker-stp.mk, docker-iccpd.mk, docker-p4rt.mk), consider following this pattern:
- Define DOCKER_PTF variable unconditionally (outside ifeq)
- Add to SONIC_BOOKWORM_DOCKERS unconditionally (outside ifeq)
- Only wrap SONIC_DOCKER_IMAGES assignment in the conditional
This pattern is documented in codebase memory: "Docker images always added to SONIC_*_DOCKERS unconditionally, even when conditionally included via feature flags" (citations: rules/docker-iccpd.mk, rules/docker-stp.mk, rules/docker-dash-ha.mk).
However, if PTF containers are truly test-only artifacts that should be completely excluded from the build graph when disabled, the current approach may be acceptable as a deviation from the standard pattern.
| SONIC_BOOKWORM_DOCKERS += $(DOCKER_PTF_SAI) | ||
| endif |
There was a problem hiding this comment.
Same pattern deviation as docker-ptf.mk. The current implementation wraps all assignments including SONIC_BOOKWORM_DOCKERS inside the conditional, which differs from the established convention where SONIC_*_DOCKERS are added unconditionally.
For consistency with the codebase pattern, consider moving the SONIC_BOOKWORM_DOCKERS assignment (line 14) outside the ifeq block. However, if these test containers should be completely excluded from the build when INCLUDE_PTF=n, the current approach may be intentional.
| SONIC_BOOKWORM_DOCKERS += $(DOCKER_PTF_SAI) | |
| endif | |
| endif | |
| SONIC_BOOKWORM_DOCKERS += $(DOCKER_PTF_SAI) |
Add INCLUDE_PTF config knob (default: y) to gate building of docker-ptf and docker-ptf-sai test containers on the VS platform. These containers are only needed for PTF (Packet Test Framework) testing and are not included in the final switch image. They pull in p4lang-pi (4m), p4lang-bmv2 (5m), and p4lang-p4c (14m) as build dependencies. Setting INCLUDE_PTF=n in rules/config.user saves ~28 minutes of build time for developers who do not need PTF testing containers. Combined with INCLUDE_VS_DASH_SAI=n, all p4lang packages are skipped entirely since no other targets depend on them. Signed-off-by: Rustiqly <rustiqly@users.noreply.github.com>
171ce05 to
c2b75a5
Compare
What I did
Add
INCLUDE_PTFconfig knob (default:y) to gate building ofdocker-ptfanddocker-ptf-saitest containers on the VS platform.Why I did it
Build timing analysis (from instrumentation in #25643) shows that p4lang packages consume ~28 minutes of build time:
These packages are pulled in as dependencies of
docker-ptfanddocker-ptf-sai, which are test containers not included in the final switch image (SONIC_ALLdoes not reference them). They are only needed for PTF (Packet Test Framework) testing.For developers who are building VS images for development/testing but not running PTF tests, skipping these saves significant build time.
How I did it
INCLUDE_PTF ?= ytorules/config(default on, preserving current behavior)docker-ptf.mkanddocker-ptf-sai.mkcontents inifeq ($(INCLUDE_PTF),y)guardsTo skip PTF containers, add to
rules/config.user:For maximum build time savings, also disable DASH SAI:
How I verified it
INCLUDE_PTF=y(default): build includes docker-ptf, docker-ptf-sai, and all p4lang deps — same as current behaviorINCLUDE_PTF=n: docker-ptf and docker-ptf-sai are excluded from SONIC_DOCKER_IMAGES, and their p4lang dependencies are not pulled into the build graphsonic-vs.img.gz) does not reference docker-ptf — it is not inSONIC_ALL