-
Notifications
You must be signed in to change notification settings - Fork 19
Skip disk space check when getting workspace for cleanup #1174
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
Skip disk space check when getting workspace for cleanup #1174
Conversation
aff1dd7 to
e8f6549
Compare
… operations When completing/cleaning up a script, the system was calling GetWorkspace() which always performed a disk space check. This created a catch-22: when disk space was low, the check would throw an exception preventing cleanup operations that would actually free up space. Additionally, read-only operations like checking status or reading logs were unnecessarily blocked by disk space checks. Added WorkspaceReadinessCheck enum with Perform/Skip values as a required parameter to GetWorkspace(). This makes it explicit at each call site whether disk space validation is needed: - Skip: For cleanup operations (freeing space) and read-only operations - Perform: For write operations that require disk space Changes: - Created WorkspaceReadinessCheck enum - Updated IScriptWorkspaceFactory.GetWorkspace() with required enum parameter - Updated ScriptWorkspaceFactory implementation to conditionally check readiness - Updated all call sites in ScriptService, ScriptServiceV2, KubernetesScriptServiceV1 - Updated Kubernetes crypto and state store operations - Updated test fixtures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cbf6e76 to
c2230e8
Compare
| public interface IScriptWorkspaceFactory | ||
| { | ||
| IScriptWorkspace GetWorkspace(ScriptTicket ticket); | ||
| IScriptWorkspace GetWorkspace(ScriptTicket ticket, WorkspaceReadinessCheck readinessCheck); |
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 opted for making this a choice the deb must make each time they get the workspace.
evolutionise
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
Happy for it to ship but I wonder whether it would've been simpler to change the behaviour to not throw an exception OR to catch the exception when we're doing a clean up.
|
I am not sure we why do a disk space check on new scripts maybe a good reason exists, so I left that. If we try catch then we wont have a workspace to call delete on so we can't clean it with that approach. |
Summary
We often see failures because we are unable to clean up after a script because the "disk space check" has failed. Meaning we don't cleanup because the tentacle is low on disk space.
Since that is insane, this PR changes tentacle so that it can cleanup and check status of scripts without checking if the tentacle is low on disk space.
Also this allows a script to be cancelled and cleaned up when it is low on disk space.
See: https://octopusdeploy.slack.com/archives/CG4JP65N2/p1762406103092919?thread_ts=1762404506.174969&cid=CG4JP65N2
AI Summary
WorkspaceReadinessCheckenum with explicitPerform/SkipvaluesIScriptWorkspaceFactory.GetWorkspace()to require readiness check specificationProblem
When completing/cleaning up a script, the system was calling
GetWorkspace()which always performed a disk space check viaCheckReadiness(). When disk space was low, this check would throw anIOExceptionpreventing:Solution
Replaced implicit disk space checking with explicit control via a
WorkspaceReadinessCheckenum:WorkspaceReadinessCheck.Skip: For cleanup operations (freeing space) and read-only operations (reading logs, checking state)WorkspaceReadinessCheck.Perform: For write operations that require disk space (starting scripts, writing files)The enum parameter is required (not optional), making it explicit at every call site whether space validation is needed.
Changes
WorkspaceReadinessCheck.cs- Enum withPerformandSkipvaluesIScriptWorkspaceFactory.cs- Changed signature to requireWorkspaceReadinessCheckparameterScriptWorkspaceFactory.cs- Updated implementation to conditionally check readiness based on enumScriptService.cs- Skip check inCompleteScriptAsyncandGetResponse(V1)ScriptServiceV2.cs- Skip check inCompleteScriptAsyncandGetResponse; perform check when starting scripts (V2)KubernetesScriptServiceV1.cs- Skip check inCompleteScriptAsync(Kubernetes)ScriptPodLogEncryptionKeyProvider.cs- Perform check when writing keys, skip when readingScriptPodSinceTimeStore.cs- Perform check (used for both read and write)Test plan
🤖 Generated with Claude Code