Feature Request: TTL support for sandbox

[barney-s]

Add a ttl field in sandbox and automate cleanup of Pod once the TTL expires.

Reqs:

1. Allow TTL to be disabled/enabled
2. Allow TTL value to be modified after creation.

Few decisions to be made:

1. When does the TTL timer start ? sandbox create vs pod create vs pod ready ?
2. Do we delete just the Pod/Service or the Sandbox object as well ?

---

Implemented via #21

[barney-s]

Proposal

For TTL we will support these fields:

spec:
  ttl:
    shutdownAt: # absolute time stamp (Takes preference over other configs)
    # relative ttl
    seconds: 100 # TTL in seconds
    startPolicy: onCreate|onReady|onEnable|never
status:
  firstReadyTime: # timestamp when ready
  expiryTime: # when sandbox would be deleted.

When we toggle .spec.ttl.startPolicy between onEnable and never, we set and reset the .status.expiryTime. We ignore onEnable setting if .status.expiryTime is set. When we see never, we reset the status.expiryTime.

When .spec.ttl.shutdownAt is preset we ignore all other fields.

The .spec.ttl.seconds and .spec.ttl.startPolicy provides a temporally relative ttl policies.
Where as, .spec.ttl.shutdownAt provides temporally absolute ttl policy.

Disabling TTL

Proposal is to set .spec.ttl.startPolicy: never
Another option is to use an annotation agents.x-k8s.io/no-ttl-apply: true instead since it is easier to block users from setting it using a gatekeeper policy on annotations.

[barney-s]

@vicentefb FYI

[aditya-shantanu]

@barney-s - What's the intended use case of startPolicy field ?
Also wondering how this will play with warm pools because for warmpools:

1. Only relative ttl is relevant.
2. TTL should start the "count-down" at claim time.

[barney-s]

@aditya-shantanu

warmpool - a pool of sandboxes that can be pre-created.
sandbox claim described in #17 would have similar TTL.

warmpool controller creates sandbox (or sandbox-claim), it sets it to:

spec:
  ttl:
    seconds = 100 # from template
    startPolicy: disable # set by warmpool

When removed from warmpool and given (claimed), the wampool controller should set it to:

spec:
  ttl:
    seconds = 100 # prev value. no change
    startPolicy: onEnable # enabling ttl now

[vicentefb]

Few decisions to be made:

When does the TTL timer start ? sandbox create vs pod create vs pod ready ?

I think the idea of having those options available at startPolicy is good enough and it also covers the warmpool case. disable|onCreate|onReady|onEnable

Do we delete just the Pod/Service or the Sandbox object as well ?

This should also delete the SandboxClaim and the Sandbox object itself and any underlying resource such as Pod/Service etc. I don't see any reason for now to have a Podless/Serviceless Sandbox (or SandboxClaim) since that's also what the SandboxTemplate appears to be as well. If there's no Sandbox but still a SandboxClaim the reconciler would try to recreate the Sandbox if it gets deleted and that would be unexpected behavior.

Also, to confirm my understanding on status.firstReadyTime. Is this the timestamp when the timer starts counting towards expiration ?

On the other hand, for

Allow TTL value to be modified after creation.

I don't see a clear path on this specially with the introduction of startPolicy is this related to the toggle that can happen between onEnable and never. I.e when we set spec.ttl.startPolicy to never that means we modify the spec and we can modify the ttl and toggle back to onEnable so that the timer starts again and there's a new expiry time? Or is this through .spec.ttl.shutdownAt ? Not sure if this is exactly the best experience for the user to extend a ttl lease, we can start from here though, I would still be inclined into exploring the annotation approach here kubectl annotate agenticsandbox my-sandbox 'ttl.model.skippy.io/extend-by'=4600 more explicit.

Additionally, is this field gonna be modified through the SandboxClaim or just the Sandbox ? What about the SandboxTemplate ? if we have ttl in SandboxTemplate it should be overidden

[maqiuyujoyce]

Hi @barney-s , thank you for the intuitive illustration of the design. I have a few questions:

1. Does this design indicate that Sandbox now become mutable?

