From 9e07eb9ac441354561ac389d6cac04f96ee9a664 Mon Sep 17 00:00:00 2001 From: Pierre Gimalac Date: Tue, 28 Nov 2023 10:22:31 +0100 Subject: [PATCH] Fix file handle check on Darwin (#21013) * refactor: rename freebsd file_handle check files to bsd * chore(file_handle): add package comment * feat(file_handles): fix bsd implementation for darwin * fix(file_handles): fix bsd tests * chore: add release note * fix: write proper oid name in logs and errors --- .../corechecks/system/filehandles/docs.go | 7 ++ .../system/filehandles/file_handles.go | 2 +- .../system/filehandles/file_handles_bsd.go | 71 +++++++++++++++++++ .../filehandles/file_handles_bsd_test.go | 45 ++++++++++++ .../system/filehandles/file_handles_darwin.go | 8 +++ .../filehandles/file_handles_freebsd.go | 65 +---------------- .../filehandles/file_handles_freebsd_test.go | 39 ---------- .../system/filehandles/file_handles_test.go | 2 +- ...file-check-on-darwin-ea525cc9369307a4.yaml | 11 +++ 9 files changed, 145 insertions(+), 105 deletions(-) create mode 100644 pkg/collector/corechecks/system/filehandles/docs.go create mode 100644 pkg/collector/corechecks/system/filehandles/file_handles_bsd.go create mode 100644 pkg/collector/corechecks/system/filehandles/file_handles_bsd_test.go create mode 100644 pkg/collector/corechecks/system/filehandles/file_handles_darwin.go delete mode 100644 pkg/collector/corechecks/system/filehandles/file_handles_freebsd_test.go create mode 100644 releasenotes/notes/file-check-on-darwin-ea525cc9369307a4.yaml diff --git a/pkg/collector/corechecks/system/filehandles/docs.go b/pkg/collector/corechecks/system/filehandles/docs.go new file mode 100644 index 0000000000000..add0f8e747c61 --- /dev/null +++ b/pkg/collector/corechecks/system/filehandles/docs.go @@ -0,0 +1,7 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. + +// Package filehandles defines the file_handle core check +package filehandles diff --git a/pkg/collector/corechecks/system/filehandles/file_handles.go b/pkg/collector/corechecks/system/filehandles/file_handles.go index c7a31168094db..10703d355e5f6 100644 --- a/pkg/collector/corechecks/system/filehandles/file_handles.go +++ b/pkg/collector/corechecks/system/filehandles/file_handles.go @@ -2,7 +2,7 @@ // under the Apache License Version 2.0. // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2016-present Datadog, Inc. -//go:build !windows && !freebsd +//go:build !windows && !freebsd && !darwin package filehandles diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_bsd.go b/pkg/collector/corechecks/system/filehandles/file_handles_bsd.go new file mode 100644 index 0000000000000..2cd04b1907de9 --- /dev/null +++ b/pkg/collector/corechecks/system/filehandles/file_handles_bsd.go @@ -0,0 +1,71 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. +//go:build freebsd || darwin + +package filehandles + +import ( + "github.com/blabber/go-freebsd-sysctl/sysctl" + + "github.com/DataDog/datadog-agent/pkg/aggregator/sender" + "github.com/DataDog/datadog-agent/pkg/autodiscovery/integration" + "github.com/DataDog/datadog-agent/pkg/collector/check" + core "github.com/DataDog/datadog-agent/pkg/collector/corechecks" + "github.com/DataDog/datadog-agent/pkg/util/log" +) + +// For testing purpose +var getInt64 = sysctl.GetInt64 + +const fileHandlesCheckName = "file_handle" + +type fhCheck struct { + core.CheckBase +} + +// Run executes the check +func (c *fhCheck) Run() error { + + sender, err := c.GetSender() + if err != nil { + return err + } + openFh, err := getInt64(openfilesOID) + if err != nil { + log.Warnf("Error getting %s value %v", openfilesOID, err) + return err + } + maxFh, err := getInt64("kern.maxfiles") + if err != nil { + log.Warnf("Error getting kern.maxfiles value %v", err) + return err + } + log.Debugf("Submitting %s %v", openfilesOID, openFh) + log.Debugf("Submitting kern.maxfiles %v", maxFh) + sender.Gauge("system.fs.file_handles.used", float64(openFh), "", nil) + sender.Gauge("system.fs.file_handles.max", float64(maxFh), "", nil) + sender.Commit() + + return nil +} + +// The check doesn't need configuration +func (c *fhCheck) Configure(senderManager sender.SenderManager, integrationConfigDigest uint64, data integration.Data, initConfig integration.Data, source string) (err error) { + if err := c.CommonConfigure(senderManager, integrationConfigDigest, initConfig, data, source); err != nil { + return err + } + + return err +} + +func fhFactory() check.Check { + return &fhCheck{ + CheckBase: core.NewCheckBase(fileHandlesCheckName), + } +} + +func init() { + core.RegisterCheck(fileHandlesCheckName, fhFactory) +} diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_bsd_test.go b/pkg/collector/corechecks/system/filehandles/file_handles_bsd_test.go new file mode 100644 index 0000000000000..55c168d11e461 --- /dev/null +++ b/pkg/collector/corechecks/system/filehandles/file_handles_bsd_test.go @@ -0,0 +1,45 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. +//go:build freebsd || darwin + +package filehandles + +import ( + "testing" + + "github.com/DataDog/datadog-agent/pkg/aggregator/mocksender" + "github.com/DataDog/datadog-agent/pkg/autodiscovery/integration" +) + +func GetInt64(_ string) (value int64, err error) { + value = 65534 + err = nil + return +} + +func TestFhCheckFreeBSD(t *testing.T) { + getInt64 = GetInt64 + + // we have to init the mocked sender here before fileHandleCheck.Configure(mock.GetSenderManager(), integration.FakeConfigHash, ...) + // (and append it to the aggregator, which is automatically done in NewMockSender) + // because the FinalizeCheckServiceTag is called in Configure. + // Hopefully, the check ID is an empty string while running unit tests; + mock := mocksender.NewMockSender("") + + fileHandleCheck := new(fhCheck) + fileHandleCheck.Configure(mock.GetSenderManager(), integration.FakeConfigHash, nil, nil, "test") + + // reset the check ID for the sake of correctness + mocksender.SetSender(mock, fileHandleCheck.ID()) + + mock.On("Gauge", "system.fs.file_handles.used", float64(65534), "", []string(nil)).Return().Times(1) + mock.On("Gauge", "system.fs.file_handles.max", float64(65534), "", []string(nil)).Return().Times(1) + mock.On("Commit").Return().Times(1) + fileHandleCheck.Run() + + mock.AssertExpectations(t) + mock.AssertNumberOfCalls(t, "Gauge", 2) + mock.AssertNumberOfCalls(t, "Commit", 1) +} diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_darwin.go b/pkg/collector/corechecks/system/filehandles/file_handles_darwin.go new file mode 100644 index 0000000000000..fba9b26115cd4 --- /dev/null +++ b/pkg/collector/corechecks/system/filehandles/file_handles_darwin.go @@ -0,0 +1,8 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. + +package filehandles + +const openfilesOID = "kern.num_files" diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_freebsd.go b/pkg/collector/corechecks/system/filehandles/file_handles_freebsd.go index 3dabce43e9d17..9818f86001e78 100644 --- a/pkg/collector/corechecks/system/filehandles/file_handles_freebsd.go +++ b/pkg/collector/corechecks/system/filehandles/file_handles_freebsd.go @@ -2,70 +2,7 @@ // under the Apache License Version 2.0. // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2016-present Datadog, Inc. -//go:build freebsd package filehandles -import ( - "github.com/blabber/go-freebsd-sysctl/sysctl" - - "github.com/DataDog/datadog-agent/pkg/aggregator/sender" - "github.com/DataDog/datadog-agent/pkg/autodiscovery/integration" - "github.com/DataDog/datadog-agent/pkg/collector/check" - core "github.com/DataDog/datadog-agent/pkg/collector/corechecks" - "github.com/DataDog/datadog-agent/pkg/util/log" -) - -// For testing purpose -var getInt64 = sysctl.GetInt64 - -const fileHandlesCheckName = "file_handle" - -type fhCheck struct { - core.CheckBase -} - -// Run executes the check -func (c *fhCheck) Run() error { - - sender, err := c.GetSender() - if err != nil { - return err - } - openFh, err := getInt64("kern.openfiles") - if err != nil { - log.Warnf("Error getting kern.openfiles value %v", err) - return err - } - maxFh, err := getInt64("kern.maxfiles") - if err != nil { - log.Warnf("Error getting kern.maxfiles value %v", err) - return err - } - log.Debugf("Submitting kern.openfiles %v", openFh) - log.Debugf("Submitting kern.maxfiles %v", maxFh) - sender.Gauge("system.fs.file_handles.used", float64(openFh), "", nil) - sender.Gauge("system.fs.file_handles.max", float64(maxFh), "", nil) - sender.Commit() - - return nil -} - -// The check doesn't need configuration -func (c *fhCheck) Configure(senderManager sender.SenderManager, integrationConfigDigest uint64, data integration.Data, initConfig integration.Data, source string) (err error) { - if err := c.CommonConfigure(senderManager, integrationConfigDigest, initConfig, data, source); err != nil { - return err - } - - return err -} - -func fhFactory() check.Check { - return &fhCheck{ - CheckBase: core.NewCheckBase(fileHandlesCheckName), - } -} - -func init() { - core.RegisterCheck(fileHandlesCheckName, fhFactory) -} +const openfilesOID = "kern.openfiles" diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_freebsd_test.go b/pkg/collector/corechecks/system/filehandles/file_handles_freebsd_test.go deleted file mode 100644 index 04182e34b94f1..0000000000000 --- a/pkg/collector/corechecks/system/filehandles/file_handles_freebsd_test.go +++ /dev/null @@ -1,39 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2016-present Datadog, Inc. -//go:build freebsd - -package filehandles - -import ( - "testing" - - "github.com/DataDog/datadog-agent/pkg/aggregator" - "github.com/DataDog/datadog-agent/pkg/aggregator/mocksender" - "github.com/DataDog/datadog-agent/pkg/autodiscovery/integration" -) - -func GetInt64(name string) (value int64, err error) { - value = 65534 - err = nil - return -} - -func TestFhCheckFreeBSD(t *testing.T) { - getInt64 = GetInt64 - - fileHandleCheck := new(fhCheck) - fileHandleCheck.Configure(aggregator.NewNoOpSenderManager(), integration.FakeConfigHash, nil, nil, "test") - - mock := mocksender.NewMockSender(fileHandleCheck.ID()) - - mock.On("Gauge", "system.fs.file_handles.used", 421, "", []string(nil)).Return().Times(1) - mock.On("Gauge", "system.fs.file_handles.max", 65534, "", []string(nil)).Return().Times(1) - mock.On("Commit").Return().Times(1) - fileHandleCheck.Run() - - mock.AssertExpectations(t) - mock.AssertNumberOfCalls(t, "Gauge", 2) - mock.AssertNumberOfCalls(t, "Commit", 1) -} diff --git a/pkg/collector/corechecks/system/filehandles/file_handles_test.go b/pkg/collector/corechecks/system/filehandles/file_handles_test.go index 4db2d94133fdf..ab8dd2fa57935 100644 --- a/pkg/collector/corechecks/system/filehandles/file_handles_test.go +++ b/pkg/collector/corechecks/system/filehandles/file_handles_test.go @@ -2,7 +2,7 @@ // under the Apache License Version 2.0. // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2016-present Datadog, Inc. -//go:build !windows +//go:build !windows && !freebsd && !darwin package filehandles diff --git a/releasenotes/notes/file-check-on-darwin-ea525cc9369307a4.yaml b/releasenotes/notes/file-check-on-darwin-ea525cc9369307a4.yaml new file mode 100644 index 0000000000000..c0a8ca03a195c --- /dev/null +++ b/releasenotes/notes/file-check-on-darwin-ea525cc9369307a4.yaml @@ -0,0 +1,11 @@ +# Each section from every release note are combined when the +# CHANGELOG.rst is rendered. So the text needs to be worded so that +# it does not depend on any information only available in another +# section. This may mean repeating some details, but each section +# must be readable independently of the other. +# +# Each section note must be formatted as reStructuredText. +--- +fixes: + - | + Fix `file_handle` core check on Darwin by using `sysctl` system call.