From cbaf13f6f8c28a9e6c893329361c2d587e0a25f0 Mon Sep 17 00:00:00 2001 From: llogen Date: Tue, 30 Jun 2026 16:12:40 +0200 Subject: [PATCH] fix: ipmi survives an unreachable or dropped BMC Two connection failures took a whole worker down: (1) the module opened the IPMI session in Init(), so an unreachable BMC (e.g. the board's outer power off) failed module init, which dutagent treats as fatal -- crash-looping the whole agent and every other device with it; (2) IPMI/RMCP sessions idle-time-out and the BMC drops them, so the cached session went stale and every command timed out until the agent was restarted. Open the session lazily on first use instead of in Init, and run each command through a helper that drops the session, reconnects, and retries once on failure. A down BMC now fails only its own command instead of the agent, and a stale session self-heals -- no more crash-loops or worker reboots. Signed-off-by: llogen --- pkg/module/ipmi/ipmi.go | 132 ++++++++++++++++++++++++++++++++-------- 1 file changed, 106 insertions(+), 26 deletions(-) diff --git a/pkg/module/ipmi/ipmi.go b/pkg/module/ipmi/ipmi.go index d83b74ea..3939f6e9 100644 --- a/pkg/module/ipmi/ipmi.go +++ b/pkg/module/ipmi/ipmi.go @@ -34,7 +34,9 @@ type IPMI struct { Password string // Password is used for IPMI authentication. WARNING: Unsavely stored as plaintext Timeout string // Timeout is the duration for IPMI commands. Default: 10 seconds - client *ipmi.Client // client is the module's internal entity to forward IPMI commands + timeout time.Duration // timeout is the resolved command timeout; set in Init + client *ipmi.Client // client is the current IPMI session; nil until the first command + connected bool // connected tracks whether client holds a live session } // Ensure implementing the Module interface. @@ -73,19 +75,19 @@ const ( func (i *IPMI) Init(ctx context.Context) error { l := log.FromContext(ctx) - port := i.Port - if port == 0 { - port = defaultPort + if i.Port == 0 { + i.Port = defaultPort l.Debug(fmt.Sprintf("no port configured, using default %d", defaultPort)) } // Parse custom timeout if provided; an unparseable value falls back to the default. - timeout := defaultTimeout + i.timeout = defaultTimeout if i.Timeout != "" { parsedTimeout, err := time.ParseDuration(i.Timeout) if err == nil { - timeout = parsedTimeout + i.timeout = parsedTimeout + l.Debug(fmt.Sprintf("using custom timeout %s", i.timeout)) } else { l.Debug(fmt.Sprintf("invalid timeout %q, using default %s", i.Timeout, defaultTimeout)) } @@ -97,42 +99,105 @@ func (i *IPMI) Init(ctx context.Context) error { return fmt.Errorf("IPMI Host is not set") } - ipmiClient, err := ipmi.NewClient(i.Host, port, i.User, i.Password) + // Deliberately do NOT open the IPMI session here. The BMC may be + // unreachable at agent startup (e.g. the board's outer power is off), and + // the dutagent treats a failed Init() as fatal — it shuts down and + // systemd crash-loops the whole agent, taking down every other device on + // the worker too. The session is opened lazily on the first command and + // re-opened automatically if it goes stale (see connect / withSession). + l.Debug(fmt.Sprintf("init completed for %s:%d (BMC session deferred to first use)", i.Host, i.Port)) + + return nil +} + +// connect ensures a live IPMI session exists. It is a no-op when already +// connected; otherwise it builds a fresh client and opens a new session. A new +// client is used each time so a previously stale session is fully discarded. +// Called lazily on the first command (not in Init) so an unreachable BMC +// surfaces as a normal command error instead of a fatal module-init failure. +func (i *IPMI) connect(ctx context.Context) error { + if i.connected && i.client != nil { + return nil + } + + client, err := ipmi.NewClient(i.Host, i.Port, i.User, i.Password) if err != nil { return fmt.Errorf("failed to create IPMI client: %v", err) } - ipmiClient.WithTimeout(timeout) - ipmiClient.WithRetry(trials) + client.WithTimeout(i.timeout) + client.WithRetry(trials) - // Bounded by ctx: today the init context is a plain background context, but - // passing it (not context.Background()) means a later startup deadline or - // shutdown cancellation on that context will bound this connect. TODO(ctx). - err = ipmiClient.Connect(ctx) + err = client.Connect(ctx) if err != nil { - return fmt.Errorf("failed to connect to IPMI BMC %s:%d: %v", i.Host, port, err) + return fmt.Errorf("failed to connect to IPMI BMC %s: %w", i.Host, err) } - i.client = ipmiClient - - l.Debug(fmt.Sprintf("connected to BMC %s:%d", i.Host, port)) + i.client = client + i.connected = true return nil } -func (i *IPMI) Deinit(ctx context.Context) error { - if i.client == nil { +// disconnect closes the current session (best-effort) and clears it so the next +// connect opens a fresh one. Used to recover from a session that has gone stale. +func (i *IPMI) disconnect(ctx context.Context) { + if i.client != nil { + _ = i.client.Close(ctx) + } + + i.client = nil + i.connected = false +} + +// withSession runs op against a live IPMI session, transparently re-opening the +// session once if op fails. IPMI/RMCP sessions idle-time-out and the BMC drops +// inactive ones, so a cached session goes stale between commands; without this +// the module would keep using the dead session and every command would time out +// until the agent was restarted. On the first failure the session is dropped, +// reconnected, and op is retried once so a stale session self-heals. +func (i *IPMI) withSession(ctx context.Context, op func() error) error { + err := i.connect(ctx) + if err != nil { + return err + } + + err = op() + if err == nil { return nil } - return i.client.Close(ctx) + // First attempt failed — most likely a stale session. Drop it, reconnect, + // and retry once against a fresh session. + log.FromContext(ctx).Debug(fmt.Sprintf("command failed (%v); re-opening session and retrying once", err)) + + i.disconnect(ctx) + + cerr := i.connect(ctx) + if cerr != nil { + return fmt.Errorf("%v; reconnect failed: %w", err, cerr) + } + + return op() } -func (i *IPMI) Run(ctx context.Context, s module.Session, args ...string) error { - if i.client == nil { - return fmt.Errorf("IPMI client not initialized") +func (i *IPMI) Deinit(ctx context.Context) error { + if i.client == nil || !i.connected { + return nil + } + + err := i.client.Close(ctx) + if err != nil { + log.FromContext(ctx).Debug(fmt.Sprintf("Deinit failed to close client: %v", err)) } + i.client = nil + i.connected = false + + return err +} + +func (i *IPMI) Run(ctx context.Context, s module.Session, args ...string) error { if len(args) == 0 { s.Println("No command specified. Try 'help' for usage.") @@ -175,7 +240,11 @@ func (i *IPMI) handlePowerCommand(ctx context.Context, s module.Session, command message = "Power RESET command sent" } - _, err := i.client.ChassisControl(ctx, controlType) + err := i.withSession(ctx, func() error { + _, cerr := i.client.ChassisControl(ctx, controlType) + + return cerr + }) if err != nil { return fmt.Errorf("power %s command failed: %v", command, err) } @@ -187,13 +256,24 @@ func (i *IPMI) handlePowerCommand(ctx context.Context, s module.Session, command } func (i *IPMI) handleStatusCommand(ctx context.Context, s module.Session) error { - status, err := i.client.GetChassisStatus(ctx) + var powerIsOn bool + + err := i.withSession(ctx, func() error { + chassis, cerr := i.client.GetChassisStatus(ctx) + if cerr != nil { + return cerr + } + + powerIsOn = chassis.PowerIsOn + + return nil + }) if err != nil { return fmt.Errorf("failed to get chassis status: %v", err) } powerStatus := "Off" - if status.PowerIsOn { + if powerIsOn { powerStatus = "On" }