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

Add automatic session limit and set max sessions with livepeer_cli #2781

Merged
merged 28 commits into from Sep 1, 2023

Conversation

eliteprox
Copy link
Contributor

@eliteprox eliteprox commented Apr 4, 2023

What does this pull request do? Explain your changes. (required)

Defines a new "auto session limit" for orchestrators based on the combined remote transcoder capacity that can be set by using -maxSessions auto or the livepeer_cli option 21.

Specific updates (required)

  • Added -maxSessions auto as an option for orchestators.
  • Added "Set max sessions" option to livepeer_cli as an option for orchestrators to change the session limit without restarting

How did you test each of these updates (required)

  • Observed livepeer_max_sessions_total while adding and removing transcoders with -maxSessions auto
  • Observed livepeer_max_sessions_total while using Option 21 Set Max Sessions in livepeer_cli to various numbers and 'auto'
  • Verified other roles like -broadcaster and -transcoder are not affected by the session limit changes

Does this pull request close any open issues?
Fixes #1935

Checklist:

@eliteprox eliteprox changed the title Add setMaxSessions handlers and livepeer_cli options Add setMaxSessions handler and livepeer_cli options Apr 4, 2023
@eliteprox eliteprox changed the title Add setMaxSessions handler and livepeer_cli options Add setMaxSessions handler and livepeer_cli option Apr 4, 2023
@eliteprox eliteprox marked this pull request as draft April 4, 2023 01:59
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #2781 (e626817) into master (a231510) will decrease coverage by 0.20348%.
Report is 3 commits behind head on master.
The diff coverage is 5.82524%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2781         +/-   ##
===================================================
- Coverage   56.70668%   56.50320%   -0.20348%     
===================================================
  Files             88          88                 
  Lines          19160       19229         +69     
===================================================
  Hits           10865       10865                 
- Misses          7699        7765         +66     
- Partials         596         599          +3     
Files Changed Coverage Δ
clog/clog.go 59.87261% <ø> (ø)
cmd/livepeer/starter/starter.go 4.22907% <0.00000%> (-0.03381%) ⬇️
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
cmd/livepeer_cli/wizard_bond.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_rounds.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
core/livepeernode.go 60.00000% <0.00000%> (-18.00000%) ⬇️
server/handlers.go 53.07018% <5.55556%> (-0.76226%) ⬇️
core/orchestrator.go 77.71664% <25.00000%> (-0.61669%) ⬇️
... and 3 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5af79b0...e626817. Read the comment docs.

Files Changed Coverage Δ
clog/clog.go 59.87261% <ø> (ø)
cmd/livepeer/starter/starter.go 4.22907% <0.00000%> (-0.03381%) ⬇️
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
cmd/livepeer_cli/wizard_bond.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_rounds.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
core/livepeernode.go 60.00000% <0.00000%> (-18.00000%) ⬇️
server/handlers.go 53.07018% <5.55556%> (-0.76226%) ⬇️
core/orchestrator.go 77.71664% <25.00000%> (-0.61669%) ⬇️
... and 3 more

... and 1 file with indirect coverage changes

@eliteprox eliteprox changed the title Add setMaxSessions handler and livepeer_cli option Add -MaxSessions auto, add Max Sessions to livepeer_cli Apr 4, 2023
@cyberj0g
Copy link
Contributor

cyberj0g commented Apr 4, 2023

Great work! A small note on auto value for maxSessions. I think it's a bit misleading. For O, this parameter adds an explicit restriction on top of already automatic capacity management. Maybe we should just have value of 0 (no restrictions) by default, instead of adding auto value? On the other hand, it would be really useful, if T could automatically detect max number of sessions, as part of startup sequence, where we already testing for available capabilities. The code could be reused from livepeer_bench utility. Maybe track is as a separate issue?

@eliteprox
Copy link
Contributor Author

Maybe we should just have value of 0 (no restrictions) by default, instead of adding auto value?

I agree we should do this, I considered it when coding because the type conversions from string to int are a bit of overhead.

I think we can apply a "default session limit" of 0 sessions for orchestrators, and make "0" the indicator for auto. For broadcasters and transcoders the default limit would remain at 10. I can make this change, would you agree?

On the other hand, it would be really useful, if T could automatically detect max number of sessions, as part of startup sequence, where we already testing for available capabilities.

I agree, this is something we could build on a separate issue. I thought of making it so that transcoders can change their session limits while running, but it seemed problematic because the auto session limit on the orchestrator would not update accordingly until the transcoder re-connected.

@eliteprox eliteprox changed the title Add -MaxSessions auto, add Max Sessions to livepeer_cli Add automatic session limit and support for setting it with livepeer_cli Apr 27, 2023
@eliteprox eliteprox changed the title Add automatic session limit and support for setting it with livepeer_cli Add automatic session limit and set max sessions with livepeer_cli Apr 27, 2023
@eliteprox eliteprox marked this pull request as ready for review April 27, 2023 06:36
@eliteprox eliteprox marked this pull request as draft April 27, 2023 07:05
@eliteprox eliteprox marked this pull request as ready for review April 27, 2023 07:12
@chrishobcroft
Copy link
Contributor

Please can you link the issue that this responds to?

@eliteprox
Copy link
Contributor Author

Please can you link the issue that this responds to?

#1935 Thanks for your review and feedback :)

@leszko
Copy link
Contributor

leszko commented May 23, 2023

@eliteprox I'm catching up with PRs, is this PR ready for review?

If yes, then please fix the CI builds failures, update the branch, and assign me as reviewer. Thanks!

cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
core/orchestrator.go Show resolved Hide resolved
@eliteprox
Copy link
Contributor Author

How about being more explicit and having a value --maxSessions auto? Otherwise, I'm afraid of not keeping the backwards-compatibility for all cases.

I agree, I will revert this back to -maxSessions auto and make it so that maxSessions defaults to 10

… add message for maxSessions auto and combined O/T
@eliteprox
Copy link
Contributor Author

Tests are passing on local test suite

@eliteprox eliteprox requested a review from leszko August 30, 2023 07:49
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added some final nit comments. Other than that it looks good 👍

Thanks for the PR.

cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
cmd/livepeer_cli/wizard_transcoder.go Outdated Show resolved Hide resolved
core/orchestrator.go Show resolved Hide resolved
@leszko leszko merged commit 72ef55c into livepeer:master Sep 1, 2023
15 of 16 checks passed
eliteprox added a commit to eliteprox/go-livepeer that referenced this pull request Feb 21, 2024
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.

Enable -maxSessions to be changed without requiring restart
5 participants