server: Skip redundant maxPrice check in ongoing session #2994
Conversation
When I was writing the tests for validatePrice I found out we were using that function wrong in a couple places and never checking any error. We were sending err.Error() to check the error from err.
victorges
force-pushed
the
vg/fix/max-price-check
branch
from
12238b9
to
73fd824
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2994 +/- ##
===================================================
- Coverage 57.47690% 57.44328% -0.03362%
===================================================
Files 92 92
Lines 15695 15692 -3
===================================================
- Hits 9021 9014 -7
- Misses 6078 6080 +2
- Partials 596 598 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
For this change, I validated the discovery loop still selects orchestrator via price. In testing this change allowed an orchestrator that is specified by the broadcaster with Do we really need the If we need to keep price validation, perhaps it can be moved to when the session is created? |
|
Hey @eliteprox, thank you for reviewing and testing this further! I agree that it should be fine to allow overriding the price for an O to sth higher than the max, given it's an explicit override. Also had seen your PR and thought that the I believe the spirit of the original fix though (#2892), was to make the prices sticky for a given session on the O side, so they would never return a different price for a given session. While we have updated the In the meantime, a solution we are evaluating is to allow some price change on the O until we figure out what is making the price to get updated for a running O session. My current suspicion is this, which updates the Then for the fix, we could merge this PR (#2994), and temporarily merge #2995 until we make the prices actually stick during a session. There will be a risk of dropping sessions until we do that, but it'd be much lower given the 2x increase threshold. Then we can go on with the migration to USD prices in the meantime. Let me know if you have any thoughts! |
The broadcaster seems to be refreshing the pricing info on every segment when generating tickets. I think it will be safer and simpler to use go-livepeer/server/segment_rpc.go Lines 761 to 768 in 73fd824 If we use
As for removing the maximum price check here, I'm also in support of that because it is already validated when the session begins. If the price should remain the same for the entire session, it makes sense to not update or validate it on each segment. I much prefer this approach over #2995 and I think it is more secure/reliable overall. I opened #2996 to track the issue since we have a few solutions out there. |
|
@eliteprox thanks for the further input. Unfortunately it shouldn't be so simple as sending a different price on the B side, since the problem is exactly that the O is updating the current price for the session while it is running. On that case, it will naturally reject segments whose price doesn't match its internal state (otherwise the B could just choose to pay "zero" for every segment). So the correct fix here, given we want to have fixed prices for a running session, is to correct the O logic that is losing the current price for the session.
FTR the As for the other issue, we cannot remove the price-change validation altogether otherwise Os could increase their price indefinitely in the middle of a session (B risk), while we also can't just set any price on the B otherwise the Os that would be at risk (since they'd need to stop validating the segment prices, as explained above). So, the ideal solution is to fix the O behavior not to lose the fixed price in the middle of a session. We are carrying on the investigation about that (all help appreciated!), but in the meantime we're deploying this and #2995 to allow the prices to change based on USD quote (up to 2x since the session started, for O prices). This will stop dropping most of the sessions (ETH price would need to drop 2x in 1h) until we figure out why the prices are changing mid-session. |
What does this pull request do? Explain your changes. (required)
After #2981, we started allowing both Bs and Os to updated their prices dynamically over time
based on an external currency. This creates a problem for Bs, given their
maxPricecouldbecome lower than an Os
pricein the middle of a session. Even though we want the price tobe dynamic, we want a given session prices to be negotiated in the beginning of the session,
and don't want to drop a session mid-transcoding because the prices were updated.
The initial intuition was to save the "initial max price" on the
BroadcastSessionobject as well,but we realized that this logic would be redundant given we already check for the max price when
the session is estabilished in the first place, and disallow the O to increase prices mid-session.
It goes something like this:
O price <= B maxPriceinitialPrice, and would save B price asinitialMaxPriceO initialPrice <= B initialMaxPriceO price <= O initialPrice(kept this check on the samevalidatePricefunc)O price <= O initialPrice <= B initialMaxPriceO price <= B initialMaxPrice, which means we don't need a separate check for that (nor even saving theinitialMaxPrice, as that would be its only use)Specific updates (required)
maxPricecheck fromvalidatePriceHow did you test each of these updates (required)
Ran automated tests.
Does this pull request close any open issues?
Related to ENG-1855 but a bit parallel to that
Checklist:
makeruns successfully./test.shpass