From 574065ff8a8877fdd824f8b157ea764b3feff173 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Evrard Date: Sun, 29 Sep 2024 21:17:04 +0200 Subject: [PATCH] Add checker interface This will be useful to refactor the checkers loop. Signed-off-by: Jean-Philippe Evrard --- cmd/kured/main.go | 82 ++++++----------- cmd/kured/main_test.go | 174 +---------------------------------- pkg/checkers/checker.go | 74 +++++++++++++++ pkg/checkers/checker_test.go | 69 ++++++++++++++ 4 files changed, 173 insertions(+), 226 deletions(-) create mode 100644 pkg/checkers/checker.go create mode 100644 pkg/checkers/checker_test.go diff --git a/cmd/kured/main.go b/cmd/kured/main.go index 18b7d73..fe25928 100644 --- a/cmd/kured/main.go +++ b/cmd/kured/main.go @@ -4,11 +4,11 @@ import ( "context" "encoding/json" "fmt" + "github.com/kubereboot/kured/pkg/checkers" "math/rand" "net/http" "net/url" "os" - "os/exec" "reflect" "regexp" "sort" @@ -309,28 +309,6 @@ func flagToEnvVar(flag string) string { return fmt.Sprintf("%s_%s", EnvPrefix, envVarSuffix) } -func rebootRequired(sentinelCommand []string) bool { - cmd := util.NewCommand(sentinelCommand[0], sentinelCommand[1:]...) - if err := cmd.Run(); err != nil { - switch err := err.(type) { - case *exec.ExitError: - // We assume a non-zero exit code means 'reboot not required', but of course - // the user could have misconfigured the sentinel command or something else - // went wrong during its execution. In that case, not entering a reboot loop - // is the right thing to do, and we are logging stdout/stderr of the command - // so it should be obvious what is wrong. - if cmd.ProcessState.ExitCode() != 1 { - log.Warnf("sentinel command ended with unexpected exit code: %v", cmd.ProcessState.ExitCode()) - } - return false - default: - // Something was grossly misconfigured, such as the command path being wrong. - log.Fatalf("Error invoking sentinel command: %v", err) - } - } - return true -} - // RebootBlocker interface should be implemented by types // to know if their instantiations should block a reboot type RebootBlocker interface { @@ -540,9 +518,9 @@ func uncordon(client *kubernetes.Clientset, node *v1.Node) error { return nil } -func maintainRebootRequiredMetric(nodeID string, sentinelCommand []string) { +func maintainRebootRequiredMetric(nodeID string, checker checkers.Checker) { for { - if rebootRequired(sentinelCommand) { + if checker.CheckRebootRequired() { rebootRequiredGauge.WithLabelValues(nodeID).Set(1) } else { rebootRequiredGauge.WithLabelValues(nodeID).Set(0) @@ -630,7 +608,7 @@ func updateNodeLabels(client *kubernetes.Clientset, node *v1.Node, labels []stri } } -func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand []string, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) { +func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, checker checkers.Checker, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) { config, err := rest.InClusterConfig() if err != nil { log.Fatal(err) @@ -671,7 +649,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [ // And (2) check if we previously annotated the node that it was in the process of being rebooted, // And finally (3) if it has that annotation, to delete it. // This indicates to other node tools running on the cluster that this node may be a candidate for maintenance - if annotateNodes && !rebootRequired(sentinelCommand) { + if annotateNodes && !checker.CheckRebootRequired() { if _, ok := node.Annotations[KuredRebootInProgressAnnotation]; ok { err := deleteNodeAnnotation(client, nodeID, KuredRebootInProgressAnnotation) if err != nil { @@ -690,7 +668,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [ preferNoScheduleTaint := taints.New(client, nodeID, preferNoScheduleTaintName, v1.TaintEffectPreferNoSchedule) // Remove taint immediately during startup to quickly allow scheduling again. - if !rebootRequired(sentinelCommand) { + if !checker.CheckRebootRequired() { preferNoScheduleTaint.Disable() } @@ -709,7 +687,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [ continue } - if !rebootRequired(sentinelCommand) { + if !checker.CheckRebootRequired() { log.Infof("Reboot not required") preferNoScheduleTaint.Disable() continue @@ -788,19 +766,6 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [ } } -// buildSentinelCommand creates the shell command line which will need wrapping to escape -// the container boundaries -func buildSentinelCommand(rebootSentinelFile string, rebootSentinelCommand string) []string { - if rebootSentinelCommand != "" { - cmd, err := shlex.Split(rebootSentinelCommand) - if err != nil { - log.Fatalf("Error parsing provided sentinel command: %v", err) - } - return cmd - } - return []string{"test", "-f", rebootSentinelFile} -} - func root(cmd *cobra.Command, args []string) { if logFormat == "json" { log.SetFormatter(&log.JSONFormatter{}) @@ -817,8 +782,6 @@ func root(cmd *cobra.Command, args []string) { log.Fatalf("Failed to build time window: %v", err) } - sentinelCommand := buildSentinelCommand(rebootSentinelFile, rebootSentinelCommand) - log.Infof("Node ID: %s", nodeID) log.Infof("Lock Annotation: %s/%s:%s", dsNamespace, dsName, lockAnnotation) if lockTTL > 0 { @@ -834,7 +797,7 @@ func root(cmd *cobra.Command, args []string) { log.Infof("PreferNoSchedule taint: %s", preferNoScheduleTaintName) log.Infof("Blocking Pod Selectors: %v", podSelectors) log.Infof("Reboot schedule: %v", window) - log.Infof("Reboot check command: %s every %v", sentinelCommand, period) + log.Infof("Reboot period %v", period) log.Infof("Concurrency: %v", concurrency) log.Infof("Reboot method: %s", rebootMethod) @@ -855,18 +818,31 @@ func root(cmd *cobra.Command, args []string) { log.Fatalf("Invalid reboot-method configured: %s", rebootMethod) } + var checker checkers.Checker + // An override of rebootsentinelcommand means a privileged command + if rebootSentinelCommand != "" { + log.Infof("Sentinel checker is user provided command: %s", rebootSentinelCommand) + cmd, err := shlex.Split(rebootSentinelCommand) + if err != nil { + log.Fatalf("Error parsing provided sentinel command: %v", err) + } + checker = checkers.NsEnterRebootChecker{ + CustomCheckCommand: cmd, + NamespacePid: 1, + } + } else { + log.Infof("Sentinel checker is (unprivileged) testing for the presence of: %s", rebootSentinelFile) + checker = checkers.UnprivilegedRebootChecker{ + CheckCommand: []string{"test", "-f", rebootSentinelFile}, + } + } + 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 - if rebootSentinelCommand != "" { - hostSentinelCommand = util.PrivilegedHostCommand(1, sentinelCommand) - } - - go rebootAsRequired(nodeID, rebooter, hostSentinelCommand, window, lockTTL, lockReleaseDelay) - go maintainRebootRequiredMetric(nodeID, hostSentinelCommand) + go rebootAsRequired(nodeID, rebooter, checker, window, lockTTL, lockReleaseDelay) + go maintainRebootRequiredMetric(nodeID, checker) http.Handle("/metrics", promhttp.Handler()) log.Fatal(http.ListenAndServe(fmt.Sprintf("%s:%d", metricsHost, metricsPort), nil)) diff --git a/cmd/kured/main_test.go b/cmd/kured/main_test.go index 2659ceb..542698c 100644 --- a/cmd/kured/main_test.go +++ b/cmd/kured/main_test.go @@ -1,30 +1,11 @@ package main import ( - "github.com/kubereboot/kured/pkg/util" + "github.com/spf13/cobra" "reflect" "testing" - - "github.com/kubereboot/kured/pkg/alerts" - log "github.com/sirupsen/logrus" - "github.com/spf13/cobra" - assert "gotest.tools/v3/assert" - - papi "github.com/prometheus/client_golang/api" ) -type BlockingChecker struct { - blocking bool -} - -func (fbc BlockingChecker) isBlocked() bool { - return fbc.blocking -} - -var _ RebootBlocker = BlockingChecker{} // Verify that Type implements Interface. -var _ RebootBlocker = (*BlockingChecker)(nil) // Verify that *Type implements Interface. - - func Test_flagCheck(t *testing.T) { var cmd *cobra.Command var args []string @@ -108,156 +89,3 @@ func Test_stripQuotes(t *testing.T) { }) } } - - -func Test_rebootBlocked(t *testing.T) { - noCheckers := []RebootBlocker{} - nonblockingChecker := BlockingChecker{blocking: false} - blockingChecker := BlockingChecker{blocking: true} - - // Instantiate a prometheusClient with a broken_url - promClient, err := alerts.NewPromClient(papi.Config{Address: "broken_url"}) - if err != nil { - log.Fatal("Can't create prometheusClient: ", err) - } - brokenPrometheusClient := PrometheusBlockingChecker{promClient: promClient, filter: nil, firingOnly: false} - - type args struct { - blockers []RebootBlocker - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "Do not block on no blocker defined", - args: args{blockers: noCheckers}, - want: false, - }, - { - name: "Ensure a blocker blocks", - args: args{blockers: []RebootBlocker{blockingChecker}}, - want: true, - }, - { - name: "Ensure a non-blocker doesn't block", - args: args{blockers: []RebootBlocker{nonblockingChecker}}, - want: false, - }, - { - name: "Ensure one blocker is enough to block", - args: args{blockers: []RebootBlocker{nonblockingChecker, blockingChecker}}, - want: true, - }, - { - name: "Do block on error contacting prometheus API", - args: args{blockers: []RebootBlocker{brokenPrometheusClient}}, - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := rebootBlocked(tt.args.blockers...); got != tt.want { - t.Errorf("rebootBlocked() = %v, want %v", got, tt.want) - } - }) - } -} - - -func Test_buildSentinelCommand(t *testing.T) { - type args struct { - rebootSentinelFile string - rebootSentinelCommand string - } - tests := []struct { - name string - args args - want []string - }{ - { - name: "Ensure a sentinelFile generates a shell 'test' command with the right file", - args: args{ - rebootSentinelFile: "/test1", - rebootSentinelCommand: "", - }, - want: []string{"test", "-f", "/test1"}, - }, - { - name: "Ensure a sentinelCommand has priority over a sentinelFile if both are provided (because sentinelFile is always provided)", - args: args{ - rebootSentinelFile: "/test1", - rebootSentinelCommand: "/sbin/reboot-required -r", - }, - want: []string{"/sbin/reboot-required", "-r"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := buildSentinelCommand(tt.args.rebootSentinelFile, tt.args.rebootSentinelCommand); !reflect.DeepEqual(got, tt.want) { - t.Errorf("buildSentinelCommand() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_rebootRequired(t *testing.T) { - type args struct { - sentinelCommand []string - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "Ensure rc = 0 means reboot required", - args: args{ - sentinelCommand: []string{"true"}, - }, - want: true, - }, - { - name: "Ensure rc != 0 means reboot NOT required", - args: args{ - sentinelCommand: []string{"false"}, - }, - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := rebootRequired(tt.args.sentinelCommand); got != tt.want { - t.Errorf("rebootRequired() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_rebootRequired_fatals(t *testing.T) { - cases := []struct { - param []string - expectFatal bool - }{ - { - param: []string{"true"}, - expectFatal: false, - }, - { - param: []string{"./babar"}, - expectFatal: true, - }, - } - - defer func() { log.StandardLogger().ExitFunc = nil }() - var fatal bool - log.StandardLogger().ExitFunc = func(int) { fatal = true } - - for _, c := range cases { - fatal = false - rebootRequired(c.param) - assert.Equal(t, c.expectFatal, fatal) - } - -} diff --git a/pkg/checkers/checker.go b/pkg/checkers/checker.go new file mode 100644 index 0000000..3a0fc01 --- /dev/null +++ b/pkg/checkers/checker.go @@ -0,0 +1,74 @@ +package checkers + +import ( + "github.com/kubereboot/kured/pkg/util" + log "github.com/sirupsen/logrus" + "os/exec" +) + +type Checker interface { + CheckRebootRequired() bool +} + +// UnprivilegedRebootChecker is the default reboot checker. +// It is unprivileged, and tests the presence of a files +type UnprivilegedRebootChecker struct { + CheckCommand []string +} + +// CheckRebootRequired runs the test command of the file +// needs refactoring to also return an error, instead of leaking it inside the code. +// This needs refactoring to get rid of NewCommand +// This needs refactoring to only contain file location, instead of CheckCommand +func (rc UnprivilegedRebootChecker) CheckRebootRequired() bool { + cmd := util.NewCommand(rc.CheckCommand[0], rc.CheckCommand[1:]...) + if err := cmd.Run(); err != nil { + switch err := err.(type) { + case *exec.ExitError: + // We assume a non-zero exit code means 'reboot not required', but of course + // the user could have misconfigured the sentinel command or something else + // went wrong during its execution. In that case, not entering a reboot loop + // is the right thing to do, and we are logging stdout/stderr of the command + // so it should be obvious what is wrong. + if cmd.ProcessState.ExitCode() != 1 { + log.Warnf("sentinel command ended with unexpected exit code: %v", cmd.ProcessState.ExitCode()) + } + return false + default: + // Something was grossly misconfigured, such as the command path being wrong. + log.Fatalf("Error invoking sentinel command: %v", err) + } + } + return true +} + +// NsEnterRebootChecker is using a custom command to check +// if a reboot is required, but therefore needs a pid for entering the namespace, +// on top of the required command. This requires elevation. +type NsEnterRebootChecker struct { + CustomCheckCommand []string + NamespacePid int +} + +func (rc NsEnterRebootChecker) CheckRebootRequired() bool { + privCommand := util.PrivilegedHostCommand(rc.NamespacePid, rc.CustomCheckCommand) + cmd := util.NewCommand(privCommand[0], privCommand[1:]...) + if err := cmd.Run(); err != nil { + switch err := err.(type) { + case *exec.ExitError: + // We assume a non-zero exit code means 'reboot not required', but of course + // the user could have misconfigured the sentinel command or something else + // went wrong during its execution. In that case, not entering a reboot loop + // is the right thing to do, and we are logging stdout/stderr of the command + // so it should be obvious what is wrong. + if cmd.ProcessState.ExitCode() != 1 { + log.Warnf("sentinel command ended with unexpected exit code: %v", cmd.ProcessState.ExitCode()) + } + return false + default: + // Something was grossly misconfigured, such as the command path being wrong. + log.Fatalf("Error invoking sentinel command: %v", err) + } + } + return true +} diff --git a/pkg/checkers/checker_test.go b/pkg/checkers/checker_test.go new file mode 100644 index 0000000..f3a6894 --- /dev/null +++ b/pkg/checkers/checker_test.go @@ -0,0 +1,69 @@ +package checkers + +import ( + log "github.com/sirupsen/logrus" + assert "gotest.tools/v3/assert" + "testing" +) + +func Test_rebootRequired(t *testing.T) { + type args struct { + sentinelCommand []string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "Ensure rc = 0 means reboot required", + args: args{ + sentinelCommand: []string{"true"}, + }, + want: true, + }, + { + name: "Ensure rc != 0 means reboot NOT required", + args: args{ + sentinelCommand: []string{"false"}, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := UnprivilegedRebootChecker{CheckCommand: tt.args.sentinelCommand} + if got := a.CheckRebootRequired(); got != tt.want { + t.Errorf("rebootRequired() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_rebootRequired_fatals(t *testing.T) { + cases := []struct { + param []string + expectFatal bool + }{ + { + param: []string{"true"}, + expectFatal: false, + }, + { + param: []string{"./babar"}, + expectFatal: true, + }, + } + + defer func() { log.StandardLogger().ExitFunc = nil }() + var fatal bool + log.StandardLogger().ExitFunc = func(int) { fatal = true } + + for _, c := range cases { + fatal = false + a := UnprivilegedRebootChecker{CheckCommand: c.param} + a.CheckRebootRequired() + assert.Equal(t, c.expectFatal, fatal) + } + +}