diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 0e47f9e058..c0ac04025a 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1403,6 +1403,14 @@ reporting source of truth. `MultiTenantMonitor.ListOrganizationIDs` may expose persisted organization IDs to API-owned background workers, but it must not initialize monitors, start pollers, or reinterpret tenant IDs as monitored resource health. +Organization deletion waits up to `tenantDeletionShutdownTimeout` (30 seconds) +for the tenant monitoring loop to exit before it refuses with +`tenant_shutdown_incomplete` and keeps the data. Process shutdown and +`RemoveTenant` keep the two-second `tenantMonitorShutdownTimeout`. Deletion +gates the data on that wait, so the budget must cover a loop that is still +starting up or finishing a tick: under CI load a just-used organization's loop +took 8-10 seconds to see its cancelled context, and the two-second budget turned +an ordinary delete into a refusal. A loop that never exits still refuses. Proxmox physical-disk polling is also a continuity boundary. A failed or permission-denied `disks/list` call must remain an error so the monitor can use linked host-agent inventory or retain same-instance, same-node prior evidence; diff --git a/internal/monitoring/guest_metadata_shutdown_test.go b/internal/monitoring/guest_metadata_shutdown_test.go index 8c9ad302ee..074518a4b9 100644 --- a/internal/monitoring/guest_metadata_shutdown_test.go +++ b/internal/monitoring/guest_metadata_shutdown_test.go @@ -79,6 +79,7 @@ func TestTenantDeletionRetainsBlockedGuestWriterOwnership(t *testing.T) { } } func TestTenantDeletionRetainsUnfinishedMonitorLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 100*time.Millisecond) mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) _, cancel := context.WithCancel(context.Background()) done := make(chan struct{}) diff --git a/internal/monitoring/monitor_additional_test.go b/internal/monitoring/monitor_additional_test.go index 15a9875f7b..eaaa70230a 100644 --- a/internal/monitoring/monitor_additional_test.go +++ b/internal/monitoring/monitor_additional_test.go @@ -1102,3 +1102,57 @@ func TestAutomaticGuestMemoryLinkNextPoll(t *testing.T) { t.Fatalf("ambiguous hostname selected %q/%q/%q", node, linkedVM, ct) } } + +// shortenTenantShutdownBudgets makes the shutdown waits fast enough for tests +// while keeping deletion's budget longer than the plain one, as in production. +func shortenTenantShutdownBudgets(t *testing.T, plain, deletion time.Duration) { + t.Helper() + previousPlain, previousDeletion := tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = plain, deletion + t.Cleanup(func() { + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = previousPlain, previousDeletion + }) +} + +// A tenant monitor that is still starting up or finishing a tick can take +// longer than the plain shutdown budget to see its cancelled context. In CI a +// just-used organization's loop took 8-10s, so deletion refused with +// tenant_shutdown_incomplete. Deletion waits on its own, longer budget instead. +func TestTenantDeletionWaitsForSlowMonitoringLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 2*time.Second) + + slowLoop := func(mtm *MultiTenantMonitor, orgID string) *Monitor { + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + <-ctx.Done() + // Exits well after the plain budget but inside deletion's. + time.Sleep(300 * time.Millisecond) + close(done) + }() + monitor := &Monitor{} + mtm.monitors[orgID] = monitor + mtm.tenantCancel[orgID] = cancel + mtm.tenantDone[orgID] = done + return monitor + } + + mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) + t.Cleanup(mtm.Stop) + + // RemoveTenant keeps the plain budget, gives up and retains the owner. + retained := slowLoop(mtm, "removal") + mtm.RemoveTenant("removal") + if got, ok := mtm.PeekMonitor("removal"); !ok || got != retained { + t.Fatal("plain removal discarded a monitor whose loop had not stopped") + } + + slowLoop(mtm, "deletion") + if err := mtm.BeginTenantDeletion("deletion"); err != nil { + t.Fatalf("deletion refused a loop that stopped inside its budget: %v", err) + } + if _, ok := mtm.PeekMonitor("deletion"); ok { + t.Fatal("completed deletion left the monitor registered") + } + mtm.FinishTenantDeletion("deletion") +} diff --git a/internal/monitoring/multi_tenant_monitor.go b/internal/monitoring/multi_tenant_monitor.go index 248b842239..a11950ad1f 100644 --- a/internal/monitoring/multi_tenant_monitor.go +++ b/internal/monitoring/multi_tenant_monitor.go @@ -47,7 +47,19 @@ func NewMultiTenantMonitor(baseCfg *config.Config, persistence *config.MultiTena } } -const tenantMonitorShutdownTimeout = 2 * time.Second +// Both shutdown budgets are variables so tests can shorten them. +var ( + // tenantMonitorShutdownTimeout bounds how long process shutdown and + // RemoveTenant wait for a tenant's monitoring loop to exit. + tenantMonitorShutdownTimeout = 2 * time.Second + + // tenantDeletionShutdownTimeout bounds how long organization deletion + // waits for the loop. Deletion refuses, and keeps the data, while the loop + // still runs, so this is a correctness gate rather than a best-effort + // wait: a loop that is still starting up or finishing a tick can take + // several seconds to see its cancelled context. + tenantDeletionShutdownTimeout = 30 * time.Second +) // SetRecoveryManager wires a recovery store manager into all existing and future tenant monitors. func (mtm *MultiTenantMonitor) SetRecoveryManager(manager *recoverymanager.Manager) { @@ -377,7 +389,11 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { if cancel != nil { cancel() } - if !waitForTenantMonitorShutdown(orgID, done) { + shutdownTimeout := tenantMonitorShutdownTimeout + if deleting { + shutdownTimeout = tenantDeletionShutdownTimeout + } + if !waitForTenantMonitorShutdown(orgID, done, shutdownTimeout) { return fmt.Errorf("organization %q monitoring loop has not stopped", orgID) } if monitor != nil { @@ -397,19 +413,19 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { return nil } -func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}) bool { +func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}, timeout time.Duration) bool { if done == nil { return true } - timer := time.NewTimer(tenantMonitorShutdownTimeout) + timer := time.NewTimer(timeout) defer timer.Stop() select { case <-done: return true case <-timer.C: - logger := log.Warn().Dur("timeout", tenantMonitorShutdownTimeout) + logger := log.Warn().Dur("timeout", timeout) if orgID != "" { logger = logger.Str("org_id", orgID) }