mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-06 17:08:10 +00:00
fix: reduce prune estimate work and add managed-scope timeout (#1768)
* fix: reduce prune estimate work and add managed-scope timeout estimateSystemReclaim previously called getDiskUsageClassified, which walks the full classified-resources pipeline (6+ Docker API calls and filesystem I/O) under the 8 s timeout, but only reads the three reclaimable* fields that getDiskUsage() (a single docker.df() call) already provides. Switch to getDiskUsage() so the timeout actually bounds the work the comment describes. Additionally, the managed-scope estimateManagedReclaim path had no timeout on either the remote route or the fleet local path. Wrap both call sites in withTimeout so a slow daemon surfaces the actionable 'Docker daemon is busy' message within 8 s instead of hanging until the hub's 15 s fetch abort fires. * fix: skip getStacks() for all scope in prune estimate route The remote handler unconditionally walked the compose directory before starting the 8 s estimate timer, but for 'all' scope the knownStackNames parameter is now unused (estimateSystemReclaim uses only docker system df). Mirror the fleet route's conditional so the walk only happens for managed scope, where estimateManagedReclaim genuinely needs stack names. Found during QA: on a Pilot node with real tunnel latency, this unbounded walk added latency outside the timeout budget. * fix: raise prune estimate budget to 12s for large image stores docker.df() cost scales with image-store size: measured ~7.4s on a 34GB / 96-image store, alone nearly exhausting the previous 8s budget before tunnel transport overhead. A healthy Pilot node could flip to 'Docker daemon is busy' at idle load. Raise PRUNE_ESTIMATE_TIMEOUT_MS and FLEET_DF_TIMEOUT_MS to 12s, which sits strictly below the hub's 15s AbortSignal.timeout on the fleet estimate fetch, keeping the remote 503 the actionable failure. The MonitorService janitor keeps its own 8s budget for destructive paths. Found in QA pass 2: single-target estimate failed at ~8.05s on a node where docker.df() alone takes ~7.4s.
This commit is contained in:
@@ -382,6 +382,48 @@ describe('DockerController - getDiskUsage', () => {
|
||||
expect(usage.reclaimableContainers).toBe(1_500);
|
||||
expect(usage.reclaimableContainerCount).toBe(2);
|
||||
});
|
||||
|
||||
it('estimateSystemReclaim returns same bytes as getDiskUsage for every target type', async () => {
|
||||
mockDocker.df.mockResolvedValue({
|
||||
LayersSize: 1_000_000_000,
|
||||
Images: [
|
||||
{ Id: 'a', Containers: 0, Size: 500_000_000, SharedSize: 0 },
|
||||
{ Id: 'b', Containers: 1, Size: 300_000_000, SharedSize: 0 },
|
||||
],
|
||||
Containers: [
|
||||
{ State: 'exited', SizeRw: 200 },
|
||||
],
|
||||
Volumes: [
|
||||
{ UsageData: { RefCount: 0, Size: 400 } },
|
||||
],
|
||||
});
|
||||
|
||||
const dc = DockerController.getInstance(1);
|
||||
const usage = await dc.getDiskUsage();
|
||||
|
||||
// Each estimateSystemReclaim(target) must equal the corresponding
|
||||
// getDiskUsage() field. knownStackNames is unused in the fast path
|
||||
// but retained for call-site symmetry.
|
||||
const expectations: Array<[target: 'images' | 'containers' | 'volumes' | 'networks', expected: number]> = [
|
||||
['images', usage.reclaimableImages],
|
||||
['containers', usage.reclaimableContainers],
|
||||
['volumes', usage.reclaimableVolumes],
|
||||
['networks', 0],
|
||||
];
|
||||
for (const [target, expected] of expectations) {
|
||||
const estimate = await dc.estimateSystemReclaim(target, []);
|
||||
expect(estimate.reclaimableBytes).toBe(expected);
|
||||
}
|
||||
|
||||
// The fast path uses only docker.df(), never the classified-resources
|
||||
// API calls that estimateSystemReclaim previously incurred via
|
||||
// getDiskUsageClassified (listImages, listVolumes, etc.).
|
||||
expect(mockDocker.df).toHaveBeenCalledTimes(5); // 1 getDiskUsage + 4 estimateSystemReclaim
|
||||
expect(mockDocker.listImages).not.toHaveBeenCalled();
|
||||
expect(mockDocker.listVolumes).not.toHaveBeenCalled();
|
||||
expect(mockDocker.listNetworks).not.toHaveBeenCalled();
|
||||
expect(mockDocker.listContainers).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// ── pruneSystem ────────────────────────────────────────────────────────
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
/**
|
||||
* F-6 regression: Fleet itemized plan enumeration and byte estimation both
|
||||
* bound the slow `docker system df` call (8s) and surface a recognizable
|
||||
* bound the slow `docker system df` call (12s) and surface a recognizable
|
||||
* timeout message to the operator.
|
||||
*
|
||||
* Covers:
|
||||
@@ -9,7 +9,7 @@
|
||||
*
|
||||
* Uses real timers because supertest dispatches lazily and the in-route
|
||||
* `withTimeout` setTimeout cannot be advanced via vi.useFakeTimers from
|
||||
* outside the request lifecycle.
|
||||
* outside the request lifecycle. Three timeout tests add ~25s to the file.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll, afterAll, afterEach, vi } from 'vitest';
|
||||
import request from 'supertest';
|
||||
@@ -29,7 +29,7 @@ beforeAll(async () => {
|
||||
({ default: DockerController } = await import('../services/DockerController'));
|
||||
({ FileSystemService } = await import('../services/FileSystemService'));
|
||||
({ activeBulkActions } = await import('../routes/labels'));
|
||||
// 10-minute expiry survives the file even with two ~8s timeout tests.
|
||||
// 10-minute expiry survives the file even with three ~12s timeout tests.
|
||||
const token = jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '10m' });
|
||||
authHeader = `Bearer ${token}`;
|
||||
});
|
||||
@@ -56,7 +56,7 @@ function stubLocalEstimate(
|
||||
vi.spyOn(FileSystemService.prototype, 'getStacks').mockResolvedValue([]);
|
||||
}
|
||||
|
||||
describe('Fleet prune routes bound docker df at 8s on local nodes (F-6)', () => {
|
||||
describe('Fleet prune routes bound docker df at 12s on local nodes (F-6)', () => {
|
||||
it('POST /api/fleet/labels/fleet-prune dry-run surfaces a busy-daemon error on local timeout', async () => {
|
||||
stubLocalEstimate(
|
||||
() => Promise.resolve({ reclaimableBytes: 0 }),
|
||||
@@ -94,6 +94,27 @@ describe('Fleet prune routes bound docker df at 8s on local nodes (F-6)', () =>
|
||||
expect(local.error).toMatch(/Docker daemon is busy/);
|
||||
}, 20_000);
|
||||
|
||||
it('POST /api/fleet/prune/estimate marks the local node unreachable on managed timeout', async () => {
|
||||
vi.spyOn(FileSystemService.prototype, 'getStacks').mockResolvedValue([]);
|
||||
// estimateManagedReclaim never settles so the managed path hits
|
||||
// FLEET_DF_TIMEOUT_MS and surfaces the busy-daemon error.
|
||||
vi.spyOn(DockerController, 'getInstance').mockReturnValue({
|
||||
estimateManagedReclaim: vi.fn().mockImplementation(() => new Promise(() => { /* never resolves */ })),
|
||||
estimateSystemReclaim: vi.fn().mockResolvedValue({ reclaimableBytes: 0 }),
|
||||
} as unknown as ReturnType<typeof DockerController.getInstance>);
|
||||
|
||||
const res = await request(app)
|
||||
.post('/api/fleet/prune/estimate')
|
||||
.set('Authorization', authHeader)
|
||||
.send({ targets: ['images'], scope: 'managed' });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(Array.isArray(res.body.perNode)).toBe(true);
|
||||
const local = res.body.perNode[0];
|
||||
expect(local.reachable).toBe(false);
|
||||
expect(local.error).toMatch(/Docker daemon is busy/);
|
||||
}, 20_000);
|
||||
|
||||
it('fleet-prune dry-run succeeds normally when estimateSystemReclaim resolves quickly', async () => {
|
||||
stubLocalEstimate(
|
||||
() => Promise.resolve({ reclaimableBytes: 256 }),
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
*
|
||||
* Uses real timers because supertest dispatches lazily and vi.useFakeTimers
|
||||
* does not compose cleanly with that pattern. Each timeout test waits the
|
||||
* full 8s withTimeout budget, so two such tests add ~17s to the file.
|
||||
* full 12s withTimeout budget, so three such tests add ~36s to the file.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll, afterAll, afterEach, vi } from 'vitest';
|
||||
import request from 'supertest';
|
||||
@@ -45,9 +45,12 @@ function stubFsStacks() {
|
||||
} as unknown as ReturnType<typeof FileSystemService.getInstance>);
|
||||
}
|
||||
|
||||
function stubEstimate(impl: () => Promise<{ reclaimableBytes: number }>) {
|
||||
function stubEstimate(
|
||||
impl: () => Promise<{ reclaimableBytes: number }>,
|
||||
method: 'estimateSystemReclaim' | 'estimateManagedReclaim' = 'estimateSystemReclaim',
|
||||
) {
|
||||
vi.spyOn(DockerController, 'getInstance').mockReturnValue({
|
||||
estimateSystemReclaim: vi.fn().mockImplementation(impl),
|
||||
[method]: vi.fn().mockImplementation(impl),
|
||||
} as unknown as ReturnType<typeof DockerController.getInstance>);
|
||||
}
|
||||
|
||||
@@ -122,6 +125,27 @@ describe('Prune estimate endpoints return 503 on slow docker df (F-6)', () => {
|
||||
expect(res.status).toBe(500);
|
||||
expect(res.body.code).not.toBe('docker_df_slow');
|
||||
});
|
||||
|
||||
it('managed images estimate returns 503 docker_df_slow when estimateManagedReclaim never settles', async () => {
|
||||
stubFsStacks();
|
||||
stubEstimate(
|
||||
() => new Promise(() => { /* never resolves */ }),
|
||||
'estimateManagedReclaim',
|
||||
);
|
||||
|
||||
const t0 = Date.now();
|
||||
const res = await request(app)
|
||||
.post('/api/system/prune/estimate')
|
||||
.set('Authorization', authHeader)
|
||||
.send({ target: 'images', scope: 'managed' });
|
||||
const elapsed = Date.now() - t0;
|
||||
|
||||
expect(res.status).toBe(503);
|
||||
expect(res.body.code).toBe('docker_df_slow');
|
||||
expect(res.body.error).toMatch(/Docker daemon is busy/);
|
||||
expect(elapsed).toBeGreaterThanOrEqual(7_500);
|
||||
expect(elapsed).toBeLessThan(15_000);
|
||||
}, 20_000);
|
||||
});
|
||||
|
||||
describe('Prune plan routes', () => {
|
||||
|
||||
@@ -38,8 +38,8 @@ import { buildTargetImageRef, isRepinBlocked, type ImagePinKind } from '../helpe
|
||||
import { withTimeout, TimeoutError } from '../utils/withTimeout';
|
||||
|
||||
// Mirror the system-maintenance route timeout so fleet's local-node prune
|
||||
// paths cap the slow `docker system df` call at the same 8s budget (F-6).
|
||||
const FLEET_DF_TIMEOUT_MS = 8_000;
|
||||
// paths cap the slow `docker system df` call at the same 12 s budget (F-6).
|
||||
const FLEET_DF_TIMEOUT_MS = 12_000;
|
||||
import { POLICY_SEVERITIES } from '../utils/severity';
|
||||
import { isNoOpBlockingPolicy } from '../utils/policy-risk';
|
||||
import { sanitizeForLog, redactSensitiveText } from '../utils/safeLog';
|
||||
@@ -2370,15 +2370,13 @@ fleetRouter.post('/prune/estimate', authMiddleware, async (req: Request, res: Re
|
||||
const dockerController = DockerController.getInstance(node.id);
|
||||
let nodeBytes = 0;
|
||||
for (const target of targets) {
|
||||
// estimateSystemReclaim hits `docker system df`; bound it so a
|
||||
// slow local daemon doesn't hang the fleet estimate (F-6).
|
||||
const result = scope === 'managed'
|
||||
? await dockerController.estimateManagedReclaim(target, knownStacks)
|
||||
: await withTimeout(
|
||||
dockerController.estimateSystemReclaim(target, knownStacks),
|
||||
FLEET_DF_TIMEOUT_MS,
|
||||
'docker disk usage',
|
||||
);
|
||||
// Bound so a slow local daemon does not hang the fleet
|
||||
// estimate (F-6). Both managed and all scopes bound each
|
||||
// per-target call at the same 12 s.
|
||||
const estimate = scope === 'managed'
|
||||
? dockerController.estimateManagedReclaim(target, knownStacks)
|
||||
: dockerController.estimateSystemReclaim(target, knownStacks);
|
||||
const result = await withTimeout(estimate, FLEET_DF_TIMEOUT_MS, 'docker disk usage');
|
||||
nodeBytes += result.reclaimableBytes;
|
||||
}
|
||||
return { nodeId: node.id, nodeName: node.name, reclaimableBytes: nodeBytes, reachable: true };
|
||||
|
||||
@@ -28,11 +28,15 @@ import { buildStackNetworkFacts } from '../services/network/composeNetworkInspec
|
||||
import { evaluateNetworkDeleteGuard } from '../services/network/networkDeleteGuards';
|
||||
import { loadNetworkingSnapshot } from '../services/network/networkingAggregate';
|
||||
|
||||
// `docker system df` (the call backing estimateSystemReclaim) can take 30+
|
||||
// seconds on Docker Desktop with many volumes; 8s matches the MonitorService
|
||||
// janitor timeout so the daemon never has more than ~16s of concurrent
|
||||
// pressure from Sencho's own paths even when prune and janitor collide.
|
||||
const PRUNE_ESTIMATE_TIMEOUT_MS = 8_000;
|
||||
// The prune estimate and plan paths are bounded at 12 s. `docker system df`
|
||||
// cost scales with image-store size (measured ~7.4 s on a 34 GB store), so
|
||||
// the estimate budget must leave headroom above a single df call. 12 s sits
|
||||
// strictly below the hub's 15 s AbortSignal.timeout on the fleet estimate
|
||||
// fetch, keeping the remote 503 the actionable failure instead of a hub-side
|
||||
// abort. The MonitorService janitor keeps its own 8 s budget
|
||||
// (JANITOR_TIMEOUT_MS) so Sencho's destructive paths never stack more than
|
||||
// ~16 s of concurrent daemon pressure with the janitor.
|
||||
const PRUNE_ESTIMATE_TIMEOUT_MS = 12_000;
|
||||
|
||||
function respondDfSlow(res: Response): Response {
|
||||
return res.status(503).json({
|
||||
@@ -322,26 +326,26 @@ systemMaintenanceRouter.post('/prune/estimate', async (req: Request, res: Respon
|
||||
}
|
||||
const pruneScope = scope === 'managed' ? 'managed' : 'all';
|
||||
const dockerController = DockerController.getInstance(req.nodeId);
|
||||
const knownStacks = await FileSystemService.getInstance(req.nodeId).getStacks();
|
||||
// Skip the filesystem walk for all scope: estimateSystemReclaim uses
|
||||
// only docker system df and ignores knownStackNames. Mirroring the
|
||||
// fleet route's conditional at fleet.ts:2367.
|
||||
const knownStacks = pruneScope === 'managed'
|
||||
? await FileSystemService.getInstance(req.nodeId).getStacks()
|
||||
: [];
|
||||
|
||||
let result: { reclaimableBytes: number };
|
||||
if (pruneScope === 'managed' && target !== 'containers') {
|
||||
result = await dockerController.estimateManagedReclaim(
|
||||
target as 'images' | 'volumes' | 'networks',
|
||||
knownStacks,
|
||||
);
|
||||
} else {
|
||||
// estimateSystemReclaim calls `docker system df`; bound it so a slow
|
||||
// daemon doesn't hang the admin's tab (F-6).
|
||||
result = await withTimeout(
|
||||
dockerController.estimateSystemReclaim(
|
||||
// Both estimate paths run under the same 12 s budget (F-6). The all-scope
|
||||
// fast path uses only `docker system df` via getDiskUsage(); the managed
|
||||
// path enumerates stacks but stays within the same bound.
|
||||
const estimate = pruneScope === 'managed' && target !== 'containers'
|
||||
? dockerController.estimateManagedReclaim(
|
||||
target as 'images' | 'volumes' | 'networks',
|
||||
knownStacks,
|
||||
)
|
||||
: dockerController.estimateSystemReclaim(
|
||||
target as 'containers' | 'images' | 'networks' | 'volumes',
|
||||
knownStacks,
|
||||
),
|
||||
PRUNE_ESTIMATE_TIMEOUT_MS,
|
||||
'docker disk usage',
|
||||
);
|
||||
}
|
||||
);
|
||||
const result = await withTimeout(estimate, PRUNE_ESTIMATE_TIMEOUT_MS, 'docker disk usage');
|
||||
res.json({ reclaimableBytes: result.reclaimableBytes });
|
||||
} catch (error: unknown) {
|
||||
if (error instanceof TimeoutError) {
|
||||
|
||||
@@ -964,15 +964,17 @@ class DockerController {
|
||||
}
|
||||
|
||||
/**
|
||||
* Non-destructive estimate for the `all` (system) prune scope. Reuses the
|
||||
* same disk-usage source as the resources page so the readout matches what
|
||||
* the operator already sees there.
|
||||
* Non-destructive estimate for the `all` (system) prune scope. Uses only
|
||||
* `docker system df` so the readout matches the daemon's own reclaimable
|
||||
* totals for each resource type. `knownStackNames` is retained for
|
||||
* call-site symmetry with {@link estimateManagedReclaim} and is unused
|
||||
* by this method.
|
||||
*/
|
||||
public async estimateSystemReclaim(
|
||||
target: 'containers' | 'images' | 'networks' | 'volumes',
|
||||
knownStackNames: string[],
|
||||
_knownStackNames: string[],
|
||||
): Promise<{ reclaimableBytes: number }> {
|
||||
const df = await this.getDiskUsageClassified(knownStackNames);
|
||||
const df = await this.getDiskUsage();
|
||||
if (target === 'images') return { reclaimableBytes: df.reclaimableImages };
|
||||
if (target === 'containers') return { reclaimableBytes: df.reclaimableContainers };
|
||||
if (target === 'volumes') return { reclaimableBytes: df.reclaimableVolumes };
|
||||
|
||||
Reference in New Issue
Block a user