diff --git a/Makefile b/Makefile index 33be81823..85a18b713 100644 --- a/Makefile +++ b/Makefile @@ -375,7 +375,7 @@ vet: $(Q)$(GO_VET) ./... golangci-lint: - $(Q)$(GOLANG_CILINT) run + $(Q)$(GOLANG_CILINT) run $(GOLANGCI_LINT_TARGET) verify-godeps: $(Q) $(GO_CMD) mod tidy && git diff --quiet; ec="$$?"; \ diff --git a/cmd/plugins/memory-qos/main.go b/cmd/plugins/memory-qos/main.go index 98c06a901..ad5dd7d11 100644 --- a/cmd/plugins/memory-qos/main.go +++ b/cmd/plugins/memory-qos/main.go @@ -32,7 +32,6 @@ import ( type plugin struct { stub stub.Stub - mask stub.EventMask config *pluginConfig } @@ -216,7 +215,7 @@ func (p *plugin) CreateContainer(ctx context.Context, pod *api.PodSandbox, ctr * return nil, nil, errWithContext } class = value - case sliceContains(p.config.UnifiedAnnotations, annPrefix) == true: + case sliceContains(p.config.UnifiedAnnotations, annPrefix): unified[annPrefix] = value log.Tracef("applying unified annotation %q resulted in unified=%v", annPrefix, unified) default: diff --git a/cmd/plugins/memtierd/main.go b/cmd/plugins/memtierd/main.go index 17a8e2171..704a0f1e1 100644 --- a/cmd/plugins/memtierd/main.go +++ b/cmd/plugins/memtierd/main.go @@ -36,7 +36,6 @@ import ( type plugin struct { stub stub.Stub - mask stub.EventMask config *pluginConfig cgroupsDir string ctrMemtierdEnv map[string]*memtierdEnv @@ -73,7 +72,6 @@ type qosClass struct { } type memtierdEnv struct { - pid int ctrDir string configFile string outputFile string @@ -314,7 +312,11 @@ func (p *plugin) StopContainer(ctx context.Context, pod *api.PodSandbox, ctr *ap log.Debugf("StopContainer: killing memtierd of %s (pid: %d) failed: %s", ppName, pid, err) } // Close files, read exit status (leave no zombie processes behind) - go mtdEnv.cmd.Wait() + go func() { + if err := mtdEnv.cmd.Wait(); err != nil { + log.Errorf("StopContainer: waiting for memtierd of %s (pid: %d) failed: %s", ppName, pid, err) + } + }() } log.Tracef("StopContainer: removing memtierd run directory %s", mtdEnv.ctrDir) @@ -438,9 +440,11 @@ func (me *memtierdEnv) startMemtierd() error { return fmt.Errorf("failed to start command %s: %q", cmd, err) } if cmd.Process != nil { - os.WriteFile(me.pidFile, + if err := os.WriteFile(me.pidFile, []byte(fmt.Sprintf("%d\n", cmd.Process.Pid)), - 0400) + 0400); err != nil { + log.Warnf("failed to write PID file %q: %s", me.pidFile, err) + } } me.cmd = cmd return nil diff --git a/cmd/plugins/template/policy/template-policy.go b/cmd/plugins/template/policy/template-policy.go index 79464fbeb..9333a594b 100644 --- a/cmd/plugins/template/policy/template-policy.go +++ b/cmd/plugins/template/policy/template-policy.go @@ -131,17 +131,10 @@ func (p *policy) ExportResourceData(c cache.Container) map[string]string { return nil } -// Initialize or reinitialize the policy. -func (p *policy) initialize() error { - return nil -} - type NoMetrics struct{} func (*NoMetrics) Describe(chan<- *prometheus.Desc) { - return } func (*NoMetrics) Collect(chan<- prometheus.Metric) { - return } diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 660e083f0..9f63cbae9 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -212,10 +212,10 @@ func (a *Agent) Start(notifyFn NotifyFn) error { break } if e.Type == watch.Added || e.Type == watch.Modified { - group, _ := e.Object.(*corev1.Node).Labels[a.groupLabel] + group := e.Object.(*corev1.Node).Labels[a.groupLabel] if group == "" { for _, l := range deprecatedGroupLabels { - group, _ = e.Object.(*corev1.Node).Labels[l] + group = e.Object.(*corev1.Node).Labels[l] if group != "" { log.Warnf("Using DEPRECATED config group label %q", l) log.Warnf("Please switch to using label %q instead", a.groupLabel) @@ -259,7 +259,7 @@ func (a *Agent) Stop() { if a.stopC != nil { close(a.stopC) - _ = <-a.doneC + <-a.doneC a.stopC = nil } } diff --git a/pkg/agent/podresapi/client.go b/pkg/agent/podresapi/client.go index e9a2e4909..fc946cbba 100644 --- a/pkg/agent/podresapi/client.go +++ b/pkg/agent/podresapi/client.go @@ -17,9 +17,7 @@ package podresapi import ( "context" "fmt" - "net" "strings" - "time" logger "github.com/containers/nri-plugins/pkg/log" "google.golang.org/grpc" @@ -43,7 +41,6 @@ const ( // these constants were obtained from NFD sources, cross-checked against // https://github.com/kubernetes/kubernetes/blob/release-1.31/test/e2e_node/util.go#L83 defaultSocketPath = "/var/lib/kubelet/pod-resources/kubelet.sock" - timeout = 10 * time.Second maxSize = 1024 * 1024 * 16 ) @@ -77,20 +74,9 @@ func NewClient(options ...ClientOption) (*Client, error) { } if c.conn == nil { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - dialer := func(ctx context.Context, addr string) (net.Conn, error) { - return (&net.Dialer{}).DialContext(ctx, "unix", addr) - } - - conn, err := grpc.DialContext( - ctx, - c.socketPath, + conn, err := grpc.NewClient("unix://"+c.socketPath, grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithContextDialer(dialer), grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(maxSize)), - grpc.WithBlock(), ) if err != nil { diff --git a/pkg/agent/watch/file.go b/pkg/agent/watch/file.go index a6fcd10b1..c8cc610db 100644 --- a/pkg/agent/watch/file.go +++ b/pkg/agent/watch/file.go @@ -87,7 +87,7 @@ func (w *FileWatch) Stop() { if w.stopC != nil { close(w.stopC) - _ = <-w.doneC + <-w.doneC w.stopC = nil } } diff --git a/pkg/agent/watch/object.go b/pkg/agent/watch/object.go index 3e906abd7..e7eb9e4a2 100644 --- a/pkg/agent/watch/object.go +++ b/pkg/agent/watch/object.go @@ -38,7 +38,6 @@ type ObjectWatch struct { name string resultC chan Event wif watch.Interface - pending *Event reopenC <-chan time.Time failing bool @@ -73,7 +72,7 @@ func (w *ObjectWatch) Stop() { if w.stopC != nil { close(w.stopC) - _ = <-w.doneC + <-w.doneC w.stopC = nil } } diff --git a/pkg/apis/config/v1alpha1/resmgr/control/config.go b/pkg/apis/config/v1alpha1/resmgr/control/config.go index 52a35742c..7742de79a 100644 --- a/pkg/apis/config/v1alpha1/resmgr/control/config.go +++ b/pkg/apis/config/v1alpha1/resmgr/control/config.go @@ -21,5 +21,5 @@ import ( // +k8s:deepcopy-gen=true type Config struct { // +optional - CPU *cpu.Config `json:"cpu",omitempty"` + CPU *cpu.Config `json:"cpu,omitempty"` } diff --git a/pkg/apis/resmgr/v1alpha1/expression.go b/pkg/apis/resmgr/v1alpha1/expression.go index 3e297fbf5..b7fbb0466 100644 --- a/pkg/apis/resmgr/v1alpha1/expression.go +++ b/pkg/apis/resmgr/v1alpha1/expression.go @@ -46,7 +46,7 @@ func (e *Expression) Validate() error { return exprError("invalid expression, '%s' requires a single value", e.Op) } case Exists, NotExist: - if e.Values != nil && len(e.Values) != 0 { + if len(e.Values) != 0 { return exprError("invalid expression, '%s' does not take any values", e.Op) } @@ -54,7 +54,7 @@ func (e *Expression) Validate() error { case MatchesAny, MatchesNone: case AlwaysTrue: - if e.Values != nil && len(e.Values) != 0 { + if len(e.Values) != 0 { return exprError("invalid expression, '%s' does not take any values", e.Op) } diff --git a/pkg/cgroups/cgroupblkio.go b/pkg/cgroups/cgroupblkio.go index 5e60662f9..686950948 100644 --- a/pkg/cgroups/cgroupblkio.go +++ b/pkg/cgroups/cgroupblkio.go @@ -17,7 +17,6 @@ package cgroups import ( "errors" "fmt" - "io/ioutil" "os" "path/filepath" "strconv" @@ -341,7 +340,7 @@ var currentPlatform platformInterface = defaultPlatform{} // readFromFile returns file contents as a string. func (dpm defaultPlatform) readFromFile(filename string) (string, error) { - content, err := ioutil.ReadFile(filename) + content, err := os.ReadFile(filename) return string(content), err } @@ -352,6 +351,6 @@ func (dpm defaultPlatform) writeToFile(filename string, content string) error { return err } defer f.Close() - _, err = fmt.Fprintf(f, content) + _, err = fmt.Fprintf(f, "%s", content) return err } diff --git a/pkg/cgroups/cgroupstats.go b/pkg/cgroups/cgroupstats.go index bd9c38dc9..87f2f4572 100644 --- a/pkg/cgroups/cgroupstats.go +++ b/pkg/cgroups/cgroupstats.go @@ -16,7 +16,7 @@ package cgroups import ( "fmt" - "io/ioutil" + "os" "path" "path/filepath" "strconv" @@ -89,7 +89,7 @@ type GlobalNumaStats struct { func readCgroupFileLines(filePath string) ([]string, error) { - f, err := ioutil.ReadFile(filePath) + f, err := os.ReadFile(filePath) if err != nil { return nil, err diff --git a/pkg/cgroupstats/collector.go b/pkg/cgroupstats/collector.go index b0db8c72a..ec89287b2 100644 --- a/pkg/cgroupstats/collector.go +++ b/pkg/cgroupstats/collector.go @@ -282,7 +282,7 @@ func walkCgroups() []string { containerDirs := []string{} cpuset := filepath.Join(cgroupRoot, "cpuset") - filepath.Walk(filepath.Join(cpuset, kubepodsDir), + err := filepath.Walk(filepath.Join(cpuset, kubepodsDir), func(path string, info os.FileInfo, err error) error { if err != nil { if os.IsNotExist(err) { @@ -315,6 +315,9 @@ func walkCgroups() []string { return nil }) + if err != nil { + log.Warnf("cgroupfs walk failed: %v", err) + } return containerDirs } diff --git a/pkg/cpuallocator/allocator.go b/pkg/cpuallocator/allocator.go index ad6e59a58..46f983154 100644 --- a/pkg/cpuallocator/allocator.go +++ b/pkg/cpuallocator/allocator.go @@ -57,9 +57,6 @@ type allocatorHelper struct { prefer CPUPriority // CPU priority to prefer cnt int // number of CPUs to allocate result cpuset.CPUSet // set of CPUs allocated - - pkgs []sysfs.CPUPackage // physical CPU packages, sorted by preference - cpus []sysfs.CPU // CPU cores, sorted by preference } // CPUAllocator is an interface for a generic CPU allocator @@ -942,7 +939,6 @@ func (a *allocatorHelper) takeCacheGroups() { a.result = result a.from = from a.cnt = 0 - return } // Allocate full idle CPU cores. diff --git a/pkg/cpuallocator/cpuallocator_test.go b/pkg/cpuallocator/cpuallocator_test.go index c4dd6dc3b..a6ac4b668 100644 --- a/pkg/cpuallocator/cpuallocator_test.go +++ b/pkg/cpuallocator/cpuallocator_test.go @@ -15,7 +15,6 @@ package cpuallocator import ( - "io/ioutil" "os" "path" "testing" @@ -30,7 +29,7 @@ import ( func TestAllocatorHelper(t *testing.T) { // Create tmpdir and decompress testdata there - tmpdir, err := ioutil.TempDir("", "nri-resource-policy-test-") + tmpdir, err := os.MkdirTemp("", "nri-resource-policy-test-") if err != nil { t.Fatalf("failed to create tmpdir: %v", err) } @@ -108,7 +107,7 @@ func TestClusteredAllocation(t *testing.T) { } // Create tmpdir and decompress testdata there - tmpdir, err := ioutil.TempDir("", "nri-resource-policy-test-") + tmpdir, err := os.MkdirTemp("", "nri-resource-policy-test-") if err != nil { t.Fatalf("failed to create tmpdir: %v", err) } @@ -324,7 +323,7 @@ func TestClusteredCoreKindAllocation(t *testing.T) { } // Create tmpdir and decompress testdata there - tmpdir, err := ioutil.TempDir("", "nri-resource-policy-test-") + tmpdir, err := os.MkdirTemp("", "nri-resource-policy-test-") if err != nil { t.Fatalf("failed to create tmpdir: %v", err) } diff --git a/pkg/healthz/healthz.go b/pkg/healthz/healthz.go index 0cd1657ca..34373d941 100644 --- a/pkg/healthz/healthz.go +++ b/pkg/healthz/healthz.go @@ -54,14 +54,20 @@ func serve(w http.ResponseWriter, req *http.Request) { status, details := check() if status == Healthy { w.WriteHeader(200) - w.Write([]byte("ok")) + _, err := w.Write([]byte("ok")) + if err != nil { + log.Errorf("failed to write response: %v", err) + } } else { errors := "" for _, err := range details { errors += fmt.Sprintf("%v\n", err) } w.WriteHeader(500) - w.Write([]byte(errors)) + _, err := w.Write([]byte(errors)) + if err != nil { + log.Errorf("failed to write response: %v", err) + } } } diff --git a/pkg/http/http.go b/pkg/http/http.go index 527f22fa7..5f3d751a9 100644 --- a/pkg/http/http.go +++ b/pkg/http/http.go @@ -161,7 +161,12 @@ func (s *Server) Start(addr string) error { s.server.Addr = ln.Addr().String() } - go s.server.Serve(ln) + go func() { + err := s.server.Serve(ln) + if err != nil && err != http.ErrServerClosed { + log.Warn("HTTP server exited with error: %v", err) + } + }() return nil } @@ -200,8 +205,10 @@ func (s *Server) Shutdown(wait bool) { close(sync) }) } - s.server.Shutdown(context.Background()) - _ = <-sync + if err := s.server.Shutdown(context.Background()); err != nil && err != http.ErrServerClosed { + log.Warnf("failed to shutdown server: %v", err) + } + <-sync s.server = nil } diff --git a/pkg/http/http_test.go b/pkg/http/http_test.go index 09ddf7e00..a67044859 100644 --- a/pkg/http/http_test.go +++ b/pkg/http/http_test.go @@ -16,7 +16,7 @@ package http import ( "fmt" - "io/ioutil" + "io" "net/http" "testing" ) @@ -48,12 +48,6 @@ func TestStartStop(t *testing.T) { srv.Stop() } -type urlTest struct { - pattern string - response string - fallback string -} - func checkURL(t *testing.T, srv *Server, path, response string, status int) { url := "http://" + srv.GetAddress() + path @@ -66,7 +60,7 @@ func checkURL(t *testing.T, srv *Server, path, response string, status int) { t.Errorf("http.Get(%s) status %d, expected %d", url, res.StatusCode, status) } - txt, err := ioutil.ReadAll(res.Body) + txt, err := io.ReadAll(res.Body) if err != nil { t.Errorf("http.Get(%s) failed to read response: %v", url, err) } diff --git a/pkg/instrumentation/instrumentation_test.go b/pkg/instrumentation/instrumentation_test.go index efa609839..e274bcf5c 100644 --- a/pkg/instrumentation/instrumentation_test.go +++ b/pkg/instrumentation/instrumentation_test.go @@ -15,11 +15,13 @@ package instrumentation import ( - "io/ioutil" + "io" "net/http" "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestPrometheusConfiguration(t *testing.T) { @@ -29,7 +31,7 @@ func TestPrometheusConfiguration(t *testing.T) { cfg.HTTPEndpoint = ":0" } - Start() + require.NoError(t, Start(), "start test server") address := srv.GetAddress() if strings.HasSuffix(cfg.HTTPEndpoint, ":0") { @@ -40,17 +42,17 @@ func TestPrometheusConfiguration(t *testing.T) { newCfg := *cfg newCfg.PrometheusExport = !newCfg.PrometheusExport - Reconfigure(&newCfg) + require.NoError(t, Reconfigure(&newCfg), "reconfigure test server") checkPrometheus(t, address, !newCfg.PrometheusExport) newCfg = *cfg newCfg.PrometheusExport = !newCfg.PrometheusExport - Reconfigure(&newCfg) + require.NoError(t, Reconfigure(&newCfg), "reconfigure test server") checkPrometheus(t, address, !newCfg.PrometheusExport) newCfg = *cfg newCfg.PrometheusExport = !newCfg.PrometheusExport - Reconfigure(&newCfg) + require.NoError(t, Reconfigure(&newCfg), "reconfigure test server") checkPrometheus(t, address, !newCfg.PrometheusExport) srv.Shutdown(true) @@ -73,7 +75,7 @@ func checkPrometheus(t *testing.T, server string, shouldFail bool) { return } - _, err = ioutil.ReadAll(rpl.Body) + _, err = io.ReadAll(rpl.Body) rpl.Body.Close() if err != nil { t.Errorf("failed to read Prometheus response: %v", err) diff --git a/pkg/instrumentation/metrics/metrics.go b/pkg/instrumentation/metrics/metrics.go index a036156ca..fbd387df4 100644 --- a/pkg/instrumentation/metrics/metrics.go +++ b/pkg/instrumentation/metrics/metrics.go @@ -34,7 +34,7 @@ type ( var ( disabled bool - namespace string + namespace = "nri" enabled []string polled []string reportPeriod time.Duration @@ -99,7 +99,7 @@ func Start(m *http.ServeMux, options ...Option) error { log.Info("starting metrics exporter...") g, err := metrics.NewGatherer( - metrics.WithNamespace("nri"), + metrics.WithNamespace(namespace), metrics.WithPollInterval(reportPeriod), metrics.WithMetrics(enabled, polled), ) diff --git a/pkg/instrumentation/tracing/exporter.go b/pkg/instrumentation/tracing/exporter.go index 084335bba..437b0a2fb 100644 --- a/pkg/instrumentation/tracing/exporter.go +++ b/pkg/instrumentation/tracing/exporter.go @@ -59,7 +59,9 @@ func (e *spanExporter) Shutdown(ctx context.Context) error { } func (e *spanExporter) setEndpoint(endpoint string) error { - e.shutdown() + if err := e.shutdown(); err != nil { + log.Warnf("failed to shutdown tracing exporter: %v", err) + } if endpoint == "" { return nil diff --git a/pkg/log/config.go b/pkg/log/config.go index 5b9f96eab..2242a88f3 100644 --- a/pkg/log/config.go +++ b/pkg/log/config.go @@ -31,8 +31,6 @@ const ( debugEnvVar = "LOGGER_DEBUG" // logSourceEnvVar is the environment variable used to seed source logging. logSourceEnvVar = "LOGGER_LOG_SOURCE" - // configModule is our module name in the runtime configuration. - configModule = "logger" ) // srcmap tracks debugging settings for sources. diff --git a/pkg/log/klogcontrol/klogcontrol.go b/pkg/log/klogcontrol/klogcontrol.go index 3b52294ac..1765f23ef 100644 --- a/pkg/log/klogcontrol/klogcontrol.go +++ b/pkg/log/klogcontrol/klogcontrol.go @@ -18,7 +18,7 @@ import ( "errors" "flag" "fmt" - "io/ioutil" + "io" "os" "strings" @@ -69,7 +69,7 @@ func klogError(format string, args ...interface{}) error { // init discovers klog flags and sets up dynamic control for them. func init() { - ctl.SetOutput(ioutil.Discard) + ctl.SetOutput(io.Discard) klog.InitFlags(ctl.FlagSet) ctl.VisitAll(func(f *flag.Flag) { if name, value, ok := getEnvForFlag(f.Name); ok { @@ -84,7 +84,10 @@ func init() { if f.Name == "skip_headers" { if value, _ := os.LookupEnv("JOURNAL_STREAM"); value != "" { klog.Infof("Logging to journald, forcing headers off...") - ctl.Set(f.Name, "true") + err := ctl.Set(f.Name, "true") + if err != nil { + klog.Warningf("failed to set klog flag %s to true: %v", f.Name, err) + } } } } diff --git a/pkg/log/log.go b/pkg/log/log.go index 3f44e9698..ef736ec88 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -161,7 +161,10 @@ func DebugEnabled(source string) bool { func SetLevel(level Level) { log.Lock() defer log.Unlock() - log.setLevel(level) + err := log.setLevel(level) + if err != nil { + Default().Warn("failed to set log level to %v: %v", level, err) + } } // Flush flushes any pending log messages. diff --git a/pkg/log/signal.go b/pkg/log/signal.go index c7302d85d..62fb94340 100644 --- a/pkg/log/signal.go +++ b/pkg/log/signal.go @@ -35,12 +35,7 @@ func SetupDebugToggleSignal(sig os.Signal) { go func(sig <-chan os.Signal) { state := map[bool]string{false: "off", true: "on"} for { - select { - case _, ok := <-sig: - if !ok { - return - } - } + <-sig log.forced = !log.forced deflog.Warn("forced full debugging is now %s...", state[log.forced]) } diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 24bcdd7ca..8ba988914 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -163,11 +163,17 @@ func (c *Collector) Matches(glob string) bool { } ok, err := path.Match(glob, c.group) + if err != nil { + log.Warnf("invalid glob pattern %q (group %s): %v", glob, c.group, err) + } if ok { return true } ok, err = path.Match(glob, c.name) + if err != nil { + log.Warnf("invalid glob pattern %q (name %s): %v", glob, c.name, err) + } if ok { return true } @@ -652,7 +658,7 @@ func (g *Gatherer) poller() { g.ticker = nil close(doneCh) return - case _ = <-g.ticker.C: + case <-g.ticker.C: g.Poll() } } @@ -668,7 +674,7 @@ func (g *Gatherer) Stop() { doneCh := make(chan struct{}) g.stopCh <- doneCh - _ = <-doneCh + <-doneCh g.stopCh = nil } diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index cc39fdc0b..2558411b8 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -17,6 +17,7 @@ package metrics_test import ( "bufio" "context" + "fmt" "net/http" "strings" "testing" @@ -350,7 +351,6 @@ func (c collected) GetValue(name string) string { type testServer struct { srv *http.Server - mux *http.ServeMux r *metrics.Registry g *metrics.Gatherer } @@ -389,7 +389,10 @@ func newTestServer(t *testing.T, r *metrics.Registry, enabled, polled []string, func (srv *testServer) stop() { if srv.srv != nil { - srv.srv.Shutdown(context.Background()) + err := srv.srv.Shutdown(context.Background()) + if err != nil && err != http.ErrServerClosed { + fmt.Printf("test server shutdown failed: %v\n", err) + } } if srv.g != nil { srv.g.Stop() @@ -422,7 +425,3 @@ func (srv *testServer) collect(t *testing.T) (described, collected) { return described(types), collected(metrics) } - -func (srv *testServer) poll() { - srv.r.Poll() -} diff --git a/pkg/pidfile/pidfile.go b/pkg/pidfile/pidfile.go index b5b3cb38b..59f5a0cd3 100644 --- a/pkg/pidfile/pidfile.go +++ b/pkg/pidfile/pidfile.go @@ -21,6 +21,8 @@ import ( "strconv" "strings" "syscall" + + logger "github.com/containers/nri-plugins/pkg/log" ) var ( @@ -93,7 +95,9 @@ func Read() (int, error) { // close closes the PID file and truncates it to zero length. func close() { if pidFile != nil { - pidFile.Truncate(0) + if err := pidFile.Truncate(0); err != nil { + logger.Default().Warnf("failed to truncate PID file: %v\n", err) + } pidFile.Close() pidFile = nil } diff --git a/pkg/pidfile/pidfile_test.go b/pkg/pidfile/pidfile_test.go index d969ed992..2ddbd3f94 100644 --- a/pkg/pidfile/pidfile_test.go +++ b/pkg/pidfile/pidfile_test.go @@ -16,7 +16,6 @@ package pidfile import ( "fmt" - "io/ioutil" "os" "path/filepath" "testing" @@ -46,7 +45,7 @@ func TestDefaults(t *testing.T) { err error ) - Remove() + require.NoError(t, Remove()) err = Write() require.Nil(t, err) @@ -66,7 +65,7 @@ func TestDefaults(t *testing.T) { err = Write() require.NotNil(t, err) - Remove() + require.NoError(t, Remove()) err = Write() require.Nil(t, err) @@ -242,7 +241,7 @@ func TestOwnerPid(t *testing.T) { } func mkTestDir(t *testing.T) (string, error) { - tmp, err := ioutil.TempDir("", ".pidfile-test*") + tmp, err := os.MkdirTemp("", ".pidfile-test*") if err != nil { return "", fmt.Errorf("failed to create test directory: %w", err) } diff --git a/pkg/resmgr/cache/cache.go b/pkg/resmgr/cache/cache.go index dc0982617..cee39af62 100644 --- a/pkg/resmgr/cache/cache.go +++ b/pkg/resmgr/cache/cache.go @@ -539,7 +539,9 @@ func (cch *cache) ResetActivePolicy() error { func (cch *cache) InsertPod(nriPod *nri.PodSandbox, ch <-chan *podresapi.PodResources) Pod { p := cch.createPod(nriPod, ch) cch.Pods[nriPod.GetId()] = p - cch.Save() + if err := cch.Save(); err != nil { + log.Warnf("failed to save cache: %v", err) + } return p } @@ -554,7 +556,9 @@ func (cch *cache) DeletePod(id string) Pod { log.Debug("removing pod %s (%s)", p.PrettyName(), p.GetID()) delete(cch.Pods, id) - cch.Save() + if err := cch.Save(); err != nil { + log.Warnf("failed to save cache: %v", err) + } return p } @@ -569,18 +573,18 @@ func (cch *cache) LookupPod(id string) (Pod, bool) { func (cch *cache) InsertContainer(ctr *nri.Container) (Container, error) { var err error - c := &container{ - cache: cch, - } - - c, err = cch.createContainer(ctr) + c, err := cch.createContainer(ctr) if err != nil { return nil, cacheError("failed to insert container %s: %v", c.GetID(), err) } cch.Containers[c.GetID()] = c - cch.createContainerDirectory(c.GetID()) - cch.Save() + if err := cch.createContainerDirectory(c.GetID()); err != nil { + log.Warnf("failed to create container directory for %s: %v", c.GetID(), err) + } + if err := cch.Save(); err != nil { + log.Warnf("failed to save cache: %v", err) + } return c, nil } @@ -593,10 +597,14 @@ func (cch *cache) DeleteContainer(id string) Container { } log.Debug("removing container %s", c.PrettyName()) - cch.removeContainerDirectory(c.GetID()) + if err := cch.removeContainerDirectory(c.GetID()); err != nil { + log.Warnf("failed to remove container directory for %s: %v", c.GetID(), err) + } delete(cch.Containers, c.GetID()) - cch.Save() + if err := cch.Save(); err != nil { + log.Warnf("failed to save cache: %v", err) + } return c } @@ -624,7 +632,7 @@ func (cch *cache) LookupContainerByCgroup(path string) (Container, bool) { continue } - if strings.Index(path, c.GetID()) != -1 { + if strings.Contains(path, c.GetID()) { return c, true } } @@ -827,12 +835,12 @@ func (cch *cache) GetPolicyEntry(key string, ptr interface{}) bool { // Marshal an opaque policy entry, special-casing cpusets and maps of cpusets. func marshalEntry(obj interface{}) ([]byte, error) { - switch obj.(type) { + switch obj := obj.(type) { case cpuset.CPUSet: - return []byte("\"" + obj.(cpuset.CPUSet).String() + "\""), nil + return []byte("\"" + obj.String() + "\""), nil case map[string]cpuset.CPUSet: dst := make(map[string]string) - for key, cset := range obj.(map[string]cpuset.CPUSet) { + for key, cset := range obj { dst[key] = cset.String() } return json.Marshal(dst) @@ -844,13 +852,13 @@ func marshalEntry(obj interface{}) ([]byte, error) { // Unmarshal an opaque policy entry, special-casing cpusets and maps of cpusets. func unmarshalEntry(data []byte, ptr interface{}) error { - switch ptr.(type) { + switch ptr := ptr.(type) { case *cpuset.CPUSet: cset, err := cpuset.Parse(string(data[1 : len(data)-1])) if err != nil { return err } - *ptr.(*cpuset.CPUSet) = cset + *ptr = cset return nil case *map[string]cpuset.CPUSet: @@ -868,7 +876,7 @@ func unmarshalEntry(data []byte, ptr interface{}) error { dst[key] = cset } - *ptr.(*map[string]cpuset.CPUSet) = dst + *ptr = dst return nil default: @@ -884,32 +892,32 @@ func (cch *cache) cacheEntry(key string, ptr interface{}) error { return nil } - switch ptr.(type) { + switch ptr := ptr.(type) { case *cpuset.CPUSet: - cch.policyData[key] = *ptr.(*cpuset.CPUSet) + cch.policyData[key] = *ptr case *map[string]cpuset.CPUSet: - cch.policyData[key] = *ptr.(*map[string]cpuset.CPUSet) + cch.policyData[key] = *ptr case *map[string]string: - cch.policyData[key] = *ptr.(*map[string]string) + cch.policyData[key] = *ptr case *string: - cch.policyData[key] = *ptr.(*string) + cch.policyData[key] = *ptr case *bool: - cch.policyData[key] = *ptr.(*bool) + cch.policyData[key] = *ptr case *int32: - cch.policyData[key] = *ptr.(*int32) + cch.policyData[key] = *ptr case *uint32: - cch.policyData[key] = *ptr.(*uint32) + cch.policyData[key] = *ptr case *int64: - cch.policyData[key] = *ptr.(*int64) + cch.policyData[key] = *ptr case *uint64: - cch.policyData[key] = *ptr.(*uint64) + cch.policyData[key] = *ptr case *int: - cch.policyData[key] = *ptr.(*int) + cch.policyData[key] = *ptr case *uint: - cch.policyData[key] = *ptr.(*uint) + cch.policyData[key] = *ptr default: return cacheError("can't handle policy data of type %T", ptr) diff --git a/pkg/resmgr/cache/cache_test.go b/pkg/resmgr/cache/cache_test.go index 2a6fedb6b..7deb42650 100644 --- a/pkg/resmgr/cache/cache_test.go +++ b/pkg/resmgr/cache/cache_test.go @@ -42,9 +42,8 @@ var _ = Describe("Cache", func() { c, _, _ = makePopulatedCache(nriPods, nil) for _, nriPod := range nriPods { - pod, err := c.InsertPod(nriPod) + pod := c.InsertPod(nriPod, nil) Expect(pod).ToNot(BeNil()) - Expect(err).To(BeNil()) } }) @@ -61,9 +60,8 @@ var _ = Describe("Cache", func() { c, _, _ = makePopulatedCache(nriPods, nil) for _, nriPod := range nriPods { - pod, err := c.InsertPod(nriPod) + pod := c.InsertPod(nriPod, nil) Expect(pod).ToNot(BeNil()) - Expect(err).To(BeNil()) chk, ok := c.LookupPod(pod.GetID()) Expect(chk).ToNot(BeNil()) @@ -107,9 +105,8 @@ func makePopulatedCache(nriPods []*nri.PodSandbox, nriCtrs []*nri.Container) (ca ) for _, nriPod := range nriPods { - pod, err := c.InsertPod(nriPod) + pod := c.InsertPod(nriPod, nil) Expect(pod).ToNot(BeNil()) - Expect(err).To(BeNil()) pods = append(pods, pod) } for _, nriCtr := range nriCtrs { diff --git a/pkg/resmgr/cache/container.go b/pkg/resmgr/cache/container.go index 3031e1f5b..26bccc360 100644 --- a/pkg/resmgr/cache/container.go +++ b/pkg/resmgr/cache/container.go @@ -19,6 +19,7 @@ import ( "fmt" "path/filepath" "regexp" + "slices" "sort" "strconv" "strings" @@ -272,11 +273,11 @@ func isReadOnlyDevice(rules []*nri.LinuxDeviceCgroup, d *nri.LinuxDevice) bool { rType, rMajor, rMinor := r.Type, r.GetMajor().GetValue(), r.GetMinor().GetValue() switch { case rType == "" && rMajor == 0 && rMinor == 0: - if strings.IndexAny(r.Access, "w") > -1 { + if strings.Contains(r.Access, "w") { readOnly = false } case d.Type == rType && d.Major == rMajor && d.Minor == rMinor: - if strings.IndexAny(r.Access, "w") > -1 { + if strings.Contains(r.Access, "w") { readOnly = false } return readOnly @@ -416,15 +417,11 @@ func (c *container) GetMounts() []*Mount { var mounts []*Mount for _, m := range c.Ctr.GetMounts() { - var options []string - for _, o := range m.Options { - options = append(options, o) - } mounts = append(mounts, &Mount{ Destination: m.Destination, Source: m.Source, Type: m.Type, - Options: options, + Options: slices.Clone(m.Options), }) } @@ -890,17 +887,6 @@ func getTopologyHintsForDevice(devType string, major, minor int64, allowPathList return topology.Hints{} } -func getKubeletHint(cpus, mems string) (ret topology.Hints) { - if cpus != "" || mems != "" { - ret = topology.Hints{ - topology.ProviderKubelet: topology.Hint{ - Provider: topology.ProviderKubelet, - CPUs: cpus, - NUMAs: mems}} - } - return -} - func (c *container) GetAffinity() ([]*Affinity, error) { pod, ok := c.GetPod() if !ok { diff --git a/pkg/resmgr/cache/container_test.go b/pkg/resmgr/cache/container_test.go index f13ec97c1..5d0d3a831 100644 --- a/pkg/resmgr/cache/container_test.go +++ b/pkg/resmgr/cache/container_test.go @@ -19,6 +19,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "syscall" "github.com/containers/nri-plugins/pkg/log" @@ -799,24 +800,24 @@ func checkAllowedHints(annotations map[string]string, expectedHints int) bool { ann := "allow" + "." + cache.TopologyHintsKey allow, ok := ctr.GetEffectiveAnnotation(ann) if !ok { - fmt.Errorf("unable to get annotation %s (%s)", ann, allow) + log.Get("cache").Errorf("unable to get annotation %s (%s)", ann, allow) return false } if err := yaml.Unmarshal([]byte(allow), &pathList); err != nil { - fmt.Errorf("Error (%v) when trying to parse \"%s\"", err, allow) + log.Get("cache").Errorf("Error (%v) when trying to parse \"%s\"", err, allow) return false } ann = "deny" + "." + cache.TopologyHintsKey deny, ok := ctr.GetEffectiveAnnotation(ann) if !ok { - fmt.Errorf("unable to get annotation %s (%s)", ann, deny) + log.Get("cache").Errorf("unable to get annotation %s (%s)", ann, deny) return false } if err := yaml.Unmarshal([]byte(deny), &pathList); err != nil { - fmt.Errorf("Error (%v) when trying to parse \"%s\"", err, deny) + log.Get("cache").Errorf("Error (%v) when trying to parse \"%s\"", err, deny) return false } @@ -840,6 +841,9 @@ func createSysFsDevice(devType string, major, minor int64) error { } realDevPath, err := filepath.EvalSymlinks(devPath) + if err != nil { + return err + } if err := os.MkdirAll(testdataDir+"/"+realDevPath, 0770); err != nil { return err } @@ -852,7 +856,9 @@ func createSysFsDevice(devType string, major, minor int64) error { return err } - f.Write([]byte(cpulist)) + if _, err := f.Write([]byte(cpulist)); err != nil { + log.Get("cache").Errorf("unable to write to %s: %v", realDevPath+"/local_cpulist", err) + } f.Close() f, err = os.Create(realDevPath + "/numa_node") @@ -860,7 +866,9 @@ func createSysFsDevice(devType string, major, minor int64) error { return err } - f.Write([]byte(numanode)) + if _, err := f.Write([]byte(numanode)); err != nil { + log.Get("cache").Errorf("unable to write to %s: %v", realDevPath+"/numa_node", err) + } f.Close() return nil @@ -927,10 +935,7 @@ func WithCtrArgs(args []string) CtrOption { nriCtr.Args = nil return nil } - nriCtr.Args = make([]string, len(args), len(args)) - for i, a := range args { - nriCtr.Args[i] = a - } + nriCtr.Args = slices.Clone(args) return nil } } @@ -941,10 +946,7 @@ func WithCtrEnv(env []string) CtrOption { nriCtr.Env = nil return nil } - nriCtr.Env = make([]string, len(env), len(env)) - for i, e := range env { - nriCtr.Env[i] = e - } + nriCtr.Env = slices.Clone(env) return nil } } @@ -955,17 +957,13 @@ func WithCtrMounts(mounts []*nri.Mount) CtrOption { nriCtr.Mounts = nil return nil } - nriCtr.Mounts = make([]*nri.Mount, len(mounts), len(mounts)) + nriCtr.Mounts = make([]*nri.Mount, len(mounts)) for i, m := range mounts { - var options []string - for _, o := range m.Options { - options = append(options, o) - } nriCtr.Mounts[i] = &nri.Mount{ Destination: m.Destination, Source: m.Source, Type: m.Type, - Options: options, + Options: slices.Clone(m.Options), } } return nil @@ -983,7 +981,7 @@ func WithCtrDevices(devices []*nri.LinuxDevice) CtrOption { if nriCtr.Linux == nil { nriCtr.Linux = &nri.LinuxContainer{} } - nriCtr.Linux.Devices = make([]*nri.LinuxDevice, len(devices), len(devices)) + nriCtr.Linux.Devices = make([]*nri.LinuxDevice, len(devices)) for i, d := range devices { nriCtr.Linux.Devices[i] = &nri.LinuxDevice{ Path: d.Path, diff --git a/pkg/resmgr/cache/pod.go b/pkg/resmgr/cache/pod.go index 2c82083b5..9fbed7274 100644 --- a/pkg/resmgr/cache/pod.go +++ b/pkg/resmgr/cache/pod.go @@ -157,7 +157,7 @@ func (p *pod) setPodResources(podRes *podresapi.PodResources) { func (p *pod) GetPodResources() *podresapi.PodResources { if p.waitResCh != nil { log.Debug("waiting for pod resources fetch to complete...") - _ = <-p.waitResCh + <-p.waitResCh } return p.PodResources } diff --git a/pkg/resmgr/cache/pod_test.go b/pkg/resmgr/cache/pod_test.go index b2d525c58..787f91bd3 100644 --- a/pkg/resmgr/cache/pod_test.go +++ b/pkg/resmgr/cache/pod_test.go @@ -223,7 +223,7 @@ var _ = Describe("Pod", func() { _, pods, _ = makePopulatedCache(nriPods, nil) - Expect(pods[0].PrettyName()).To(Equal(pods[0].GetName())) + Expect(pods[0].PrettyName()).To(Equal(pods[0].GetNamespace() + "/" + pods[0].GetName())) Expect(pods[1].PrettyName()).To(Equal(pods[1].GetNamespace() + "/" + pods[1].GetName())) }) }) diff --git a/pkg/resmgr/cache/utils.go b/pkg/resmgr/cache/utils.go index 93513fcde..940dd5fe3 100644 --- a/pkg/resmgr/cache/utils.go +++ b/pkg/resmgr/cache/utils.go @@ -15,7 +15,6 @@ package cache import ( - "io/ioutil" "os" "path" "strconv" @@ -97,7 +96,7 @@ func getMemoryCapacity() int64 { return memoryCapacity } - if data, err = ioutil.ReadFile("/proc/meminfo"); err != nil { + if data, err = os.ReadFile("/proc/meminfo"); err != nil { return -1 } @@ -124,27 +123,6 @@ func getMemoryCapacity() int64 { return memoryCapacity } -// cgroupParentToQOS tries to map Pod cgroup parent to QOS class. -func cgroupParentToQOS(dir string) corev1.PodQOSClass { - var qos corev1.PodQOSClass - - // The parent directory naming scheme depends on the cgroup driver in use. - // Thus, rely on substring matching - split := strings.Split(strings.TrimPrefix(dir, "/"), "/") - switch { - case len(split) < 2: - qos = corev1.PodQOSClass("") - case strings.Index(split[1], strings.ToLower(string(corev1.PodQOSBurstable))) != -1: - qos = corev1.PodQOSBurstable - case strings.Index(split[1], strings.ToLower(string(corev1.PodQOSBestEffort))) != -1: - qos = corev1.PodQOSBestEffort - default: - qos = corev1.PodQOSGuaranteed - } - - return qos -} - // findContainerDir brute-force searches for a container cgroup dir. func findContainerDir(podCgroupDir, podID, ID string) string { var dirs []string @@ -178,10 +156,6 @@ func findContainerDir(podCgroupDir, podID, ID string) string { return "" } -func isSupportedQoSComputeResource(name corev1.ResourceName) bool { - return name == corev1.ResourceCPU || name == corev1.ResourceMemory -} - func init() { // TODO: get rid of this eventually, use pkg/sysfs instead... getMemoryCapacity() diff --git a/pkg/resmgr/control/control.go b/pkg/resmgr/control/control.go index 085a205ad..9418e877d 100644 --- a/pkg/resmgr/control/control.go +++ b/pkg/resmgr/control/control.go @@ -17,7 +17,6 @@ package control import ( "errors" "fmt" - "os" "sort" "strings" @@ -27,15 +26,6 @@ import ( cfgapi "github.com/containers/nri-plugins/pkg/apis/config/v1alpha1/resmgr/control" ) -const ( - // EnvVarEnableTestAPIs controls if test APIS are enabled (currently e2e test controller). - EnvVarEnableTestAPIs = "ENABLE_TEST_APIS" -) - -var ( - enableTestAPIs = (os.Getenv(EnvVarEnableTestAPIs) != "") -) - // Control is the interface for triggering controller-/domain-specific post-decision actions. type Control interface { // StartStopControllers starts/stops all controllers according to configuration. diff --git a/pkg/resmgr/control/cpu/cache.go b/pkg/resmgr/control/cpu/cache.go index 57868942b..4b3bb28bb 100644 --- a/pkg/resmgr/control/cpu/cache.go +++ b/pkg/resmgr/control/cpu/cache.go @@ -45,12 +45,11 @@ func setClassAssignments(c cache.Cache, a *cpuClassAssignments) { // Set the value of cached cpuClassAssignments func (c *cpuClassAssignments) Set(value interface{}) { - switch value.(type) { + switch v := value.(type) { case cpuClassAssignments: - *c = value.(cpuClassAssignments) + *c = v case *cpuClassAssignments: - cp := value.(*cpuClassAssignments) - *c = *cp + *c = *v } } diff --git a/pkg/resmgr/control/cpu/cpu.go b/pkg/resmgr/control/cpu/cpu.go index 8f7a07725..f50450b2e 100644 --- a/pkg/resmgr/control/cpu/cpu.go +++ b/pkg/resmgr/control/cpu/cpu.go @@ -295,5 +295,8 @@ func (ctl *cpuctl) getClasses() map[string]Class { // Register us as a controller. func init() { - control.Register(CPUController, "CPU controller", getCPUController()) + err := control.Register(CPUController, "CPU controller", getCPUController()) + if err != nil { + log.Warnf("failed to register CPU controller: %v", err) + } } diff --git a/pkg/resmgr/control/e2e-test/e2e-test.go b/pkg/resmgr/control/e2e-test/e2e-test.go index 2ad2cc266..106b11f7f 100644 --- a/pkg/resmgr/control/e2e-test/e2e-test.go +++ b/pkg/resmgr/control/e2e-test/e2e-test.go @@ -154,5 +154,8 @@ func (ctl *testctl) registerHandler() { // Register us as a controller. func init() { - control.Register(ControllerName, "Test controller", getE2ETestController()) + err := control.Register(ControllerName, "Test controller", getE2ETestController()) + if err != nil { + log.Warnf("failed to register controller: %v", err) + } } diff --git a/pkg/resmgr/events.go b/pkg/resmgr/events.go index 2ad5265de..1a9e3b7e7 100644 --- a/pkg/resmgr/events.go +++ b/pkg/resmgr/events.go @@ -16,7 +16,6 @@ package resmgr import ( logger "github.com/containers/nri-plugins/pkg/log" - "github.com/containers/nri-plugins/pkg/resmgr/cache" ) // Our logger instance for events. @@ -36,7 +35,7 @@ func (m *resmgr) startEventProcessing() error { go func() { for { select { - case _ = <-stop: + case <-stop: return case event := <-m.events: m.processEvent(event) @@ -48,14 +47,6 @@ func (m *resmgr) startEventProcessing() error { return nil } -// stopEventProcessing stops event and metrics processing. -func (m *resmgr) stopEventProcessing() { - if m.stop != nil { - close(m.stop) - m.stop = nil - } -} - // SendEvent injects the given event to the resource manager's event processing loop. func (m *resmgr) SendEvent(event interface{}) error { if m.events == nil { @@ -82,8 +73,3 @@ func (m *resmgr) processEvent(e interface{}) { evtlog.Warn("event of unexpected type %T...", e) } } - -// resolveCgroupPath resolves a cgroup path to a container. -func (m *resmgr) resolveCgroupPath(path string) (cache.Container, bool) { - return m.cache.LookupContainerByCgroup(path) -} diff --git a/pkg/resmgr/lib/memory/allocator.go b/pkg/resmgr/lib/memory/allocator.go index 41c6a8673..18ed11e77 100644 --- a/pkg/resmgr/lib/memory/allocator.go +++ b/pkg/resmgr/lib/memory/allocator.go @@ -305,7 +305,10 @@ func (a *Allocator) allocate(req *Request) (retErr error) { defer func() { if retErr != nil { - a.revertJournal(req) + _, err := a.revertJournal(req) + if err != nil { + log.Warn("failed to revert journal on error: %v", err) + } } }() @@ -337,7 +340,10 @@ func (a *Allocator) realloc(req *Request, nodes NodeMask, types TypeMask) (zone defer func() { if retErr != nil { - a.revertJournal(nil) + _, err := a.revertJournal(nil) + if err != nil { + log.Warn("failed to revert journal on error: %v", err) + } } }() diff --git a/pkg/resmgr/lib/memory/allocator_test.go b/pkg/resmgr/lib/memory/allocator_test.go index 3cf438a87..f5c7f88dd 100644 --- a/pkg/resmgr/lib/memory/allocator_test.go +++ b/pkg/resmgr/lib/memory/allocator_test.go @@ -16,7 +16,6 @@ package libmem_test import ( "fmt" - "strconv" "testing" "github.com/stretchr/testify/require" @@ -1155,7 +1154,7 @@ func TestRealloc(t *testing.T) { require.Equal(t, tc.updates, updates, "updated nodes") } - nodes, updates, err = a.Realloc(tc.id, tc.newNodes, tc.newTypes) + nodes, _, err = a.Realloc(tc.id, tc.newNodes, tc.newTypes) if !tc.fail { require.Nil(t, err, "unexpected realloc failure") @@ -1451,13 +1450,3 @@ func (s *testSetup) nodes(t *testing.T) []*Node { return nodes } - -var ( - nextID = 1 -) - -func newID() string { - id := strconv.Itoa(nextID) - nextID++ - return id -} diff --git a/pkg/resmgr/lib/memory/request_test.go b/pkg/resmgr/lib/memory/request_test.go index e819e2c19..c4b9788cd 100644 --- a/pkg/resmgr/lib/memory/request_test.go +++ b/pkg/resmgr/lib/memory/request_test.go @@ -95,7 +95,6 @@ func TestHumanReadableSize(t *testing.T) { func TestRequestString(t *testing.T) { type testCase struct { name string - aff NodeMask req *Request result string } diff --git a/pkg/resmgr/lib/memory/types.go b/pkg/resmgr/lib/memory/types.go index 36bba29fc..aede65a4e 100644 --- a/pkg/resmgr/lib/memory/types.go +++ b/pkg/resmgr/lib/memory/types.go @@ -153,10 +153,6 @@ const ( TypeMaskAll TypeMask = (TypeMaskHBM << 1) - 1 // all types of memory ) -var ( - typeMaskToString map[TypeMask]string -) - // NewTypeMask returns a TypeMask containing the given memory types. func NewTypeMask(types ...Type) TypeMask { m := TypeMask(0) diff --git a/pkg/resmgr/main/main.go b/pkg/resmgr/main/main.go index d2e3c7579..bf3f97f4d 100644 --- a/pkg/resmgr/main/main.go +++ b/pkg/resmgr/main/main.go @@ -63,7 +63,9 @@ func (m *Main) Run() error { log.Infof("starting '%s' policy version %s/build %s...", m.policy.Name(), version.Version, version.Build) - m.startTracing() + if err := m.startTracing(); err != nil { + log.Warnf("failed to start tracing: %v", err) + } defer m.stopTracing() err := m.mgr.Start() diff --git a/pkg/resmgr/nri.go b/pkg/resmgr/nri.go index ba7c58eba..2858d5853 100644 --- a/pkg/resmgr/nri.go +++ b/pkg/resmgr/nri.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "os" + "slices" "time" "github.com/containers/nri-plugins/pkg/instrumentation/metrics" @@ -332,12 +333,8 @@ func (p *nriPlugin) StopPodSandbox(ctx context.Context, podSandbox *api.PodSandb b := metrics.Block() defer b.Done() - released := []cache.Container{} pod, _ := m.cache.LookupPod(podSandbox.GetId()) - - for _, c := range pod.GetContainers() { - released = append(released, c) - } + released := slices.Clone(pod.GetContainers()) if err := p.runPostReleaseHooks(event, released...); err != nil { nri.Error("%s: failed to run post-release hooks for pod %s: %v", @@ -371,12 +368,8 @@ func (p *nriPlugin) RemovePodSandbox(ctx context.Context, podSandbox *api.PodSan m := p.resmgr - released := []cache.Container{} pod, _ := m.cache.LookupPod(podSandbox.GetId()) - - for _, c := range pod.GetContainers() { - released = append(released, c) - } + released := slices.Clone(pod.GetContainers()) if err := p.runPostReleaseHooks(event, released...); err != nil { nri.Error("%s: failed to run post-release hooks for pod %s: %v", @@ -446,7 +439,11 @@ func (p *nriPlugin) CreateContainer(ctx context.Context, pod *api.PodSandbox, co if err := p.runPostAllocateHooks(event, c); err != nil { nri.Error("%s: failed to run post-allocate hooks for %s: %v", event, container.GetName(), err) - p.runPostReleaseHooks(event, c) + relErr := p.runPostReleaseHooks(event, c) + if relErr != nil { + nri.Warnf("%s: failed to run post-release hooks on error for %s: %v", + event, container.GetName(), relErr) + } return nil, nil, fmt.Errorf("failed to allocate container resources: %w", err) } @@ -1091,20 +1088,3 @@ func (p *nriPlugin) runPostReleaseHooks(method string, released ...cache.Contain } return nil } - -// runPostUpdateHooks runs the necessary hooks after reconciliation. -func (p *nriPlugin) runPostUpdateHooks(method string) error { - m := p.resmgr - for _, c := range m.cache.GetPendingContainers() { - switch c.GetState() { - case cache.ContainerStateRunning, cache.ContainerStateCreated: - if err := m.control.RunPostUpdateHooks(c); err != nil { - return err - } - default: - nri.Warn("%s: skipping container %s (in state %v)", method, - c.PrettyName(), c.GetState()) - } - } - return nil -} diff --git a/pkg/resmgr/policy/metrics.go b/pkg/resmgr/policy/metrics.go index 368bb18bf..2fac050ff 100644 --- a/pkg/resmgr/policy/metrics.go +++ b/pkg/resmgr/policy/metrics.go @@ -16,7 +16,6 @@ package policy import ( "strconv" - "sync" "github.com/prometheus/client_golang/prometheus" v1 "k8s.io/api/core/v1" @@ -29,7 +28,6 @@ import ( type PolicyCollector struct { policy *policy - block sync.Mutex } func (p *policy) newPolicyCollector() *PolicyCollector { @@ -89,7 +87,7 @@ func (p *policy) newSystemCollector() *SystemCollector { system: p.system, Nodes: map[int]*NodeMetric{}, Cpus: map[int]*CpuMetric{}, - Metrics: make([]*prometheus.GaugeVec, metricsCount, metricsCount), + Metrics: make([]*prometheus.GaugeVec, metricsCount), } s.Metrics[nodeCapacity] = prometheus.NewGaugeVec( diff --git a/pkg/resmgr/policy/policy.go b/pkg/resmgr/policy/policy.go index 8b74d8995..bbc1bac93 100644 --- a/pkg/resmgr/policy/policy.go +++ b/pkg/resmgr/policy/policy.go @@ -198,20 +198,12 @@ type ZoneAttribute struct { // Policy instance/state. type policy struct { - options Options // policy options - cache cache.Cache // system state cache - active Backend // our active backend - system system.System // system/HW/topology info - sendEvent SendEventFn // function to send event up to the resource manager - pcollect *PolicyCollector // policy metrics collector - scollect *SystemCollector // system metrics collector -} - -// backend is a registered Backend. -type backend struct { - name string // unique backend name - description string // verbose backend description - create CreateFn // backend creation function + options Options // policy options + cache cache.Cache // system state cache + active Backend // our active backend + system system.System // system/HW/topology info + pcollect *PolicyCollector // policy metrics collector + scollect *SystemCollector // system metrics collector } // Out logger instance. @@ -322,7 +314,10 @@ func (p *policy) ExportResourceData(c cache.Container) { } } - p.cache.WriteFile(c.GetID(), ExportedResources, 0644, buf.Bytes()) + err := p.cache.WriteFile(c.GetID(), ExportedResources, 0644, buf.Bytes()) + if err != nil { + log.Warnf("container %s: failed to export resource data: %v", c.PrettyName(), err) + } } // GetTopologyZones returns the policy/pool data for 'topology zone' CRDs. diff --git a/pkg/resmgr/resource-manager.go b/pkg/resmgr/resource-manager.go index b5ba74f23..02676aa87 100644 --- a/pkg/resmgr/resource-manager.go +++ b/pkg/resmgr/resource-manager.go @@ -171,7 +171,9 @@ func (m *resmgr) start(cfg cfgapi.ResmgrConfig) error { m.cfg = cfg mCfg := cfg.CommonConfig() - logger.Configure(&mCfg.Log) + if err := logger.Configure(&mCfg.Log); err != nil { + log.Warnf("failed to configure logger: %v", err) + } if err := m.policy.Start(m.cfg.PolicyConfig()); err != nil { return err @@ -232,8 +234,13 @@ func (m *resmgr) setupCache() error { func (m *resmgr) setupPolicy(backend policy.Backend) error { var err error - m.cache.ResetActivePolicy() - m.cache.SetActivePolicy(backend.Name()) + if err := m.cache.ResetActivePolicy(); err != nil { + log.Warnf("failed to reset active policy: %v", err) + } + + if err := m.cache.SetActivePolicy(backend.Name()); err != nil { + log.Warnf("failed to set active policy: %v", err) + } p, err := policy.NewPolicy(backend, m.cache, &policy.Options{SendEvent: m.SendEvent}) if err != nil { @@ -285,11 +292,15 @@ func (m *resmgr) reconfigure(cfg cfgapi.ResmgrConfig) error { apply := func(cfg cfgapi.ResmgrConfig) error { mCfg := cfg.CommonConfig() - logger.Configure(&mCfg.Log) + if err := logger.Configure(&mCfg.Log); err != nil { + log.Warnf("failed to configure logger: %v", err) + } if err := instrumentation.Reconfigure(&mCfg.Instrumentation); err != nil { return err } - m.control.StartStopControllers(&mCfg.Control) + if err := m.control.StartStopControllers(&mCfg.Control); err != nil { + log.Warnf("failed to restart controllers: %v", err) + } err := m.policy.Reconfigure(cfg.PolicyConfig()) if err != nil { diff --git a/pkg/sysfs/parsers.go b/pkg/sysfs/parsers.go index 74b2c72a5..2c127fecb 100644 --- a/pkg/sysfs/parsers.go +++ b/pkg/sysfs/parsers.go @@ -15,7 +15,7 @@ package sysfs import ( - "io/ioutil" + "os" "strconv" "strings" ) @@ -70,43 +70,43 @@ func parseNumeric(path, value string, ptr interface{}) error { return err } - switch ptr.(type) { + switch ptr := ptr.(type) { case *int: num, err = strconv.ParseInt(numstr, 0, strconv.IntSize) - *ptr.(*int) = int(num * unit) + *ptr = int(num * unit) case *int8: num, err = strconv.ParseInt(numstr, 0, 8) - *ptr.(*int8) = int8(num * unit) + *ptr = int8(num * unit) case *int16: num, err = strconv.ParseInt(numstr, 0, 16) - *ptr.(*int16) = int16(num * unit) + *ptr = int16(num * unit) case *int32: num, err = strconv.ParseInt(numstr, 0, 32) - *ptr.(*int32) = int32(num * unit) + *ptr = int32(num * unit) case *int64: num, err = strconv.ParseInt(numstr, 0, 64) - *ptr.(*int64) = int64(num * unit) + *ptr = int64(num * unit) case *uint: num, err = strconv.ParseInt(numstr, 0, strconv.IntSize) - *ptr.(*uint) = uint(num * unit) + *ptr = uint(num * unit) case *uint8: num, err = strconv.ParseInt(numstr, 0, 8) - *ptr.(*uint8) = uint8(num * unit) + *ptr = uint8(num * unit) case *uint16: num, err = strconv.ParseInt(numstr, 0, 16) - *ptr.(*uint16) = uint16(num * unit) + *ptr = uint16(num * unit) case *uint32: num, err = strconv.ParseInt(numstr, 0, 32) - *ptr.(*uint32) = uint32(num * unit) + *ptr = uint32(num * unit) case *uint64: num, err = strconv.ParseInt(numstr, 0, 64) - *ptr.(*uint64) = uint64(num * unit) + *ptr = uint64(num * unit) case *float32: f, err = strconv.ParseFloat(numstr, 32) - *ptr.(*float32) = float32(f) * float32(unit) + *ptr = float32(f) * float32(unit) case *float64: f, err = strconv.ParseFloat(numstr, 64) - *ptr.(*float64) = f * float64(unit) + *ptr = f * float64(unit) default: err = sysfsError(path, "can't parse numeric value '%s' into type %T", value, ptr) @@ -119,9 +119,9 @@ func parseNumeric(path, value string, ptr interface{}) error { func ParseFileEntries(path string, values map[string]interface{}, pickFn PickEntryFn) error { var err error - data, err := ioutil.ReadFile(path) + data, err := os.ReadFile(path) if err != nil { - sysfsError(path, "failed to read file: %v", err) + return sysfsError(path, "failed to read file: %v", err) } left := len(values) @@ -136,7 +136,7 @@ func ParseFileEntries(path string, values map[string]interface{}, pickFn PickEnt continue } - switch ptr.(type) { + switch ptr := ptr.(type) { case *int, *int8, *int32, *int16, *int64, *uint, *uint8, *uint16, *uint32, *uint64: if err = parseNumeric(path, value, ptr); err != nil { return err @@ -146,9 +146,9 @@ func ParseFileEntries(path string, values map[string]interface{}, pickFn PickEnt return err } case *string: - *ptr.(*string) = value + *ptr = value case *bool: - *ptr.(*bool), err = strconv.ParseBool(value) + *ptr, err = strconv.ParseBool(value) if err != nil { return sysfsError(path, "failed to parse line %s, value '%s' for boolean key '%s'", line, value, key) diff --git a/pkg/sysfs/system.go b/pkg/sysfs/system.go index 7381a6460..ee2886fdd 100644 --- a/pkg/sysfs/system.go +++ b/pkg/sysfs/system.go @@ -243,9 +243,8 @@ type cpu struct { // CPUFreq is a CPU frequency scaling range type CPUFreq struct { - min uint64 // minimum frequency (kHz) - max uint64 // maximum frequency (kHz) - all []uint64 // discrete set of frequencies if applicable/known + min uint64 // minimum frequency (kHz) + max uint64 // maximum frequency (kHz) } // EPP represents the value of a CPU energy performance profile @@ -926,8 +925,12 @@ func (sys *system) discoverCPU(path string) error { if _, err := readSysfsEntry(path, "topology/physical_package_id", &cpu.pkg); err != nil { return err } - readSysfsEntry(path, "topology/die_id", &cpu.die) - readSysfsEntry(path, "topology/cluster_id", &cpu.cluster) + if _, err := readSysfsEntry(path, "topology/die_id", &cpu.die); err != nil { + log.Warnf("failed to read die_id for CPU %d: %v", cpu.id, err) + } + if _, err := readSysfsEntry(path, "topology/cluster_id", &cpu.cluster); err != nil { + log.Warnf("failed to read cluster_id for CPU %d: %v", cpu.id, err) + } if _, err := readSysfsEntry(path, "topology/core_id", &cpu.core); err != nil { return err } diff --git a/pkg/sysfs/utils.go b/pkg/sysfs/utils.go index 754b9e843..7f86b4a88 100644 --- a/pkg/sysfs/utils.go +++ b/pkg/sysfs/utils.go @@ -16,7 +16,6 @@ package sysfs import ( "fmt" - "io/ioutil" "os" "path/filepath" "strconv" @@ -54,7 +53,7 @@ func readSysfsEntry(base, entry string, ptr interface{}, args ...interface{}) (s path := filepath.Join(base, entry) - blob, err := ioutil.ReadFile(path) + blob, err := os.ReadFile(path) if err != nil { return "", sysfsError(path, "failed to read sysfs entry: %w", err) } @@ -64,7 +63,7 @@ func readSysfsEntry(base, entry string, ptr interface{}, args ...interface{}) (s return buf, nil } - switch ptr.(type) { + switch ptr := ptr.(type) { case *string, *int, *uint, *int8, *uint8, *int16, *uint16, *int32, *uint32, *int64, *uint64: err := parseValue(buf, ptr) if err != nil { @@ -83,7 +82,7 @@ func readSysfsEntry(base, entry string, ptr interface{}, args ...interface{}) (s } return buf, nil case *EPP: - *ptr.(*EPP) = EPPFromString(buf) + *ptr = EPPFromString(buf) return buf, nil } @@ -151,9 +150,9 @@ func getSeparator(defaultVal string, args []interface{}) (string, error) { // Parse a value from a string. func parseValue(str string, value interface{}) error { - switch value.(type) { + switch value := value.(type) { case *string: - *value.(*string) = str + *value = str case *int, *int8, *int16, *int32, *int64: v, err := strconv.ParseInt(str, 0, 0) @@ -161,17 +160,17 @@ func parseValue(str string, value interface{}) error { return fmt.Errorf("invalid entry '%s': %w", str, err) } - switch value.(type) { + switch value := value.(type) { case *int: - *value.(*int) = int(v) + *value = int(v) case *int8: - *value.(*int8) = int8(v) + *value = int8(v) case *int16: - *value.(*int16) = int16(v) + *value = int16(v) case *int32: - *value.(*int32) = int32(v) - case int64: - *value.(*int64) = v + *value = int32(v) + case *int64: + *value = v } case *uint, *uint8, *uint16, *uint32, *uint64: @@ -180,17 +179,17 @@ func parseValue(str string, value interface{}) error { return fmt.Errorf("invalid entry: '%s': %w", str, err) } - switch value.(type) { + switch value := value.(type) { case *uint: - *value.(*uint) = uint(v) + *value = uint(v) case *uint8: - *value.(*uint8) = uint8(v) + *value = uint8(v) case *uint16: - *value.(*uint16) = uint16(v) + *value = uint16(v) case *uint32: - *value.(*uint32) = uint32(v) + *value = uint32(v) case *uint64: - *value.(*uint64) = v + *value = v } } @@ -232,14 +231,14 @@ func parseValueList(str, sep string, valuep interface{}) error { if s == "" { break } - switch value.(type) { + switch value := value.(type) { case idset.IDSet: if rng := strings.Split(s, "-"); len(rng) == 1 { id, err := strconv.Atoi(s) if err != nil { return fmt.Errorf("invalid entry '%s': %w", s, err) } - value.(idset.IDSet).Add(idset.ID(id)) + value.Add(idset.ID(id)) } else { beg, err := strconv.Atoi(rng[0]) if err != nil { @@ -250,7 +249,7 @@ func parseValueList(str, sep string, valuep interface{}) error { return fmt.Errorf("invalid entry '%s': %w", s, err) } for id := beg; id <= end; id++ { - value.(idset.IDSet).Add(idset.ID(id)) + value.Add(idset.ID(id)) } } @@ -259,17 +258,17 @@ func parseValueList(str, sep string, valuep interface{}) error { if err != nil { return fmt.Errorf("invalid entry '%s': %w", s, err) } - switch value.(type) { + switch vslice := value.(type) { case []int: - value = append(value.([]int), int(v)) + value = append(vslice, int(v)) case []int8: - value = append(value.([]int8), int8(v)) + value = append(vslice, int8(v)) case []int16: - value = append(value.([]int16), int16(v)) + value = append(vslice, int16(v)) case []int32: - value = append(value.([]int32), int32(v)) + value = append(vslice, int32(v)) case []int64: - value = append(value.([]int64), v) + value = append(vslice, v) } case []uint, []uint8, []uint16, []uint32, []uint64: @@ -277,17 +276,17 @@ func parseValueList(str, sep string, valuep interface{}) error { if err != nil { return fmt.Errorf("invalid entry '%s': %w", s, err) } - switch value.(type) { + switch vslice := value.(type) { case []uint: - value = append(value.([]uint), uint(v)) + value = append(vslice, uint(v)) case []uint8: - value = append(value.([]uint8), uint8(v)) + value = append(vslice, uint8(v)) case []uint16: - value = append(value.([]uint16), uint16(v)) + value = append(vslice, uint16(v)) case []uint32: - value = append(value.([]uint32), uint32(v)) + value = append(vslice, uint32(v)) case []uint64: - value = append(value.([]uint64), v) + value = append(vslice, v) } } } @@ -322,9 +321,9 @@ func parseValueList(str, sep string, valuep interface{}) error { // Format a value into a string. func formatValue(value interface{}) (string, error) { - switch value.(type) { + switch value := value.(type) { case string: - return value.(string), nil + return value, nil case int, uint, int8, uint8, int16, uint16, int32, uint32, int64, uint64: return fmt.Sprintf("%d", value), nil default: @@ -336,9 +335,9 @@ func formatValue(value interface{}) (string, error) { func formatValueList(sep string, value interface{}) (string, error) { var v []interface{} - switch value.(type) { + switch value := value.(type) { case idset.IDSet: - return value.(idset.IDSet).StringWithSeparator(sep), nil + return value.StringWithSeparator(sep), nil case []int, []uint, []int8, []uint8, []int16, []uint16, []int32, []uint32, []int64, []uint64: v = value.([]interface{}) default: