From d60616999ee5ae3486d3406653aa144d4c7784db Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Mon, 19 Oct 2026 03:19:25 +0200 Subject: [PATCH 1/3] fix(hetzner): power off servers that do not finish a graceful shutdown `ksail cluster stop` on Hetzner sent an ACPI shutdown and waited five minutes for every server to be off, then failed. Since Talos v1.14.3 the scheduled Hetzner system test's Talos nodes no longer finish that shutdown, so every stop timed out. Wait up to three minutes for the graceful shutdown, then power off the servers that are still on and wait for them to be off. API failures and a cancelled stop are returned unchanged, without forcing anything. Part of #7858 Co-Authored-By: Claude Opus 5.5 --- pkg/svc/provider/hetzner/export_test.go | 7 +- pkg/svc/provider/hetzner/provider.go | 64 ++++++- pkg/svc/provider/hetzner/stop_nodes_test.go | 183 ++++++++++++++++++++ 3 files changed, 248 insertions(+), 6 deletions(-) create mode 100644 pkg/svc/provider/hetzner/stop_nodes_test.go diff --git a/pkg/svc/provider/hetzner/export_test.go b/pkg/svc/provider/hetzner/export_test.go index a163246c52..48febcd92a 100644 --- a/pkg/svc/provider/hetzner/export_test.go +++ b/pkg/svc/provider/hetzner/export_test.go @@ -18,7 +18,7 @@ func (p *Provider) WaitForServersStatusForTest( clusterName string, status hcloud.ServerStatus, ) error { - return p.waitForServersStatus(ctx, clusterName, status) + return p.waitForServersStatus(ctx, clusterName, status, DefaultActionTimeout) } // SourceIPsEqualForTest exports sourceIPsEqual for testing. @@ -131,3 +131,8 @@ func NewSnapshotManagerWithUploaderForTest( // BuildSSHFirewallRulesForTest exports buildSSHFirewallRules for testing. var BuildSSHFirewallRulesForTest = buildSSHFirewallRules + +// SetGracefulShutdownTimeoutForTest shortens the graceful shutdown window StopNodes waits for. +func (p *Provider) SetGracefulShutdownTimeoutForTest(timeout time.Duration) { + p.gracefulShutdownTimeout = timeout +} diff --git a/pkg/svc/provider/hetzner/provider.go b/pkg/svc/provider/hetzner/provider.go index cded0a0b6e..1e71eade99 100644 --- a/pkg/svc/provider/hetzner/provider.go +++ b/pkg/svc/provider/hetzner/provider.go @@ -16,6 +16,9 @@ import ( const ( // DefaultActionTimeout is the timeout for waiting on Hetzner actions. DefaultActionTimeout = 5 * time.Minute + // DefaultGracefulShutdownTimeout is how long StopNodes waits for a graceful + // (ACPI) shutdown before it powers off the servers that are still on. + DefaultGracefulShutdownTimeout = 3 * time.Minute // DefaultOperationTimeout is the timeout for individual operations. DefaultOperationTimeout = 2 * time.Minute // DefaultPollingInterval is the interval between status checks. @@ -48,9 +51,14 @@ const ( // ErrHetznerActionFailed indicates that a Hetzner action failed. var ErrHetznerActionFailed = errors.New("hetzner action failed") +// ErrServerStatusTimeout indicates that servers did not reach a status in time. +var ErrServerStatusTimeout = errors.New("timeout waiting for servers to reach status") + // Provider implements provider.Provider for Hetzner Cloud servers. type Provider struct { client *hcloud.Client + // gracefulShutdownTimeout overrides DefaultGracefulShutdownTimeout when positive. + gracefulShutdownTimeout time.Duration } // NewProvider creates a new Hetzner Cloud provider with the given client. @@ -87,7 +95,12 @@ func (p *Provider) StartNodes(ctx context.Context, clusterName string) error { } // StopNodes stops all servers for the given cluster. -// Uses graceful shutdown (ACPI signal) and waits for servers to be off. +// +// It sends a graceful shutdown (ACPI signal) and waits up to the graceful +// shutdown timeout for every server to be off. Servers still on after that +// window are powered off, the same as pulling the plug, so a node whose +// operating system never finishes its shutdown cannot leave the cluster +// half-stopped. func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { err := p.forEachServer(ctx, clusterName, func(server *hcloud.Server) (*hcloud.Action, error) { // Skip if already off @@ -106,8 +119,47 @@ func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { return err } - // Wait for all servers to be off - return p.waitForServersStatus(ctx, clusterName, hcloud.ServerStatusOff) + gracefulErr := p.waitForServersStatus( + ctx, + clusterName, + hcloud.ServerStatusOff, + p.gracefulShutdownWindow(), + ) + // Force servers off only when the graceful window itself ran out. An API + // failure, or a caller that cancelled the stop, is returned unchanged. + if gracefulErr == nil || !errors.Is(gracefulErr, ErrServerStatusTimeout) || ctx.Err() != nil { + return gracefulErr + } + + err = p.forEachServer(ctx, clusterName, func(server *hcloud.Server) (*hcloud.Action, error) { + if server.Status == hcloud.ServerStatusOff { + return nil, provider.ErrSkipAction + } + + action, _, poweroffErr := p.client.Server.Poweroff(ctx, server) + if poweroffErr != nil { + return nil, fmt.Errorf("failed to power off server %s: %w", server.Name, poweroffErr) + } + + return action, nil + }) + if err != nil { + return fmt.Errorf("graceful shutdown did not finish (%w), and forcing power off failed: %w", + gracefulErr, err) + } + + return p.waitForServersStatus(ctx, clusterName, hcloud.ServerStatusOff, DefaultActionTimeout) +} + +// gracefulShutdownWindow returns how long StopNodes waits for an ACPI shutdown. +// +//nolint:funcorder // Grouped with StopNodes for logical code organization +func (p *Provider) gracefulShutdownWindow() time.Duration { + if p.gracefulShutdownTimeout > 0 { + return p.gracefulShutdownTimeout + } + + return DefaultGracefulShutdownTimeout } // waitForServersStatus polls until all servers in the cluster reach the desired status. @@ -117,6 +169,7 @@ func (p *Provider) waitForServersStatus( ctx context.Context, clusterName string, desiredStatus hcloud.ServerStatus, + timeout time.Duration, ) error { var ( condErr error @@ -126,7 +179,7 @@ func (p *Provider) waitForServersStatus( err := wait.PollUntilContextTimeout( ctx, DefaultPollingInterval, - DefaultActionTimeout, + timeout, false, // match the legacy ticker: wait one interval before the first check. func(ctx context.Context) (bool, error) { ready, checkErr := p.allServersAtStatus(ctx, clusterName, desiredStatus) @@ -151,7 +204,8 @@ func (p *Provider) waitForServersStatus( } return fmt.Errorf( - "timeout waiting for servers to reach status %s: %w", + "%w %s: %w", + ErrServerStatusTimeout, desiredStatus, err, ) diff --git a/pkg/svc/provider/hetzner/stop_nodes_test.go b/pkg/svc/provider/hetzner/stop_nodes_test.go new file mode 100644 index 0000000000..46e17ca6d1 --- /dev/null +++ b/pkg/svc/provider/hetzner/stop_nodes_test.go @@ -0,0 +1,183 @@ +package hetzner_test + +import ( + "net/http" + "net/http/httptest" + "strconv" + "sync" + "testing" + "time" + + "github.com/devantler-tech/ksail/v7/pkg/svc/provider/hetzner" + "github.com/hetznercloud/hcloud-go/v2/hcloud/schema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const stopTestCluster = "stop-test" + +// stopTestAPI is a fake Hetzner API holding the power state of a cluster's +// servers. Servers whose name is in ignoreShutdown stay running after a graceful +// shutdown, like a node whose operating system never finishes shutting down. +type stopTestAPI struct { + mu sync.Mutex + status map[string]string + ids map[int64]string + ignoreShutdown map[string]bool + shutdowns []string + poweroffs []string +} + +func newStopTestAPI(status map[string]string, ignoreShutdown ...string) *stopTestAPI { + api := &stopTestAPI{ + status: status, + ids: map[int64]string{}, + ignoreShutdown: map[string]bool{}, + } + + var nextID int64 = 1 + for name := range status { + api.ids[nextID] = name + nextID++ + } + + for _, name := range ignoreShutdown { + api.ignoreShutdown[name] = true + } + + return api +} + +func (api *stopTestAPI) servers(nameFilter string) []schema.Server { + api.mu.Lock() + defer api.mu.Unlock() + + servers := []schema.Server{} + + for serverID, name := range api.ids { + if nameFilter != "" && nameFilter != name { + continue + } + + servers = append(servers, schema.Server{ + ID: serverID, + Name: name, + Status: api.status[name], + Labels: map[string]string{ + hetzner.LabelOwned: "true", + hetzner.LabelClusterName: stopTestCluster, + }, + }) + } + + return servers +} + +func (api *stopTestAPI) act(t *testing.T, writer http.ResponseWriter, request *http.Request) { + t.Helper() + + serverID, err := strconv.ParseInt(request.PathValue("id"), 10, 64) + if err != nil { + t.Errorf("parse id: %v", err) + http.Error(writer, err.Error(), http.StatusBadRequest) + + return + } + + command := request.PathValue("command") + + api.mu.Lock() + + name := api.ids[serverID] + + switch command { + case "shutdown": + api.shutdowns = append(api.shutdowns, name) + if !api.ignoreShutdown[name] { + api.status[name] = "off" + } + case "poweroff": + api.poweroffs = append(api.poweroffs, name) + api.status[name] = "off" + default: + t.Errorf("unexpected server action %q", command) + } + + api.mu.Unlock() + + writeJSONResponse(t, writer, map[string]any{"action": successTestAction(serverID)}) +} + +func (api *stopTestAPI) start(t *testing.T) *httptest.Server { + t.Helper() + + mux := http.NewServeMux() + + mux.HandleFunc("GET /servers", func(writer http.ResponseWriter, request *http.Request) { + writeJSONResponse( + t, + writer, + schema.ServerListResponse{Servers: api.servers(request.URL.Query().Get("name"))}, + ) + }) + + mux.HandleFunc( + "POST /servers/{id}/actions/{command}", + func(writer http.ResponseWriter, request *http.Request) { + api.act(t, writer, request) + }, + ) + + mux.HandleFunc("GET /actions", func(writer http.ResponseWriter, request *http.Request) { + actionID, err := strconv.ParseInt(request.URL.Query().Get("id"), 10, 64) + if err != nil { + t.Errorf("parse id: %v", err) + http.Error(writer, err.Error(), http.StatusBadRequest) + + return + } + + writeJSONResponse(t, writer, map[string]any{"actions": []any{successTestAction(actionID)}}) + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + return srv +} + +// TestStopNodes_GracefulShutdownNeedsNoPowerOff proves that servers which shut +// down gracefully are never forced off. +func TestStopNodes_GracefulShutdownNeedsNoPowerOff(t *testing.T) { + t.Parallel() + + api := newStopTestAPI(map[string]string{"cp-1": "running", "worker-1": "running"}) + prov := hetzner.NewProvider(newTestHcloudClient(t, api.start(t).URL)) + prov.SetGracefulShutdownTimeoutForTest(hetzner.DefaultPollingInterval + 3*time.Second) + + require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) + + assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, api.shutdowns) + assert.Empty(t, api.poweroffs, "a graceful shutdown that finished must not be forced") +} + +// TestStopNodes_PowersOffServersThatIgnoreShutdown proves that a server still +// running after the graceful window is powered off, so the stop succeeds +// instead of timing out, and that only that server is forced. +func TestStopNodes_PowersOffServersThatIgnoreShutdown(t *testing.T) { + t.Parallel() + + api := newStopTestAPI( + map[string]string{"cp-1": "running", "worker-1": "running", "worker-2": "off"}, + "cp-1", + ) + prov := hetzner.NewProvider(newTestHcloudClient(t, api.start(t).URL)) + prov.SetGracefulShutdownTimeoutForTest(hetzner.DefaultPollingInterval + time.Second) + + require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) + + assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, api.shutdowns, + "a server that is already off is not shut down again") + assert.Equal(t, []string{"cp-1"}, api.poweroffs, + "only the server still running after the graceful window is forced off") +} From 3f9be16514f6b3d28472c0db0c1ef761144f6034 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Mon, 19 Oct 2026 03:34:47 +0200 Subject: [PATCH 2/3] test(hetzner): pin power off when the last status read fails at the window deadline The provider cannot tell a status read its own graceful-window deadline interrupted from one that failed with a matching error at that moment; in both cases the window has run out. StopNodes powers off the servers still running, after listing them again, so it acts on their current state and fails if the API is down. Part of #7858 Co-Authored-By: Claude Opus 5.5 --- pkg/svc/provider/hetzner/stop_nodes_test.go | 46 +++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/pkg/svc/provider/hetzner/stop_nodes_test.go b/pkg/svc/provider/hetzner/stop_nodes_test.go index 46e17ca6d1..a9a7d9eaf9 100644 --- a/pkg/svc/provider/hetzner/stop_nodes_test.go +++ b/pkg/svc/provider/hetzner/stop_nodes_test.go @@ -26,6 +26,25 @@ type stopTestAPI struct { ignoreShutdown map[string]bool shutdowns []string poweroffs []string + // stallStatusRead, when set, holds the first server list that follows the + // graceful shutdowns open until the client gives up on it, like a slow API + // whose status read is still in flight when the graceful window ends. + stallStatusRead bool + stalled bool +} + +// stallsThisRead reports whether a server list request must be held open. +func (api *stopTestAPI) stallsThisRead(nameFilter string) bool { + api.mu.Lock() + defer api.mu.Unlock() + + if !api.stallStatusRead || api.stalled || nameFilter != "" || len(api.shutdowns) == 0 { + return false + } + + api.stalled = true + + return true } func newStopTestAPI(status map[string]string, ignoreShutdown ...string) *stopTestAPI { @@ -114,6 +133,12 @@ func (api *stopTestAPI) start(t *testing.T) *httptest.Server { mux := http.NewServeMux() mux.HandleFunc("GET /servers", func(writer http.ResponseWriter, request *http.Request) { + if api.stallsThisRead(request.URL.Query().Get("name")) { + <-request.Context().Done() + + return + } + writeJSONResponse( t, writer, @@ -181,3 +206,24 @@ func TestStopNodes_PowersOffServersThatIgnoreShutdown(t *testing.T) { assert.Equal(t, []string{"cp-1"}, api.poweroffs, "only the server still running after the graceful window is forced off") } + +// TestStopNodes_PowersOffWhenTheLastStatusReadFailsAtTheWindowDeadline proves +// that a status read still in flight when the graceful window ends does not +// stop the power off. The provider cannot tell a read its own deadline +// interrupted from one that failed with a matching error at that moment, and +// in both cases the window has run out. The power off step lists the servers +// again, so it acts on their current state and fails if the API is down. +func TestStopNodes_PowersOffWhenTheLastStatusReadFailsAtTheWindowDeadline(t *testing.T) { + t.Parallel() + + api := newStopTestAPI(map[string]string{"cp-1": "running", "worker-1": "running"}, "cp-1") + api.stallStatusRead = true + prov := hetzner.NewProvider(newTestHcloudClient(t, api.start(t).URL)) + prov.SetGracefulShutdownTimeoutForTest(hetzner.DefaultPollingInterval + time.Second) + + require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) + + assert.True(t, api.stalled, "the status read at the window deadline must have been interrupted") + assert.Equal(t, []string{"cp-1"}, api.poweroffs, + "only the server still running after the graceful window is forced off") +} From 3f4d578c59c3261640dd031f2466f21ec19de17e Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Mon, 19 Oct 2026 04:13:51 +0200 Subject: [PATCH 3/3] test(hetzner): read the fake API's records under its lock A stalled status handler can still be running after the client gave up, so the StopNodes tests now read shutdowns, power-offs and the stall flag through one locked snapshot instead of the raw fields. Co-Authored-By: Claude Opus 5.5 --- pkg/svc/provider/hetzner/stop_nodes_test.go | 30 ++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/pkg/svc/provider/hetzner/stop_nodes_test.go b/pkg/svc/provider/hetzner/stop_nodes_test.go index a9a7d9eaf9..0bf2c5da0d 100644 --- a/pkg/svc/provider/hetzner/stop_nodes_test.go +++ b/pkg/svc/provider/hetzner/stop_nodes_test.go @@ -3,6 +3,7 @@ package hetzner_test import ( "net/http" "net/http/httptest" + "slices" "strconv" "sync" "testing" @@ -47,6 +48,17 @@ func (api *stopTestAPI) stallsThisRead(nameFilter string) bool { return true } +// recorded returns the servers shut down, the servers powered off, and whether +// the API held a status read open, all read under the lock its handlers write +// them under. A stalled handler can still be running after the client gave up +// on it, so a test must never read these fields directly. +func (api *stopTestAPI) recorded() ([]string, []string, bool) { + api.mu.Lock() + defer api.mu.Unlock() + + return slices.Clone(api.shutdowns), slices.Clone(api.poweroffs), api.stalled +} + func newStopTestAPI(status map[string]string, ignoreShutdown ...string) *stopTestAPI { api := &stopTestAPI{ status: status, @@ -182,8 +194,10 @@ func TestStopNodes_GracefulShutdownNeedsNoPowerOff(t *testing.T) { require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) - assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, api.shutdowns) - assert.Empty(t, api.poweroffs, "a graceful shutdown that finished must not be forced") + shutdowns, poweroffs, _ := api.recorded() + + assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, shutdowns) + assert.Empty(t, poweroffs, "a graceful shutdown that finished must not be forced") } // TestStopNodes_PowersOffServersThatIgnoreShutdown proves that a server still @@ -201,9 +215,11 @@ func TestStopNodes_PowersOffServersThatIgnoreShutdown(t *testing.T) { require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) - assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, api.shutdowns, + shutdowns, poweroffs, _ := api.recorded() + + assert.ElementsMatch(t, []string{"cp-1", "worker-1"}, shutdowns, "a server that is already off is not shut down again") - assert.Equal(t, []string{"cp-1"}, api.poweroffs, + assert.Equal(t, []string{"cp-1"}, poweroffs, "only the server still running after the graceful window is forced off") } @@ -223,7 +239,9 @@ func TestStopNodes_PowersOffWhenTheLastStatusReadFailsAtTheWindowDeadline(t *tes require.NoError(t, prov.StopNodes(t.Context(), stopTestCluster)) - assert.True(t, api.stalled, "the status read at the window deadline must have been interrupted") - assert.Equal(t, []string{"cp-1"}, api.poweroffs, + _, poweroffs, stalled := api.recorded() + + assert.True(t, stalled, "the status read at the window deadline must have been interrupted") + assert.Equal(t, []string{"cp-1"}, poweroffs, "only the server still running after the graceful window is forced off") }