From 9e1bbc911b451ee01736d8f9becdfe6c78f1a11d Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 20:01:21 +0200 Subject: [PATCH 01/23] template: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- cmd/plugins/template/policy/template-policy.go | 7 ------- 1 file changed, 7 deletions(-) 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 } From 05de728566db367e42ea648f083072135aed34a4 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 20:12:47 +0200 Subject: [PATCH 02/23] memtierd: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- cmd/plugins/memtierd/main.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) 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 From 93c4081d0ba786d3bc70dfab488b697ca6cabcb5 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 20:14:02 +0200 Subject: [PATCH 03/23] memory-qos: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- cmd/plugins/memory-qos/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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: From b5048144c4e96814285ad00825019a1e3589271d Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Tue, 10 Dec 2024 11:08:46 +0200 Subject: [PATCH 04/23] scripts: clean up all test VMs after test. Update test VM directory glob pattern to test/e2e/n[0-9]*-c*. This should also catch and clean up the latest VM topology addition (n6-hbm-cxl-fedora-40-cloud-base-containerd). Signed-off-by: Krisztian Litkey --- scripts/testing/e2e-runner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/testing/e2e-runner b/scripts/testing/e2e-runner index 281f6a36a..76d206275 100755 --- a/scripts/testing/e2e-runner +++ b/scripts/testing/e2e-runner @@ -201,7 +201,7 @@ cleanup_test_vms() { info "cleaning up test VMs..." must-cd "$WORKTREE" - for src in test/e2e/n[0-9]*c[0-9]*-c*; do + for src in test/e2e/n[0-9]*-c*; do (cd "$src" && vagrant destroy --force) done } From a961af3931165ad9075d617ad6d8b2c797d650f2 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Wed, 4 Dec 2024 16:59:26 +0200 Subject: [PATCH 05/23] cache: update tests for InsertPod signature change. Signed-off-by: Krisztian Litkey --- pkg/resmgr/cache/cache_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) 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 { From 6814acef9b30ad84f0d502919b0e7f3abeca0481 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Wed, 4 Dec 2024 16:59:52 +0200 Subject: [PATCH 06/23] cache: update tests for update pod.PrettyName(). Signed-off-by: Krisztian Litkey --- pkg/resmgr/cache/pod_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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())) }) }) From 6562f54c41519adbd5efc2fb5cc08b1dc3c4e021 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:30:09 +0200 Subject: [PATCH 07/23] apis/resmgr: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/apis/resmgr/v1alpha1/expression.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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) } From ac591f1913541710374ad171be9e44649858e5e3 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 19:32:32 +0200 Subject: [PATCH 08/23] resmgr/cache: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/resmgr/cache/cache.go | 68 +++++++++++++++++------------- pkg/resmgr/cache/container.go | 22 ++-------- pkg/resmgr/cache/container_test.go | 40 +++++++++--------- pkg/resmgr/cache/pod.go | 2 +- pkg/resmgr/cache/utils.go | 28 +----------- 5 files changed, 63 insertions(+), 97 deletions(-) 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/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/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() From 934279f8d373fbbf26a3758c85c8c8e9593043d3 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:39:31 +0200 Subject: [PATCH 09/23] cgroups: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/cgroups/cgroupblkio.go | 5 ++--- pkg/cgroups/cgroupstats.go | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) 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 From 637479139d07b4c3985cc53c6edd4b54224421b4 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:39:45 +0200 Subject: [PATCH 10/23] cgroupstats: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/cgroupstats/collector.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 } From f1cedb1d9deec9e16d64299463ed4dd16b4bd28a Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:29:36 +0200 Subject: [PATCH 11/23] config: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/apis/config/v1alpha1/resmgr/control/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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"` } From ce697f1852e19c1b9c1cf9b8838a15b6b7248fc8 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:21:05 +0200 Subject: [PATCH 12/23] resmgr/control: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/resmgr/control/control.go | 10 ---------- pkg/resmgr/control/cpu/cache.go | 7 +++---- pkg/resmgr/control/cpu/cpu.go | 5 ++++- pkg/resmgr/control/e2e-test/e2e-test.go | 5 ++++- 4 files changed, 11 insertions(+), 16 deletions(-) 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) + } } From 8af4c2e71c03b7840451c3a5f0b9548b2ad9bd3f Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:57:18 +0200 Subject: [PATCH 13/23] cpuallocator: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/cpuallocator/allocator.go | 4 ---- pkg/cpuallocator/cpuallocator_test.go | 7 +++---- 2 files changed, 3 insertions(+), 8 deletions(-) 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) } From 26aa513a75759554b95ee1566f86347da8dd3656 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:33:24 +0200 Subject: [PATCH 14/23] healthz: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/healthz/healthz.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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) + } } } From 8c96eabff19eeee268ccde2abac4f16c35522fce Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:23:29 +0200 Subject: [PATCH 15/23] http: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/http/http.go | 13 ++++++++++--- pkg/http/http_test.go | 10 ++-------- 2 files changed, 12 insertions(+), 11 deletions(-) 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) } From 0c7e9f26f3ca0b34804002d14b418a6ee777ac80 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 19:14:53 +0200 Subject: [PATCH 16/23] instrumentation: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/instrumentation/instrumentation_test.go | 14 ++++++++------ pkg/instrumentation/metrics/metrics.go | 4 ++-- pkg/instrumentation/tracing/exporter.go | 4 +++- 3 files changed, 13 insertions(+), 9 deletions(-) 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 From abeff17d29bfcf483cf6f3b647ed88ec9e80aec0 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:44:06 +0200 Subject: [PATCH 17/23] libmem: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/resmgr/lib/memory/allocator.go | 10 ++++++++-- pkg/resmgr/lib/memory/allocator_test.go | 13 +------------ pkg/resmgr/lib/memory/request_test.go | 1 - pkg/resmgr/lib/memory/types.go | 4 ---- 4 files changed, 9 insertions(+), 19 deletions(-) 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) From 9aa2f965af438c091446bd1caf5190c343d2a527 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 17:21:26 +0200 Subject: [PATCH 18/23] log: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/log/config.go | 2 -- pkg/log/klogcontrol/klogcontrol.go | 9 ++++++--- pkg/log/log.go | 5 ++++- pkg/log/signal.go | 7 +------ 4 files changed, 11 insertions(+), 12 deletions(-) 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]) } From bec1e79f41bcf42f41826d6181d4b686e5eba38b Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:07:47 +0200 Subject: [PATCH 19/23] metrics: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/metrics/metrics.go | 10 ++++++++-- pkg/metrics/metrics_test.go | 11 +++++------ 2 files changed, 13 insertions(+), 8 deletions(-) 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() -} From c52c7d457be170c2eaaab4852580536f69e1c2e0 Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:42:39 +0200 Subject: [PATCH 20/23] agent: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/agent/agent.go | 6 +++--- pkg/agent/podresapi/client.go | 16 +--------------- pkg/agent/watch/file.go | 2 +- pkg/agent/watch/object.go | 3 +-- 4 files changed, 6 insertions(+), 21 deletions(-) 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 } } From 7dbcb38b16cf6b88b089e351fa3135f8cbf08b8b Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 19:06:16 +0200 Subject: [PATCH 21/23] pidfile: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/pidfile/pidfile.go | 6 +++++- pkg/pidfile/pidfile_test.go | 7 +++---- 2 files changed, 8 insertions(+), 5 deletions(-) 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) } From 0c2b3df9e3523e87355a785120d7970a5298647e Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:35:22 +0200 Subject: [PATCH 22/23] resmgr/policy: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/resmgr/policy/metrics.go | 4 +--- pkg/resmgr/policy/policy.go | 25 ++++++++++--------------- 2 files changed, 11 insertions(+), 18 deletions(-) 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. From 8ab768c8ebb158940e917f0a1927e2d8380f712a Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Mon, 9 Dec 2024 18:40:36 +0200 Subject: [PATCH 23/23] resmgr: golangci-lint fixes. Signed-off-by: Krisztian Litkey --- pkg/resmgr/events.go | 16 +-------------- pkg/resmgr/main/main.go | 4 +++- pkg/resmgr/nri.go | 36 ++++++++-------------------------- pkg/resmgr/resource-manager.go | 21 +++++++++++++++----- 4 files changed, 28 insertions(+), 49 deletions(-) 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/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/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 {