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

Maximum duration 86400 in SessionInfo cannot support 'Extend Session Duration' #272

Closed
emil-cheung opened this issue Mar 11, 2024 · 7 comments · Fixed by #277
Closed

Maximum duration 86400 in SessionInfo cannot support 'Extend Session Duration' #272

emil-cheung opened this issue Mar 11, 2024 · 7 comments · Fixed by #277
Assignees

Comments

@emil-cheung
Copy link
Collaborator

emil-cheung commented Mar 11, 2024

Problem description
QoD has introduced 'Extend Session Duration' since v0.10.0 which means that the total duration of a QoD session may exceed 86400.

The responses of:

  • GET /sessions/{sessionId}
  • POST /sessions/{sessionId}/extend

shall return the total duration of the QoD session.

However, in the datatype 'SessionInfo' used by the responses above, the max limit of 86400 remains.

Expected behavior
Remove the max limit of 86400 in datatype 'SessionInfo'.

Alternative solution
Or define another max limit larger than 86400 in datatype 'SessionInfo'.

Additional context
The documentation of 'Extend Session Duration' operation:
"Extend the duration of an active QoS session. If this operation is executed successfully, the new duration of the target session will be the original duration plus the additional duration. The remaining duration plus the requested additional duration shall not exceed the maximum limit. Otherwise, the new remaining duration will be the maximum limit."

@hdamker
Copy link
Collaborator

hdamker commented Mar 11, 2024

@emil-cheung Currently the description you have cited does not allow to extend beyond the "maximum limit" which is defined as 86400 seconds in SessionInfo:

The remaining duration plus the requested additional duration shall not exceed the maximum limit. Otherwise, the new remaining duration will be the maximum limit.

But there is another reason to remove the "86400 seconds" limit from SessionInfo, see #249. The maxDuration will be given by the QosProfile going forward (currently we have restricted maxDurationwith a comment in the description also to 86400 seconds, to avoid inconsistencies). maxDuration will be then the upper limit for a session duration and "extend session duration" shall not allow to extend beyond this limit.

Do you agree to close your issue and continue the discussion within #249?

@emil-cheung
Copy link
Collaborator Author

emil-cheung commented Mar 12, 2024

@emil-cheung Currently the description you have cited does not allow to extend beyond the "maximum limit" which is defined as 86400 seconds in SessionInfo:

The remaining duration plus the requested additional duration shall not exceed the maximum limit. Otherwise, the new remaining duration will be the maximum limit.

But there is another reason to remove the "86400 seconds" limit from SessionInfo, see #249. The maxDuration will be given by the QosProfile going forward (currently we have restricted maxDurationwith a comment in the description also to 86400 seconds, to avoid inconsistencies). maxDuration will be then the upper limit for a session duration and "extend session duration" shall not allow to extend beyond this limit.

Do you agree to close your issue and continue the discussion within #249?

@hdamker
Probably a bit more discussion before closing the issue.
Let's take one step back to clarify our understandings.

Please allow me to describe an extreme example.
Let's say a developer originally created a QoD session with duration of 86,400 seconds (already reaching the max limit).
Now, 80,000 seconds has passed and he wants to extend the duration with another 80,000 seconds.

At this very moment:

  • Elapsed time is 80,000 (s)
  • Remaining duration is 86,400 - 80,000 = 6,400 (s)
  • Requested additional duration is 80,000 (s)
  • Remaining duration + requested additional duration is 6,400 + 80,000 = 86,400 (s), which is still within the max limit.
  • Total duration is 86,400 + 80,000 = 166,400 (s), which significantly exceeds the max limit of 86,400.

My understanding was that as remaining duration + requested additional duration is 86,400 seconds, the duration extension request is acceptable. While your understanding is that since total duration is 166,400 seconds, the duration extension request is not acceptable.

I am fine with either understanding, but let's clarify which one is the expected one among the community.
If my understanding is correct, then the max limit in 'SessionInfo' should be removed;
If your understanding is correct, then the API documentation should be improved, e.g., adding additional statement about the total duration shall not exceed the max limit.

@dfischer-tech
Copy link
Contributor

Hi @emil-cheung and @hdamker,
Upon thorough review, it appears that the following formulation may be ambiguous and thus requires adjustment:

