From f34864758ebc62552511e447fe2c5541525e9361 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Evrard Date: Sun, 29 Sep 2024 18:16:18 +0200 Subject: [PATCH] Cleanup rebooter interface Without this, the interface and the code to reboot is a bit more complex than it should be. We do not need setters and getters, as we are just instanciating a single instance of a rebooter interface. We create it based on user input, then pass the object around. This should cleanup the code. Signed-off-by: Jean-Philippe Evrard --- cmd/kured/main.go | 59 ++++++++++++++++-------------------------- cmd/kured/main_test.go | 28 +------------------- pkg/reboot/command.go | 17 +++++------- pkg/reboot/reboot.go | 7 +++-- pkg/reboot/signal.go | 21 ++++++--------- 5 files changed, 44 insertions(+), 88 deletions(-) diff --git a/cmd/kured/main.go b/cmd/kured/main.go index 3ad1cbe..77217e2 100644 --- a/cmd/kured/main.go +++ b/cmd/kured/main.go @@ -108,11 +108,6 @@ const ( // EnvPrefix The environment variable prefix of all environment variables bound to our command line flags. EnvPrefix = "KURED" - // MethodCommand is used as "--reboot-method" value when rebooting with the configured "--reboot-command" - MethodCommand = "command" - // MethodSignal is used as "--reboot-method" value when rebooting with a SIGRTMIN+5 signal. - MethodSignal = "signal" - sigTrminPlus5 = 34 + 5 ) @@ -646,7 +641,7 @@ func updateNodeLabels(client *kubernetes.Clientset, node *v1.Node, labels []stri } } -func rebootAsRequired(nodeID string, booter reboot.Reboot, sentinelCommand []string, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) { +func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand []string, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) { config, err := rest.InClusterConfig() if err != nil { log.Fatal(err) @@ -796,7 +791,7 @@ func rebootAsRequired(nodeID string, booter reboot.Reboot, sentinelCommand []str } } - booter.Reboot() + rebooter.Reboot() for { log.Infof("Waiting for reboot") time.Sleep(time.Minute) @@ -817,16 +812,6 @@ func buildSentinelCommand(rebootSentinelFile string, rebootSentinelCommand strin return []string{"test", "-f", rebootSentinelFile} } -// parseRebootCommand creates the shell command line which will need wrapping to escape -// the container boundaries -func parseRebootCommand(rebootCommand string) []string { - command, err := shlex.Split(rebootCommand) - if err != nil { - log.Fatalf("Error parsing provided reboot command: %v", err) - } - return command -} - func root(cmd *cobra.Command, args []string) { if logFormat == "json" { log.SetFormatter(&log.JSONFormatter{}) @@ -844,7 +829,6 @@ func root(cmd *cobra.Command, args []string) { } sentinelCommand := buildSentinelCommand(rebootSentinelFile, rebootSentinelCommand) - restartCommand := parseRebootCommand(rebootCommand) log.Infof("Node ID: %s", nodeID) log.Infof("Lock Annotation: %s/%s:%s", dsNamespace, dsName, lockAnnotation) @@ -864,20 +848,32 @@ func root(cmd *cobra.Command, args []string) { log.Infof("Reboot check command: %s every %v", sentinelCommand, period) log.Infof("Concurrency: %v", concurrency) log.Infof("Reboot method: %s", rebootMethod) - if rebootCommand == MethodCommand { - log.Infof("Reboot command: %s", restartCommand) - } else { - log.Infof("Reboot signal: %v", rebootSignal) - } - if annotateNodes { - log.Infof("Will annotate nodes during kured reboot operations") + restartCommand, err := shlex.Split(rebootCommand) + if err != nil { + log.Fatalf("Error parsing provided reboot command: %v", err) } // To run those commands as it was the host, we'll use nsenter // Relies on hostPID:true and privileged:true to enter host mount space // PID set to 1, until we have a better discovery mechanism. - hostRestartCommand := buildHostCommand(1, restartCommand) + privilegedRestartCommand := buildHostCommand(1, restartCommand) + + var rebooter reboot.Rebooter + switch { + case rebootMethod == "command": + log.Infof("Reboot command: %s", restartCommand) + rebooter = reboot.CommandRebooter{NodeID: nodeID, RebootCommand: privilegedRestartCommand} + case rebootMethod == "signal": + log.Infof("Reboot signal: %v", rebootSignal) + rebooter = reboot.SignalRebooter{NodeID: nodeID, Signal: rebootSignal} + default: + log.Fatalf("Invalid reboot-method configured: %s", rebootMethod) + } + + if annotateNodes { + log.Infof("Will annotate nodes during kured reboot operations") + } // Only wrap sentinel-command with nsenter, if a custom-command was configured, otherwise use the host-path mount hostSentinelCommand := sentinelCommand @@ -885,16 +881,7 @@ func root(cmd *cobra.Command, args []string) { hostSentinelCommand = buildHostCommand(1, sentinelCommand) } - var booter reboot.Reboot - if rebootMethod == MethodCommand { - booter = reboot.NewCommandReboot(nodeID, hostRestartCommand) - } else if rebootMethod == MethodSignal { - booter = reboot.NewSignalReboot(nodeID, rebootSignal) - } else { - log.Fatalf("Invalid reboot-method configured: %s", rebootMethod) - } - - go rebootAsRequired(nodeID, booter, hostSentinelCommand, window, lockTTL, lockReleaseDelay) + go rebootAsRequired(nodeID, rebooter, hostSentinelCommand, window, lockTTL, lockReleaseDelay) go maintainRebootRequiredMetric(nodeID, hostSentinelCommand) http.Handle("/metrics", promhttp.Handler()) diff --git a/cmd/kured/main_test.go b/cmd/kured/main_test.go index a7b11b1..04d4cd4 100644 --- a/cmd/kured/main_test.go +++ b/cmd/kured/main_test.go @@ -4,9 +4,9 @@ import ( "reflect" "testing" + "github.com/kubereboot/kured/pkg/alerts" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" - "github.com/kubereboot/kured/pkg/alerts" assert "gotest.tools/v3/assert" papi "github.com/prometheus/client_golang/api" @@ -223,32 +223,6 @@ func Test_buildSentinelCommand(t *testing.T) { } } -func Test_parseRebootCommand(t *testing.T) { - type args struct { - rebootCommand string - } - tests := []struct { - name string - args args - want []string - }{ - { - name: "Ensure a reboot command is properly parsed", - args: args{ - rebootCommand: "/sbin/systemctl reboot", - }, - want: []string{"/sbin/systemctl", "reboot"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := parseRebootCommand(tt.args.rebootCommand); !reflect.DeepEqual(got, tt.want) { - t.Errorf("parseRebootCommand() = %v, want %v", got, tt.want) - } - }) - } -} - func Test_rebootRequired(t *testing.T) { type args struct { sentinelCommand []string diff --git a/pkg/reboot/command.go b/pkg/reboot/command.go index 1522d2a..f353fe5 100644 --- a/pkg/reboot/command.go +++ b/pkg/reboot/command.go @@ -5,21 +5,18 @@ import ( log "github.com/sirupsen/logrus" ) -// CommandRebootMethod holds context-information for a command reboot. -type CommandRebootMethod struct { - nodeID string - rebootCommand []string +// CommandRebooter holds context-information for a command reboot. +type CommandRebooter struct { + NodeID string + RebootCommand []string } // NewCommandReboot creates a new command-rebooter which needs full privileges on the host. -func NewCommandReboot(nodeID string, rebootCommand []string) *CommandRebootMethod { - return &CommandRebootMethod{nodeID: nodeID, rebootCommand: rebootCommand} -} // Reboot triggers the command-reboot. -func (c *CommandRebootMethod) Reboot() { - log.Infof("Running command: %s for node: %s", c.rebootCommand, c.nodeID) - if err := util.NewCommand(c.rebootCommand[0], c.rebootCommand[1:]...).Run(); err != nil { +func (c CommandRebooter) Reboot() { + log.Infof("Running command: %s for node: %s", c.RebootCommand, c.NodeID) + if err := util.NewCommand(c.RebootCommand[0], c.RebootCommand[1:]...).Run(); err != nil { log.Fatalf("Error invoking reboot command: %v", err) } } diff --git a/pkg/reboot/reboot.go b/pkg/reboot/reboot.go index 83d788e..84ea93a 100644 --- a/pkg/reboot/reboot.go +++ b/pkg/reboot/reboot.go @@ -1,6 +1,9 @@ package reboot -// Reboot interface defines the Reboot function to be implemented. -type Reboot interface { +// Rebooter is the standard interface to use to execute +// the reboot, after it has been considered as necessary. +// The Reboot method does not expect any return, yet should +// most likely be refactored in the future to return an error +type Rebooter interface { Reboot() } diff --git a/pkg/reboot/signal.go b/pkg/reboot/signal.go index 245018c..7f5a2c3 100644 --- a/pkg/reboot/signal.go +++ b/pkg/reboot/signal.go @@ -7,27 +7,22 @@ import ( log "github.com/sirupsen/logrus" ) -// SignalRebootMethod holds context-information for a signal reboot. -type SignalRebootMethod struct { - nodeID string - signal int +// SignalRebooter holds context-information for a signal reboot. +type SignalRebooter struct { + NodeID string + Signal int } -// NewSignalReboot creates a new signal-rebooter which can run unprivileged. -func NewSignalReboot(nodeID string, signal int) *SignalRebootMethod { - return &SignalRebootMethod{nodeID: nodeID, signal: signal} -} - -// Reboot triggers the signal-reboot. -func (c *SignalRebootMethod) Reboot() { - log.Infof("Emit reboot-signal for node: %s", c.nodeID) +// Reboot triggers the reboot signal using SIGTERMIN+5 +func (c SignalRebooter) Reboot() { + log.Infof("Emit reboot-signal for node: %s", c.NodeID) process, err := os.FindProcess(1) if err != nil { log.Fatalf("There was no systemd process found: %v", err) } - err = process.Signal(syscall.Signal(c.signal)) + err = process.Signal(syscall.Signal(c.Signal)) if err != nil { log.Fatalf("Signal of SIGRTMIN+5 failed: %v", err) }