2. We probably want to set TTL at both the SandboxTemplate and SandboxClaim levels as well, so:
   a. Supporting both absolute and relative TTLs seems to make the UX pretty complicated, it makes the CR less readable (unsophisticated Sandbox users may get confused when the fields are all set). Can we start with the relative TTL only? If we really want to support all of them, can we make those fields mutually exclusive (i.e. either relative TTL fields or absolute TTL field can be set).

   b. What is the expected interpretation for TTL when it is unset? In the SandboxTemplate + SandboxClaim model, when TTL is unset in the SandboxClaim, it means that SandboxClaim will leverage the TTL configured in the corresponding SandboxTemplate. Though for SandboxTemplate and Sandbox, it means the TTL is infinite/never triggered. I'd find the defaults differences a bit confusing. Can we have a status field describing the TTL to be applied?

3. My understanding is that warmpool controller didn't plan to access and modify Sandbox CRs. Have you checked with stakeholders if they are okay with doing that?

[gongmax]

To the questions around warmpool. Warmpool controller will need permission to create Sandbox CR. So when WarmPool controller create a sandbox CR, it should set startPolicy: disable. When a Sandbox is claimed from pool, the SandboxClaim controller will update the Sandbox CR to enable TTL. (Note with the new WarmPool design, the claim of a pre-warmed Sandbox pod from the pool will also be done via creating a SandboxClaim).

[barney-s]

This request is scoped to Sandbox only. We should have a similar one for SandboxClaim (#17). I would assume it would have a similar shape ttl config.

@gongmax: My understanding with warmpool is that it is generic enough to take any object right ? So a user can create a pool of sandboxes as well if they want. In that case would this ttl config mechanism work ?

@maqiuyujoyce:

1. Does this design indicate that Sandbox now become mutable?

We haven't made Sandbox immutable. We should make parts of sandbox (like podTemplate) immutable. But we haven't yet.
TTL is something that should be mutable.

a. Supporting both absolute and relative TTLs seems to make the UX pretty complicated, it makes the CR less readable
Can we start with the relative TTL only? ...

#23 from community describes using absolute shutdown time. There is already a desire in the community for that. We have relative ttl use. So supporting both may be the right thing. We can definitely change the ttl config to make it less confusing.

What is the expected interpretation for TTL when it is unset?

We set the .status.shutdownAt to nil effectively stopping the timer.

In the SandboxTemplate + SandboxClaim model, when TTL is unset in the SandboxClaim, it means that SandboxClaim will leverage the TTL configured in the corresponding SandboxTemplate. ...

This is scoped to Sandbox only. Even when we have sandboxClaim we will set a TTL in sandboxClaim and thats it. No propagation. we will rely on cleanup via ownership.

@vicentefb:

Allow TTL value to be modified after creation.
I don't see a clear path on this specially with the introduction of startPolicy is this related to the toggle that can happen between onEnable and never. I.e when we set spec.ttl.startPolicy to never that means we modify the spec and we can modify the ttl and toggle back to onEnable so that the timer starts again and there's a new expiry time?

That is correct. It effectively resets the timer.

Not sure if this is exactly the best experience for the user to extend a ttl lease, we can start from here though,

We should encourage using absolute time (shutdownAt) in such cases where they want to keep extending it.

I would still be inclined into exploring the annotation approach here kubectl annotate agenticsandbox my-sandbox 'ttl.model.skippy.io/extend-by'=4600 more explicit.

This would be a one time extension ? What if i keep changing the value of extend-by ?

Additionally, is this field gonna be modified through the SandboxClaim or just the Sandbox ? What about the SandboxTemplate ? if we have ttl in SandboxTemplate it should be overidden.

Ideally the Sandbox TTL is for its own lifecycle. Similarly for SandboxClaim. We should not propagate TTL from claim to sandbox. We should instead rely on owner-refs for cleanup.

[gongmax]

@barney-s we switched the Warmpool design to make it Sandbox specific. Details see: #34. I also have an internal doc for it and add you as a reviewer so feel free to take a look.
And yes, I think this ttl config mechanism work with the new design.

[barney-s]

@flpanbin this is related to the scheduling you had in #23 . Would you please review this proposal and related PR #21

[maqiuyujoyce]

Thank you, @barney-s !

This is scoped to Sandbox only. Even when we have sandboxClaim we will set a TTL in sandboxClaim and thats it. No propagation. we will rely on cleanup via ownership.

When you said "no propagation", did you mean the SandboxClaim will not use the TTL specified at SandboxTemplate level by default? I think there may be a use case for that. @vicentefb could you confirm?