-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[build] add support for 2 stage rootfs build #15924
Conversation
HLD: |
/azpw run |
/AzurePipelines run |
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. |
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please build with and without this feature and compare file /etc/sonic/sonic_version.yml Lines 1341 to 1343 in 0bd8c3b
sonic-buildimage/build_debian.sh Line 591 in 0bd8c3b
sonic-buildimage/files/build_templates/sonic_version.yml.j2 Lines 23 to 28 in 0bd8c3b
Possible solution is to generate file sonic_version.yml later (after 1st stage) |
SONIC_TARGET_LIST += $(addprefix $(TARGET_PATH)/, $(SONIC_RFS_TARGETS)) | ||
endif | ||
|
||
$(addprefix $(TARGET_PATH)/, $(SONIC_RFS_TARGETS)) : $(TARGET_PATH)/% : \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make sure the new target SONIC_RFS_TARGETS
is reused if we build multiple vendors image in series?
We also have reproducible mechanism in rules/*.dep files. Is it possible to define this target's dep file, so the target is reused as cached for other later build jobs?
@xumia to review. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target is reused. I've also added a cache: a880d53
Since the targets are defined dynamically, I could not add the '.dep' file. Instead, the relevant variables are also set dynamically.
who ever is reviewing this PR, we have a target to finish this review and enjoy the approved time of build in the next week. |
Still need to fix one issue I mentioned before. Also @qiluo-msft suggested to use cache for 1st-stage squashfs file. Need feedback from @Yakiv-Huryk is it possible or not. |
slave.mk
Outdated
SONIC_RFS_TARGETS= $(foreach installer, $(SONIC_INSTALLERS), $(call rfs_get_installer_dependencies,$(installer))) | ||
|
||
ifeq ($(ENABLE_RFS_SPLIT_BUILD),y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid the if..else statement inside the build_debian.sh file.
Can you split the build_debian.sh into two separate files?
- build_debian_rootfs.sh => which contains all the standard squashfs packages
- build_debian_sonic.sh => Includes all the sonic packages into existing squashfs and creates the dockerfs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why splitting the build_debian.sh is better. Also, with the current design, the feature is optional and configurable (via ENABLE_RFS_SPLIT_BUILD). If we split the build_debian.sh, the current way to build will no longer be available.
Can you please help to understand why the if..else is better avoided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we split the build_debian.sh, the current way to build will no longer be available.
I think this feature (RFS_SPLIT_BUILD) should be enabled by default in the future.
Two versions are hard to maintain.
In this case, splitting into two separate files is a good solution.
But probably we can merge this PR as a temporary solution and test builds with enabled option for a while.
@@ -1573,7 +1575,9 @@ SONIC_CLEAN_TARGETS += $(addsuffix -clean,$(addprefix $(TARGET_PATH)/, \ | |||
$(SONIC_DOCKER_IMAGES) \ | |||
$(SONIC_DOCKER_DBG_IMAGES) \ | |||
$(SONIC_SIMPLE_DOCKER_IMAGES) \ | |||
$(SONIC_INSTALLERS))) | |||
$(SONIC_INSTALLERS) \ | |||
$(SONIC_RFS_TARGETS))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add caching support for rootfs preparation in squashfs file system?
You can refer DPKG caching framework for deb packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in a880d53
@Yakiv-Huryk So need to check these lines in rules/functions:
Maybe Many files from Need to fix these issues or merge this PR without DPKG cache. We can add DPKG cache support for rootfs later after additional testing. But anyway for caching need review from @xumia or @Kalimuthu-Velappan. I'm not very familiar with these feature. All other issues I mentioned earlier are fixed. |
@k-v1 Regarding the first stage dependencies (including many files from |
dd87b35
to
b12c093
Compare
Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
* add define_rfs_target function to define cache-related variables * add SONIC_RFS_TARGETS handling to Makefile.cache Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
…uild Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
* cache-related variables are now defined in Makefile.cache * removed define_rfs_target * add a list of files that are used in the first stage of RFS build Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
* removed ENABLE_RFS_SPLIT_BUILD variable Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
This is to address the fact that the build_debian_base_system.sh uses $TARGET/baseimage, so running two build_debian_base_system.sh in parallel is a race. * add DEPENDENT_RFS (analogous to DEPENDENT_MACHINE) * rearrange the build so that the installer's MACHINE RFS and DEPENDENT_MACHINE RFS are not built in parallel Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
* add all the files from files/sshd,dhcp,image_config Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
* add more files to RFS_DEB_FILES * improved umount in build_debian.sh (taken from sonic-net#16672) * explicit RFS_SPLIT_LAST_STAGE=n when building the first stage Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
Makefile.cache
Outdated
$(shell git ls-files files/apparmor) \ | ||
$(shell git ls-files files/apt) \ | ||
$(wildcard files/sshd/*) \ | ||
$(wildcard files/dhcp/*) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be changed to git ls-files
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
@saiarcot895 could you please take further look now that the comments were addressed? |
@Yakiv-Huryk many thanks for this great achievements. |
@Yakiv-Huryk Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md |
Original feature was implemented and merged to master. I suggested additional fixes and improvements (#17100). Other possible solution if @Yakiv-Huryk provides fixes by himself (e.g. partially based on #17100). |
It’s GA. Since it’s a build improvement, it doesn't really fit the Feature Quality Definition. There is no flag to enable/disable it and no sonic-mgmt tests. |
This adds optimization for the SONiC image build by splitting the final build step into two stages. It allows running the first stage in parallel, improving build time.
The optimization is enabled via new rules/config flag ENABLE_RFS_SPLIT_BUILD (disabled by default)
Why I did it
To improve a build time.
Work item tracking
How I did it
Added a logic to run build_debian.sh in two stages, transferring the progress via a new build artifact.
How to verify it
Which release branch to backport (provide reason below if selected)
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)