-
Notifications
You must be signed in to change notification settings - Fork 60
feat: added shutdownTime to extensions CRs #124
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: main
Are you sure you want to change the base?
feat: added shutdownTime to extensions CRs #124
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vicentefb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @vicentefb. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
055781e to
d8d27ee
Compare
|
Thanks! Yes, there's this if statement in the sandbox controller that checks if the current time is after the shutdownTime https://github.com/kubernetes-sigs/agent-sandbox/blob/main/controllers/sandbox_controller.go#L448-L451 |
hzxuzhonghu
left a comment
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.
/lgtm
71123b0 to
01f8f1d
Compare
|
New changes are detected. LGTM label has been removed. |
|
/ok-to-test |
nit nit rebasing
01f8f1d to
b31418f
Compare
dbdc1d4 to
9ce884e
Compare
|
/test presubmit-agent-sandbox-e2e-test |
janetkuo
left a comment
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.
Please also see #201
| claimTime := claim.Spec.ShutdownTime | ||
| sandboxTime := sandbox.Spec.ShutdownTime | ||
|
|
||
| if claimTime == nil && sandboxTime != 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'm not sure why we are reconciling the claim time against the live sandbox here. What's the intention here? Claim doesn't support updating sandbox currently.
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.
So i saw that the sandbox_controller.go allows and correctly handles updates to the shutdownTime field so in this case i was also allowing updates to the field from the SandboxClaim. Since the SandboxClaim is the user-facing interface, i thought users shouldn't need to find and edit the underlying Sandbox resource directly to change the field. I may be wrong by thinking about it like this but lmk if this makes sense or if i should revert this change.
| Name: claim.Name, | ||
| }, | ||
| } | ||
| sandbox.Spec.ShutdownTime = claim.Spec.ShutdownTime |
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 behavior is different from what's documented in the API spec
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.
Ohh true, i forgot to update the API spec comments.
Fixes #108
This PR introduces
shutdownTimefeature to the sandbox extensions, allowing users to define an expiration time for their sandboxes.shutdownTimein their SandboxClaim.This logic is implemented in the
sandboxclaim-controller, which copies the correct shutdownTime to the Sandbox resource upon creation. The coresandbox-controlleris then responsible for watching this field and deleting the sandbox pod when the time is reached.