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

Refresh session when within the last 10% of ticket params validity period #1852

Closed
yondonfu opened this issue Apr 27, 2021 · 7 comments · Fixed by #1877
Closed

Refresh session when within the last 10% of ticket params validity period #1852

yondonfu opened this issue Apr 27, 2021 · 7 comments · Fixed by #1877
Assignees

Comments

@yondonfu
Copy link
Member

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

At the moment, B will refresh a session (i.e. call GetOrchestrator on O to get a fresh OrchestratorInfo message) if the O's ticket params are expired. However, ticket params can expire while a segment is in flight to O. If this happens, the O will return a TicketParams expired error when the segment is received and B will temporarily suspend O for returning an error.

Describe the solution you'd like
A clear and concise description of what you want to happen.

B should refresh a session if O's ticket params are almost expired. B refreshes a session if O's auth token is within 10% of the end of its validity period. We can do something similar with ticket params where B refreshes a session if O's ticket params are within 10% of the end of its validity period.

@kyriediculous
Copy link
Contributor

kyriediculous commented Apr 29, 2021

Are there cases of this actually happening ?

Because currently the orchestrator would send updated ticket params with a new expiry based on its last seen block with every segment. The expiration is currently set at 5 blocks so for this to occur the segment in flight would have to be longer than 5 ethereum blocks or 1 minute or the orchestrator has a stale view of the last seen block (in which case refreshing doesn't help) or a combination of both.

@yondonfu
Copy link
Member Author

Hard to say for certain, but I have observed a number of TicketParams expired errors being returned by various Os on the network at a regular frequency. So, in each of those cases, the O that returns the TicketParams expired would be temporarily suspended from selection by the O causing unnecessary churn in B's session list for a stream.

The expiration is currently set at 5 blocks so for this to occur the segment in flight would have to be longer than 5 ethereum blocks or 1 minute or the orchestrator has a stale view of the last seen block (in which case refreshing doesn't help) or a combination of both.

If a B receives TicketParams from a set of Os and then switches to one of the Os within the 5 block validity period for the TicketParams then there is a greater chance that the TicketParams expire while a segment is in flight.

It is possible that a solution for this issue would not completely address these TicketParams expired errors, but I believe that it would at least help avoid possible edge cases where TicketParams expire while a segment is in flight. If we implement this and still observe those errors consistently, we can look into additional solutions (for example, increasing the TicketParams validity period to relax the block synchronization requirements b/w B & O).

@kyriediculous
Copy link
Contributor

kyriediculous commented Apr 29, 2021

It seems that there could actually be to distinguishable issues here then and the solution for each might be different.

  1. TicketParams expire for the current orchestrator

    This would occur if either:

    • the segment in flight is longer than the remaining expiration. This would have a higher chance of happening when the orchestrator's view of the last block is behind by a couple. The minimum window would be 1 block and the time between receiving the last response and the response for the current segment in flight would have to exceed the duration of a block
    • the orchestrator's view of the last seen block falls behind further than the expiration while the segment is in flight (and thus the newly returned ticket params in the response will have expired)

    It seems like both of these issues could simply be addressed by a longer expiration time, loosening synchronisation requirements

  2. The TicketParams for other selected orchestrators (sessions) expire, this would require an HTTP request when the B has to fail over to get updated ticket params

    At the current expiration time this would send 1 HTTP request to each orchestrator selected (session) every minute.
    Since there's some discussion to increase the initial number of sessions to work with, I'm not sure I'm comfortable with that amount of additional requests.

    While increasing the expiration would alleviate this linearly , I'm wondering whether that's enough. A longer expiration time by itself might also be sufficient here because that gives the B more time to fail over at the start of a stream if necessary without having to make additional HTTP requests for updated ticket params. In the case it has to fail over somewhere in the middle of a stream then it would have to make a single HTTP request to the new orchestrator, which might be okay.

@yondonfu
Copy link
Member Author

yondonfu commented May 3, 2021

Agreed on the following points:

  • Extending the TicketParams validity period can reduce the # of refresh requests that B has to send especially in the scenario where B & O have different views of the latest block number
  • Extending the TicketParams validity period can extend the period of time after the start of a stream during which B can failover to an O without needing to send a refresh request to that O. For example, B can failover in the first 1 min of a stream without an additional refresh request and with a validity period of 10 min B can failover in the first 10 min of a stream

With all that said, I think the extension of the TicketParams validity period can be considered in a separate issue especially since this would be a change on O.

The OP describes a change on B. Specifically, the OP describes a way for B to avoid the edge case where it believes the TicketParams are expired and then the TicketParams expire while a segment is in flight. Even if the TicketParams validity period is extended, this edge case is possible. We send a refresh request for an auth token when it is within 10% of the end of its validity period for a similar reason.

@kyriediculous
Copy link
Contributor

kyriediculous commented May 4, 2021

We only refresh the auth token for the session that is being actively used. We already refresh ticket params for the actively used session on a per segment basis. This behaviour is already similar , and even better, the ticketparams are refreshed more often (each segment) rather than 10% before expiry of a timer.

Since we do not actively refresh auth tokens for sessions that are not transcoding I don't deem a change necessary on the B side here.

In addition more concretely, since the expiration is currently at 5 blocks and 10% would be less than a block we would essentially be refreshing every 3 blocks on top of receiving new ticket params with each segment. I think this is too often so I would first do a longer expiry period and see if the edge case still occurs with what is considered normal length segments.

@yondonfu
Copy link
Member Author

yondonfu commented May 4, 2021

We only refresh the auth token for the session that is being actively used.

We would also only be sending a refresh request for the current session that is being used if the associated TicketParams are close to expiring similar to what we do for auth tokens.

Since we do not actively refresh auth tokens for sessions that are not transcoding

See above.

I think this is too often so I would first do a longer expiry period

Sure. Can you open an issue for that? I think tackling that first is reasonable given your point on how short the TicketParams validity period is right now.

@kyriediculous
Copy link
Contributor

Not sure if #1877 is along the lines of what you had in mind.

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 a pull request may close this issue.

2 participants