Upgrade brcm syncd to buster. #6106
Conversation
There was a problem hiding this comment.
As part of this PR, please also switch to using Python 3 to run supervisord_dependent_startup and supervisor-proc-exit-listener. In the file platform/broadcom/docker-syncd-brcm/supervisord.conf, please change the line
command=python2 -m supervisord_dependent_startupto
command=python3 -m supervisord_dependent_startupAnd change the line
command=python2 /usr/bin/supervisor-proc-exit-listener --container-name syncdto
command=/usr/bin/supervisor-proc-exit-listener --container-name syncd
|
retest vs please |
Done. |
|
please add proper description of the pr. |
|
can you also fill the questions in the description.
|
platform/broadcom/docker-ptf-brcm.mk
Outdated
| @@ -5,4 +5,4 @@ $(DOCKER_PTF_BRCM)_PATH = $(DOCKERS_PATH)/docker-ptf-saithrift | |||
| $(DOCKER_PTF_BRCM)_DEPENDS += $(PYTHON_SAITHRIFT) | |||
| $(DOCKER_PTF_BRCM)_LOAD_DOCKERS += $(DOCKER_PTF) | |||
| SONIC_DOCKER_IMAGES += $(DOCKER_PTF_BRCM) | |||
| SONIC_STRETCH_DOCKERS += $(DOCKER_PTF_BRCM) | |||
| SONIC_BUSTER_DOCKERS += $(DOCKER_PTF_BRCM) | |||
There was a problem hiding this comment.
I don't think it is necessary to append to SONIC_BUSTER_DOCKERS. I don't think we reference this variable anywhere. All Dockers are considered Buster by default, unless appended to SONIC_STRETCH_DOCKERS. @lguohan to confirm.
There was a problem hiding this comment.
Sure. Will wait for @lguohan to confirm.
There was a problem hiding this comment.
@jleveque , yes you are right.
|
retest broadcom please |
| @@ -40,4 +40,4 @@ DELTA_AGC032_PLATFORM_MODULE = platform-modules-agc032_$(DELTA_AGC032_PLATFORM_M | |||
| $(DELTA_AGC032_PLATFORM_MODULE)_PLATFORM = x86_64-delta_agc032-r0 | |||
| $(eval $(call add_extra_package,$(DELTA_AG9032V1_PLATFORM_MODULE),$(DELTA_AGC032_PLATFORM_MODULE))) | |||
|
|
|||
| SONIC_STRETCH_DEBS += $(DELTA_AG9032V1_PLATFORM_MODULE) | |||
| SONIC_BUSTER_DEBS += $(DELTA_AG9032V1_PLATFORM_MODULE) | |||
There was a problem hiding this comment.
we can do that in a different pr.
There was a problem hiding this comment.
Done. @lguohan Shall i open a different PR for check in these changes ? or is there already one i can use ?
|
@jleveque , can you approve? |
| SONIC_DOCKER_IMAGES += $(DOCKER_SAISERVER_BRCM) | ||
| SONIC_STRETCH_DOCKERS += $(DOCKER_SAISERVER_BRCM) | ||
| SONIC_BUSTER_DOCKERS += $(DOCKER_SAISERVER_BRCM) |
There was a problem hiding this comment.
As discussed in a previous comment, we do not reference a SONIC_BUSTER_DOCKERS variable. This line is unnecessary, correct, @lguohan?
There was a problem hiding this comment.
Done, reverted the change.
There was a problem hiding this comment.
I think you should still delete the line, just not add the SONIC_BUSTER_DOCKERS line. This is now a Buster Docker, not Stretch, correct?
| @@ -12,7 +12,7 @@ endif | |||
| $(DOCKER_SYNCD_BRCM_RPC)_FILES += $(DSSERVE) $(BCMCMD) $(SUPERVISOR_PROC_EXIT_LISTENER_SCRIPT) | |||
| $(DOCKER_SYNCD_BRCM_RPC)_LOAD_DOCKERS += $(DOCKER_SYNCD_BASE) | |||
| SONIC_DOCKER_IMAGES += $(DOCKER_SYNCD_BRCM_RPC) | |||
| SONIC_STRETCH_DOCKERS += $(DOCKER_SYNCD_BRCM_RPC) | |||
| SONIC_BUSTER_DOCKERS += $(DOCKER_SYNCD_BRCM_RPC) | |||
There was a problem hiding this comment.
As discussed in a previous comment, we do not reference a SONIC_BUSTER_DOCKERS variable. This line is unnecessary, correct, @lguohan?
There was a problem hiding this comment.
Done. Reverted the change.
There was a problem hiding this comment.
I think you should still delete the line, just not add the SONIC_BUSTER_DOCKERS line. This is now a Buster Docker, not Stretch, correct?
| @@ -4,7 +4,7 @@ DOCKER_SAISERVER_BRCM = docker-saiserver-brcm.gz | |||
| $(DOCKER_SAISERVER_BRCM)_PATH = $(PLATFORM_PATH)/docker-saiserver-brcm | |||
| $(DOCKER_SAISERVER_BRCM)_DEPENDS += $(SAISERVER) | |||
| $(DOCKER_SAISERVER_BRCM)_FILES += $(DSSERVE) $(BCMCMD) | |||
| $(DOCKER_SAISERVER_BRCM)_LOAD_DOCKERS += $(DOCKER_CONFIG_ENGINE_STRETCH) | |||
| $(DOCKER_SAISERVER_BRCM)_LOAD_DOCKERS += $(DOCKER_CONFIG_ENGINE_BUSTER) | |||
| SONIC_DOCKER_IMAGES += $(DOCKER_SAISERVER_BRCM) | |||
| SONIC_STRETCH_DOCKERS += $(DOCKER_SAISERVER_BRCM) | |||
There was a problem hiding this comment.
Shouldn't you still delete this line? This is now a Buster Docker, not Stretch, correct?
There was a problem hiding this comment.
Just confirming that "$(DOCKER_SAISERVER_BRCM)_LOAD_DOCKERS += $(DOCKER_CONFIG_ENGINE_STRETCH)" need to be removed too ?
There was a problem hiding this comment.
and not adding "$(DOCKER_SAISERVER_BRCM)_LOAD_DOCKERS += $(DOCKER_CONFIG_ENGINE_BUSTER)" ?
There was a problem hiding this comment.
I assumed you are now basing this container off Buster. If so, then I believe this section should read:
DOCKER_SAISERVER_BRCM = docker-saiserver-brcm.gz
$(DOCKER_SAISERVER_BRCM)_PATH = $(PLATFORM_PATH)/docker-saiserver-brcm
$(DOCKER_SAISERVER_BRCM)_DEPENDS += $(SAISERVER)
$(DOCKER_SAISERVER_BRCM)_FILES += $(DSSERVE) $(BCMCMD)
$(DOCKER_SAISERVER_BRCM)_LOAD_DOCKERS += $(DOCKER_CONFIG_ENGINE_BUSTER)
SONIC_DOCKER_IMAGES += $(DOCKER_SAISERVER_BRCM)
|
@vmittal-msft: FYI, for future PRs, I suggest only using force-pushes when necessary (i.e., after a rebase), because we lose track of the commit history. |
|
retest vs please |
|
retest vsimage please |
2 similar comments
|
retest vsimage please |
|
retest vsimage please |
- Why I did it
To upgrade brcm syncd to buster
- How I did it
- How to verify it
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
Upgrade syncd from stretch to buster
- A picture of a cute animal (not mandatory but encouraged)