Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skip CMIS manager #10907

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Skip CMIS manager #10907

merged 4 commits into from
Jun 22, 2022

Conversation

prgeor
Copy link
Contributor

@prgeor prgeor commented May 23, 2022 •
edited
Loading

Why I did it

CMIS manager needs to be skipped if a platform wants to. The changes are as per the HLD:-
sonic-net/SONiC#971

How I did it

Platform provides input to supervisord via pmon_daemon_control.json to pass "skip_cmis_mgr" argument to Xcvrd while launching it

How to verify it

Use SN2700 platform to verify and ensure CMIS manager is not running.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Link to config_db schema for YANG module changes

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

@prgeor prgeor requested a review from lguohan as a code owner May 23, 2022 21:54
@prgeor prgeor force-pushed the xcvrd-skip-cmis branch from dab6068 to 183fa05 Compare May 23, 2022 21:55
lguohan
lguohan reviewed May 30, 2022
View reviewed changes
dockers/docker-platform-monitor/docker-pmon.supervisord.conf.j2 Outdated
@@ -96,9 +96,9 @@ dependent_startup_wait_for=rsyslogd:running
{% if not skip_xcvrd %}
[program:xcvrd]
{% if delay_xcvrd %}
command=bash -c "sleep 30 && {% if API_VERSION == 3 and 'xcvrd' not in python2_daemons %}python3 {% else %} python2 {% endif %}/usr/local/bin/xcvrd"
command={% if skip_cmis_mgr %} bash -c "sleep 30 && python3 /usr/local/bin/xcvrd --skip_cmis_mgr" {% else %} bash -c "sleep 30 && python3 /usr/local/bin/xcvrd" {% endif % }
Copy link
Collaborator

Choose a reason for hiding this comment

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

the code refactor seems not having same criteria as previous one. can you explain why skip_cmis_mgr can replace python3 check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan platform-common and platform-daemon has moved to using python3. We no longer support python2

Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to skip cmis_mgr for sn2700? we are not going to support cmis for sn2700 in the future?

@prgeor
Copy link
Contributor Author

prgeor commented May 30, 2022

@keboliu can you review?

@prgeor prgeor requested a review from keboliu May 30, 2022 18:59
@liushilongbuaa
Copy link
Contributor

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor force-pushed the xcvrd-skip-cmis branch from a1f28a3 to 28deae5 Compare June 10, 2022 19:37
@prgeor prgeor force-pushed the xcvrd-skip-cmis branch from 28deae5 to f75a831 Compare June 12, 2022 16:15
@keboliu
Copy link
Collaborator

keboliu commented Jun 13, 2022

btw: in the design (https://github.com/sonic-net/SONiC/pull/971/files), the name of the flag is: skip_xcvrd_cmis_mgr, can you please align the name with the design?

@prgeor
Copy link
Contributor Author

prgeor commented Jun 13, 2022

btw: in the design (https://github.com/sonic-net/SONiC/pull/971/files), the name of the flag is: skip_xcvrd_cmis_mgr, can you please align the name with the design?

@keboliu please check

@prgeor
Copy link
Contributor Author

prgeor commented Jun 20, 2022

@shyam77git @jaganbal-a could you review?

@jaganbal-a
Copy link
Contributor

Unit test log:
PID: Cisco-88_LC_36FH_M_O
the below ps command shows the xcvrd is spawned with arg --skip_cmis_mgr when skip_xcvrd_cmis_mgr is set to true in respective pmon_daemon_control.json file. Also copy pasted the supervisord.conf from pmon docker.
root@sonic:/# ps -ef
UID PID PPID C STIME TTY TIME CMD
root 1 0 0 19:58 pts/0 00:00:01 /usr/bin/python3 /usr/local/bin/supervisord
root 20 1 0 19:58 pts/0 00:00:00 python3 /usr/bin/supervisor-proc-exit-listener --container-name pmon
root 28 1 0 19:58 pts/0 00:00:00 /usr/sbin/rsyslogd -n -iNONE
root 33 1 1 19:58 pts/0 00:00:08 /usr/bin/python3 /usr/local/bin/chassisd
root 36 1 1 19:58 pts/0 00:00:07 python3 /usr/local/bin/xcvrd --skip_cmis_mgr
root 37 1 1 19:58 pts/0 00:00:08 python3 /usr/local/bin/psud
root 38 1 0 19:58 pts/0 00:00:03 python3 /usr/local/bin/syseepromd
root 40 1 1 19:58 pts/0 00:00:07 python3 /usr/local/bin/thermalctld
root 41 1 0 19:58 pts/0 00:00:03 /usr/bin/python3 /usr/local/bin/pcied
root 80 40 0 19:59 pts/0 00:00:03 python3 /usr/local/bin/thermalctld
root 136 36 0 20:00 pts/0 00:00:00 [python3]

supervisord.conf file snippet in pmon docker

[program:xcvrd]
command= python3 /usr/local/bin/xcvrd --skip_cmis_mgr
priority=6
autostart=false
autorestart=unexpected
stdout_logfile=syslog
stderr_logfile=syslog
startsecs=10
dependent_startup=true
dependent_startup_wait_for=rsyslogd:running

jaganbal-a
jaganbal-a approved these changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@jaganbal-a jaganbal-a left a comment

Choose a reason for hiding this comment

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

unit test log added as comment

shyam77git
shyam77git reviewed Jun 22, 2022
View reviewed changes
dockers/docker-platform-monitor/docker-pmon.supervisord.conf.j2
@@ -96,9 +96,11 @@ dependent_startup_wait_for=rsyslogd:running
{% if not skip_xcvrd %}
[program:xcvrd]
{% if delay_xcvrd %}
command=bash -c "sleep 30 && {% if API_VERSION == 3 and 'xcvrd' not in python2_daemons %}python3 {% else %} python2 {% endif %}/usr/local/bin/xcvrd"
command={% if skip_xcvrd_cmis_mgr %} bash -c "sleep 30 && python3 /usr/local/bin/xcvrd --skip_cmis_mgr" {% else %} bash -c "sleep 30 && python3 /usr/local/bin/xcvrd" {% endif %}

Copy link
Contributor

Choose a reason for hiding this comment

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

@prgeor
skip_xcvrd_cmis_mgr is set in platform_daemon_control.json file and in turn used by supervisord to spawn xcvrd with --skip_cmis_mgr as an arg.
So, better to make the variable name same at both places (producer/setter and consumer)

@prgeor prgeor merged commit 11ff80b into sonic-net:master Jun 22, 2022
yxieca pushed a commit that referenced this pull request Jul 28, 2022
@prgeor @yxieca
Skip CMIS manager (#10907)
60c0019 
* Removed unwanted changes

* Fix j2 compilation error

* Address review comment

* Add newline
skbarista pushed a commit to skbarista/sonic-buildimage that referenced this pull request Aug 17, 2022
@prgeor @skbarista
Skip CMIS manager (sonic-net#10907)
f91e383 
* Removed unwanted changes

* Fix j2 compilation error

* Address review comment

* Add newline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyam77git shyam77git shyam77git left review comments

@jaganbal-a jaganbal-a jaganbal-a approved these changes

@keboliu keboliu keboliu approved these changes

@lguohan lguohan lguohan approved these changes

Assignees
No one assigned
Labels
Chassis 🤖 Modular chassis support Included in 202205 Branch Request for 202205 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@prgeor @liushilongbuaa @keboliu @jaganbal-a @lguohan @shyam77git @yxieca @abdosi