Skip to content

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

Merged
vdahiya12 merged 8 commits intosonic-net:masterfrom
vdahiya12:support-ycable
Jan 25, 2022
Merged

[y_cable] Support for initialization of new daemon ycable to support ycables#9125
vdahiya12 merged 8 commits intosonic-net:masterfrom
vdahiya12:support-ycable

Conversation

@vdahiya12
Copy link
Copy Markdown
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
Copy Markdown
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.

{% endif %}

{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}
{% if not skip_xcvrd %}
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12
Copy link
Copy Markdown
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>
@vdahiya12 vdahiya12 requested a review from sujinmkang as a code owner January 19, 2022 02:23
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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