-
Notifications
You must be signed in to change notification settings - Fork 201
BYOC #3727
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
base: master
Are you sure you want to change the base?
BYOC #3727
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3727 +/- ##
===================================================
+ Coverage 31.94301% 33.37411% +1.43110%
===================================================
Files 158 159 +1
Lines 47519 49050 +1531
===================================================
+ Hits 15179 16370 +1191
- Misses 31437 31633 +196
- Partials 903 1047 +144
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
fdc7fc4
to
34e53b9
Compare
} | ||
stopJob.sign() //no changes to make, sign job | ||
|
||
token, err := sessionToToken(params.liveParams.sess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets check/log this err here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still hoping we can check and log this err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an error check and log in d73fe37. Note that this function cannot return an error currently, should I just update to only return the JobToken. I think I added an error here thinking as I built it out something could cause an error possibly.
e862a20
to
33fda51
Compare
…able live payments
769b8e6
to
55666ef
Compare
} | ||
stopJob.sign() //no changes to make, sign job | ||
|
||
token, err := sessionToToken(params.liveParams.sess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still hoping we can check and log this err
server/job_stream.go
Outdated
|
||
// fetch new JobToken with each payment | ||
// update the session for the LivePipeline with new token | ||
newToken, err := getToken(ctx, 3*time.Second, token.ServiceAddr, stream.Pipeline, jobSender.Addr, jobSender.Sig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this 3 second param a constant, say TokenRefreshTimeout = 3 * time.second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it would be good to have a few retries here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server/job_stream.go
Outdated
clog.Infof(ctx, "Starting stream processing") | ||
//refresh the token if not first Orch to confirm capacity and new ticket params | ||
if firstProcessed { | ||
newToken, err := getToken(ctx, 3*time.Second, orch.ServiceAddr, gatewayJob.Job.Req.Capability, gatewayJob.Job.Req.Sender, gatewayJob.Job.Req.Sig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment RE: constant for timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in bfb0135
server/job_stream.go
Outdated
|
||
// Setup request body to be able to preserve for retries | ||
// Read the entire body first with 10MB limit | ||
bodyBytes, err := io.ReadAll(io.LimitReader(r.Body, 10<<20)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since io.ReadALL truncates to 10MB here, maybe we want to log that?
bodyBytes, err := io.ReadAll(io.LimitReader(r.Body, 10<<20)) | |
if len(bodyBytes) > 10<<20 { | |
fmt.Errorf("request body too large, maximum size is %d bytes", 10<<20) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated how the r.Body is limited to 10MB so that can get a specific error in 67cb9cd. Looked like from docs that limit reader would just stop reading and provide no error.
clog.V(common.DEBUG).Infof(ctx, "job price=%v units=%v", jobPrice.PricePerUnit, jobPrice.PixelsPerUnit) | ||
|
||
//no payment included, confirm if balance remains | ||
jobPriceRat := big.NewRat(jobPrice.PricePerUnit, jobPrice.PixelsPerUnit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should extract this payment logic here into another method(s), a bit hard to reason about this with so all the nested conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, Adding test(s) around that new method would be fantastic!
|
||
jobPriceRat := big.NewRat(orchJob.JobPrice.PricePerUnit, orchJob.JobPrice.PixelsPerUnit) | ||
if jobPriceRat.Cmp(big.NewRat(0, 1)) > 0 { | ||
h.orchestrator.DebitFees(orchJob.Sender, core.ManifestID(orchJob.Req.Capability), orchJob.JobPrice, int64(pmtCheckDur.Seconds())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This concerns me a bit as it seems like a couple things could happen here since these two operations ( debit and check ) are not one atomic operation
Between the debit and balance check, other goroutines can create race conditions by:
- Crediting the account (incoming payments)
- Debit the account (other streams)
- Change the balance state(maybe somehow?)
I think we can pull these into another method debit and check and using a mutex lock to prevent this
stream, err := h.node.ExternalCapabilities.AddStream(orchJob.Req.ID, orchJob.Req.Capability, reqBodyBytes) | ||
if err != nil { | ||
clog.Errorf(ctx, "Error adding stream to external capabilities: %v", err) | ||
respondWithError(w, "Error adding stream to external capabilities", http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be calling RemoveStream here in case of error?
return false | ||
} | ||
|
||
if sd.controlChannel == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should/need to acquire the mutex sd.sdm.lock() here before reading or there is a possiblility of reading nil, old, or partial data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d73fe37
<-ctx.Done() | ||
|
||
//orchestrator channels shutdown | ||
if stream.pubChannel != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add some error handling here just in case we call close() twice ( even though adding locks to the other channel operations should prevent this )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added error if stmt to catch error and log it in d73fe37
delete(ls.LivepeerNode.LivePipelines, streamId) | ||
return | ||
case <-pmtTicker.C: | ||
if !params.inputStreamExists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: suggest breaking this case out into a new method
core/ai_orchestrator.go
Outdated
//ensure price numerator and denominator can be int64 | ||
jobPrice, err = common.PriceToInt64(jobPrice) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("invalid job price: %w", err)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in d73fe37
* Add SDXL and SDXL FaceID streamdiffusion images Co-authored-by: victorgelias <[email protected]> * Refactor: Clarify docker image descriptions Co-authored-by: victorgelias <[email protected]> --------- Co-authored-by: Cursor Agent <[email protected]>
…gateway->sendPaymentForStream
…5 segment history
ddf2612
to
1ca67c4
Compare
What does this pull request do? Explain your changes. (required)
Adds configurable streaming for BYOC entrypoint to go-livepeer. Uses trickle protocol to handle streaming for similar entrypoints and outputs from go-livepeer as live-video-to-video.
Streams can be any or a mix of the following:
Control and Events channels are created for every stream.
Streams are created with a POST request to
/ai/stream/start
that will start the stream and reserve the capacity with an Orchestrator that is providing the BYOC capability. If video ingress is enabled, the client should then start a stream with WHIP or RTMP to the provided ingress URLs provided in the response. URLs for egress video, data, updates (control) and events are also included in the response as well as the stream_id. The stream_id is an integral part of the URLs provided to interact with the stream and is combined with a provided stream name in the /ai/stream/start request.Streams are stopped with a POST request to
/ai/stream/stop
. Orchestrators and Gateways track payment balance and the Gateway adjusts to the Orchestrators provided balance in new JobTokens provided at each payment interval every minute. Orchestrators will shutdown a stream when payment balance is zero.Specific updates (required)
job_stream.go
andjob_stream_test.go
job_rpc.go
to reuse stream setup where made sensecommon/testutil.go
.How did you test each of these updates (required)
Used
byoc-stream
to test end to end: https://github.com/ad-astra-video/livepeer-app-pipelines/tree/main/byoc-streamAdded tests to
job_stream_test.go
and some additional tests tojob_rpc_test.go
.Does this pull request close any open issues?
Checklist:
make
runs successfully./test.sh
pass