... (1) the new duration of the target session will be the original duration plus the additional duration. 
(2) The remaining duration plus the requested additional duration shall not exceed the maximum limit. Otherwise, the new remaining duration will be the maximum limit.

Paragraph 1: In this section, the new duration of the target session is the sum of the original duration and the additional duration.
Paragraph 2: Here, the new duration time is the sum of the remaining duration of the session along with the requested additional duration.

In the handling of session information, (SessionInfo), parameters such as "startedAt" and "expiredAt" are crucial. To ensure that the duration limit of a session does not exceed beyond a day, I propose integrating the logic from the first paragraph. An example of extending a session could be as follows:

var newDuration = oldDuration + additionalDuration;
var newExpiresAt = oldStartedAt + oldDuration + additionalDuration;
..
if (newDuration > SECONDS_PER_DAY) {
  newDuration = SECONDS_PER_DAY;
  newExpiresAt = oldStartedAt + SECONDS_PER_DAY;
}

The limitation of a session to one day would be maintained.

The second paragraph appears to lack coherence, as it would imply extending the duration of a session beyond a day.
I trust these adjustments better align with the basic requirements.

@hdamker
Copy link
Collaborator

hdamker commented Mar 18, 2024

@dfischer-tech I agree, the second paragraph is ambiguous. We might have had another semantic in mind when writing it.

There are two options:

(a) maxDuration (or the limit of 86400 seconds) is absolut. Also the extended session duration can't be beyond this limit.

(b) maxDuration is relative to the current time. The maxDuration will be "restarted" by extendDuration (similar to deleting the current session and creating a new one). The requirement would be in this option that "expiresAt" is not more than maxDuration into the future.

We had defined the calculation of the new duration based on the original duration (the first sentence), to avoid unclarity about the new duration.

Regarding the limit for the duration we obviously currently described the option (b). The new remaining duration shall not exceed maxDuration. But as @emil-cheung has rightfully shown, that would mean that the resulting overall duration of the session could exceed "maxDuration" (even could be unlimited extended again).

If option (a) we need to correct the second paragraph, e.g. to
The resulting duration shall not exceed the maximum duration limit. Otherwise, the new session duration will be set to the maximum duration.

If option (b) we need also make the second paragraph clearer:
The *new* remaining duration shall not exceed the maximum duration limit. Otherwise, the new remaining duration will be set to the maximum limit. The remaining duration is calculated as the 'expiresAt' minus the current time of the extendDuration API call

We need also to add examples to make it clear.

We have to decide first if we want to follow option (a) or (b). and then patch the documentation.

@emil-cheung @jlurien @eric-murray @SyeddR @RandyLevensalor - which option is fitting to your understanding and should be followed?

@RandyLevensalor
Copy link
Collaborator

(b) maxDuration is relative to the current time. The maxDuration will be "restarted" by extendDuration (similar to deleting the current session and creating a new one). The requirement would be in this option that "expiresAt" is not more than maxDuration into the future.

@hdamker I'd lean towards option b.

By treating extend duration as a new session, we can apply the same pending / approval flow as we would for a new session. We also need to ensure that we document the behavior if the session can't be extended. This also allows a service provider to implement a policy that would reject an extend session that exceeds the maxDuration of the initial session in their business logic without limiting their options.

Also, with treating extend session as new session it keeps the door open to allow for other QoD session attributes to be changed in the same manner. Either through a patch or a specific modify option, such as we are using with extend session.

@jlurien
Copy link
Collaborator

jlurien commented Mar 22, 2024

My understanding on the topic was also option (b), but I see the ambiguity in the statement. Option (b) allows more flexibility and it's probably easier to understand for developers. We may discuss if implementations could impose further limits to extension. It's somehow related to #257

@hdamker
Copy link
Collaborator

hdamker commented Mar 26, 2024

Decision from QoD Call March 22nd:

  • Option (b) will be followed, update of documentation as described in issue
  • In addition the limit of duration has to be eliminated in SessionInfo => reference to "CreateSession" has to be expanded to allow that
  • The PR for this issue will be included within the v0.10.1 patch release

Action:

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.

5 participants