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

[Celestica] Make 'sfputil show presence' 10x faster #18041

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dal00
Copy link

@dal00 dal00 commented Feb 5, 2024

Why I did it

The is_host function is used when enumerating SFPs which was very slow when detecting if it was running inside docker. The following call makes the detection 10x faster.

How I did it

Instead of executing docker check for /.dockerenv which is present when inside a container.

How to verify it

Before patch:

  $ time sudo sfputil show presence
  Port         Presence
  -----------  -----------
  Ethernet0    Present
  Ethernet1    Present
  Ethernet2    Present

  -- snip --

  real    0m13.521s
  user    0m10.239s
  sys     0m4.104s

After patch:

  $ time sudo sfputil show presence
  Port         Presence
  -----------  -----------
  Ethernet0    Present
  Ethernet1    Present
  Ethernet2    Present

  -- snip --

  real    0m2.133s
  user    0m1.461s
  sys     0m0.347s

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

  • 202211

Description for the changelog

n/a

Link to config_db schema for YANG module changes

n/a

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

image

The is_host function is used when enumerating SFPs which was very slow
when detecting if it was running inside docker. The following call makes
the detection 10x faster.

Before patch:
  $ time sudo sfputil show presence
  Port         Presence
  -----------  -----------
  Ethernet0    Present
  Ethernet1    Present
  Ethernet2    Present

  -- snip --

  real    0m13.521s
  user    0m10.239s
  sys     0m4.104s

After patch:
  $ time sudo sfputil show presence
  Port         Presence
  -----------  -----------
  Ethernet0    Present
  Ethernet1    Present
  Ethernet2    Present

  -- snip --

  real    0m2.133s
  user    0m1.461s
  sys     0m0.347s

Signed-off-by: Tommy Nevtelen <tommy@nevtelen.com>
Copy link

linux-foundation-easycla bot commented Feb 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dal00 / name: Tommy Nevtelen (721fa21)

@dal00 dal00 requested a review from lguohan as a code owner February 5, 2024 19:41
@dal00 dal00 changed the title [Celestica] Speed up docker detection [Celestica] Make 'sfputil show presence' faster Feb 19, 2024
@dal00
Copy link
Author

dal00 commented Apr 12, 2024

@lguohan Is there still something needed before this can be reviewed?

@bluecmd
Copy link
Contributor

bluecmd commented Oct 27, 2024

Ping @lguohan - would be nice to get into the 202411 release if possible.

@bluecmd
Copy link
Contributor

bluecmd commented Nov 19, 2024

Ping @lguohan - one last attempt before the 202411 release fork happens!

@dal00 dal00 changed the title [Celestica] Make 'sfputil show presence' faster [Celestica] Make 'sfputil show presence' 10x faster Nov 19, 2024
@bradh352
Copy link
Contributor

bradh352 commented Dec 24, 2024

The only thing I can think of why they haven't merged this is maybe they don't like this change in how the check works for fear issues without Celestica's approval?

Technically the more proper thing to do is look at /proc/1/cgroup and if it's "/" then its the host, otherwise you're running in a container of some sort. But though more proper its probably not as easy to accept as keeping the check the same.

Perhaps more acceptable would be to cache the result instead inside of __init__, so you're actually still using the same container check. Probably rename is_host() to _is_host() to fetch the result and create a new is_host() that just returns the cached variable.

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.

4 participants