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.

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

[y_cable] Support for initialization of new daemon ycable to support ycables #9125

Merged
merged 8 commits into from
Jan 25, 2022

Conversation

vdahiya12
Copy link
Contributor

@vdahiya12 vdahiya12 commented Oct 29, 2021

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

This PR also adds the commit in sonic-platform-daemons

94fa239 [y_cable] refactor y_cable to a seperate logic and new daemon from xcvrd (#219)

Why I did it

This PR separates the logic of Y-Cable from xcvrd. Before this change we were utilizing xcvrd daemon to control all aspects of Y-Cable right from initialization to processing requests from other entities like orch,linkmgr.
Now we would have another daemon ycabled which will serve this purpose.
Logically everything still remains the same from the perspective of other daemons.
it also take care aspects like init/delete daemon from Y-Cable perspective.

How I did it

To serve the purpose we build a new wheel sonic_ycabled-1.0-py3-none-any.whl and install it inside pmon.
We also initalize the daemon ycabled which serves our purpose for refactor inside pmon

How to verify it

Ran the changes with an image for dualtor tests on a 7050cx3 platform

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

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

ycables

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

@vdahiya12 can we rename the daemon to 'ycabled' just to have uniformity with other processes running in PMON? Also added other comments.

rules/docker-platform-monitor.mk Outdated Show resolved Hide resolved
@@ -108,6 +108,25 @@ dependent_startup=true
dependent_startup_wait_for=rsyslogd:running
{% endif %}

{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}
{% if not skip_xcvrd %}
Copy link
Contributor

Choose a reason for hiding this comment

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

why this check 'skip_xcvrd'? how is this new daemon dependent on xcvrd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should have a separate flag for skip_ycabled, to keep homogeneous with other daemons

{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}
{% if not skip_xcvrd %}
[program:ycable]
{% if delay_xcvrd %}
Copy link
Contributor

Choose a reason for hiding this comment

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

why delay start of ycable daemon based upon 'delay_xcvrd'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should have a separate flag for delay_ycabled

{% else %}
command=nice -n -20 {% if API_VERSION == 3 and 'ycable' not in python2_daemons %}python3 {% else %} python2 {% endif %}/usr/local/bin/ycable
{% endif %}
priority=6
Copy link
Contributor

Choose a reason for hiding this comment

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

you seem to be reusing the 'priority' level of xcvrd. is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking about it, all daemons have a priority from 1-10. Maybe need to have this as 7 ?

Comment on lines 4 to 10

SONIC_YCABLE_PY2 = sonic_ycable-1.0-py2-none-any.whl
$(SONIC_YCABLE_PY2)_SRC_PATH = $(SRC_PATH)/sonic-platform-daemons/sonic-ycable
$(SONIC_YCABLE_PY2)_DEPENDS = $(SONIC_PY_COMMON_PY2) $(SONIC_PLATFORM_COMMON_PY2)
$(SONIC_YCABLE_PY2)_DEBS_DEPENDS = $(LIBSWSSCOMMON) $(PYTHON_SWSSCOMMON)
$(SONIC_YCABLE_PY2)_PYTHON_VERSION = 2
SONIC_PYTHON_WHEELS += $(SONIC_YCABLE_PY2)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are moving to PY3 only, then why PY2 support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

rules/sonic-ycable.dep Outdated Show resolved Hide resolved
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12
Copy link
Contributor Author

Dependent on this sonic-net/sonic-platform-daemons#219 and module update

prgeor
prgeor previously approved these changes Dec 1, 2021
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
we add this commit in sonic-platform-daemons
94fa239 [y_cable] refactor y_cable to a seperate logic and new daemon from xcvrd (sonic-net#219)

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 requested a review from prgeor January 24, 2022 05:14
Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

i think you haven't tested the 'delay_ycabled' == true case?

{% if not skip_ycabled %}
[program:ycabled]
{% if delay_ycabled %}
command=bash -c "sleep 30 nice -n -20 python3 /usr/local/bin/ycabled"
Copy link
Contributor

Choose a reason for hiding this comment

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

'&&' missing between sleep and nice ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank You for the catch, yes fixed now

@prgeor prgeor self-assigned this Jan 24, 2022
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 requested a review from prgeor January 25, 2022 06:15
@vdahiya12 vdahiya12 merged commit 61e9a76 into sonic-net:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants