diff --git a/.agents/skills/project-writing-go-modules-framework-v2/SKILL.md b/.agents/skills/project-writing-go-modules-framework-v2/SKILL.md index f47de2eed98434..5fc31239d3b300 100644 --- a/.agents/skills/project-writing-go-modules-framework-v2/SKILL.md +++ b/.agents/skills/project-writing-go-modules-framework-v2/SKILL.md @@ -67,6 +67,13 @@ source files for evidence. - If Functions exist, isolate them in a `func/` subpackage with a narrow `Deps` interface declared there. The Function package MUST NOT import the collector package or hold `*Collector`. +- If a single-instance collector exposes `AgentWide` module `Methods`, its + `MethodHandler(job)` receives the running canonical runtime job. Use + `job.Collector()` to bind the Function handler to collector-owned state; do + not add a `__job` parameter or introduce a package-global registry to bridge + Function dispatch. During method execution, when the singleton job is not + running, the framework returns an unavailable response before calling + `MethodHandler`. - `collectorapi.Creator.InstancePolicy` defaults to `InstancePolicyPerJob`. Use `InstancePolicySingle` only for collectors that are intentionally one canonical job per agent. Single-instance configs MUST diff --git a/.agents/sow/specs/snmp-traps/netdata.md b/.agents/sow/specs/snmp-traps/netdata.md index 78d7fe5757cfd7..05feb2568906af 100644 --- a/.agents/sow/specs/snmp-traps/netdata.md +++ b/.agents/sow/specs/snmp-traps/netdata.md @@ -784,7 +784,7 @@ _HOSTNAME= 0 ? (aclk_connection_counter - 1) : 0); json_object_object_add(msg, "reconnect-count", tmp); diff --git a/src/aclk/mqtt_websockets/common_public.h b/src/aclk/mqtt_websockets/common_public.h index 31ea4400128462..ef36ff8347168f 100644 --- a/src/aclk/mqtt_websockets/common_public.h +++ b/src/aclk/mqtt_websockets/common_public.h @@ -27,6 +27,8 @@ struct mqtt_ng_stats { int tx_messages_sent; int rx_messages_rcvd; int packets_waiting_puback; + // MQTT 5.0 server Receive Maximum from CONNACK (max concurrent unacked QoS1/2); 65535 if unset + uint16_t rx_maximum; size_t tx_buffer_used; size_t tx_buffer_free; size_t tx_buffer_size; diff --git a/src/aclk/mqtt_websockets/mqtt_ng.c b/src/aclk/mqtt_websockets/mqtt_ng.c index 88c690cc0a4593..c7672268153b4f 100644 --- a/src/aclk/mqtt_websockets/mqtt_ng.c +++ b/src/aclk/mqtt_websockets/mqtt_ng.c @@ -250,6 +250,9 @@ struct mqtt_ng_client { c_rhash rx_aliases; size_t max_msg_size; + + // MQTT 5.0 server Receive Maximum (CONNACK prop 0x21); absent => 65535 default [MQTT-3.2.2.3.3] + uint16_t rx_maximum; }; usec_t publish_latency; @@ -630,6 +633,9 @@ struct mqtt_ng_client *mqtt_ng_init(struct mqtt_ng_init *settings) client->tx_topic_aliases.stoi_dict = TX_ALIASES_INITIALIZE(); client->tx_topic_aliases.idx_max = UINT16_MAX; + // MQTT 5.0 default Receive Maximum when the server omits the property [MQTT-3.2.2.3.3] + __atomic_store_n(&client->rx_maximum, UINT16_MAX, __ATOMIC_RELAXED); + // TODO just embed the struct into mqtt_ng_client client->parser.received_data = settings->data_in; client->send_fnc_ptr = settings->data_out_fnc; @@ -2144,6 +2150,11 @@ int handle_incoming_traffic(struct mqtt_ng_client *client) client->max_msg_size = prop->data.uint32; } + if ((prop = get_property_by_id(client->parser.properties_parser.head, MQTT_PROP_RECEIVE_MAX)) != NULL) { + nd_log(NDLS_DAEMON, NDLP_INFO, "ACLK: MQTT server receive maximum is %" PRIu16, prop->data.uint16); + __atomic_store_n(&client->rx_maximum, prop->data.uint16, __ATOMIC_RELAXED); + } + if (client->connack_callback) client->connack_callback(client->user_ctx, client->parser.mqtt_packet.connack.reason_code); if (!client->parser.mqtt_packet.connack.reason_code) { @@ -2300,6 +2311,7 @@ void mqtt_ng_get_stats(struct mqtt_ng_client *client, struct mqtt_ng_stats *stat stats->tx_messages_sent = __atomic_load_n(&client->stats.tx_messages_sent, __ATOMIC_RELAXED); stats->rx_messages_rcvd = __atomic_load_n(&client->stats.rx_messages_rcvd, __ATOMIC_RELAXED); stats->packets_waiting_puback = __atomic_load_n(&client->stats.packets_waiting_puback, __ATOMIC_RELAXED); + stats->rx_maximum = __atomic_load_n(&client->rx_maximum, __ATOMIC_RELAXED); stats->tx_bytes_queued = 0; stats->tx_buffer_reclaimable = 0; diff --git a/src/claim/ui.c b/src/claim/ui.c index 92805d777be382..986577349a7cea 100644 --- a/src/claim/ui.c +++ b/src/claim/ui.c @@ -10,8 +10,6 @@ static LPCTSTR szWindowClass = _T("DesktopApp"); static HINSTANCE hInst; -static HWND hToken; -static HWND hRoom; LRESULT CALLBACK WndProc(HWND hNetdatawnd, UINT message, WPARAM wParam, LPARAM lParam) { diff --git a/src/collectors/cgroups.plugin/cgroup-discovery.c b/src/collectors/cgroups.plugin/cgroup-discovery.c index a895b8ae53bd99..483dbebd659d2f 100644 --- a/src/collectors/cgroups.plugin/cgroup-discovery.c +++ b/src/collectors/cgroups.plugin/cgroup-discovery.c @@ -794,7 +794,7 @@ static inline void discovery_update_filenames_all_cgroups() { static inline void discovery_cleanup_all_cgroups() { struct cgroup *cg = discovered_cgroup_root, *last = NULL; - for(; cg ;) { + while(cg) { if(!cg->available) { // enable the first duplicate cgroup { diff --git a/src/collectors/freebsd.plugin/freebsd_sysctl.c b/src/collectors/freebsd.plugin/freebsd_sysctl.c index 7dea4a71b61b5f..475ba1b098d0d7 100644 --- a/src/collectors/freebsd.plugin/freebsd_sysctl.c +++ b/src/collectors/freebsd.plugin/freebsd_sysctl.c @@ -574,9 +574,6 @@ int do_hw_intcnt(int update_every, usec_t dt) { for (i = 0; i < nintr; i++) totalintr += intrcnt[i]; - static RRDSET *st_intr = NULL; - static RRDDIM *rd_intr = NULL; - common_interrupts(totalintr, update_every, "hw.intrcnt"); size_t size; diff --git a/src/collectors/statsd.plugin/README.md b/src/collectors/statsd.plugin/README.md index 1d1019a655e613..5eed2e576f8f70 100644 --- a/src/collectors/statsd.plugin/README.md +++ b/src/collectors/statsd.plugin/README.md @@ -517,14 +517,22 @@ Example of a synthetic chart combining multiple metrics: The `[app]` section defines the application and has these options: +:::warning + +The `[app]` section is a **namespace/container** — it groups metrics and sets defaults, but does **not** create any dashboard charts by itself. To see synthetic charts on the dashboard, you **must** add one or more chart definition sections (e.g., `[mychart]`) below the `[app]` section. If you only define an `[app]` section without chart definitions, the only visible charts will be private charts for individual metrics (if `private charts = yes` or the global default is enabled). + +Settings like `private charts`, `gaps when not collected`, and `history` configure how the app's metrics and charts behave — they are not chart-level settings. The `memory mode` setting under `[app]` is currently ignored. See [Chart Definitions](#chart-definitions) below for how to create charts. + +::: + :::note - **name** - Defines the application name - **metrics** - [Simple pattern](https://github.com/netdata/netdata/blob/master/src/libnetdata/simple_pattern/README.md) matching all metrics for this app - **private charts** - Enable/disable private charts for matched metrics (yes|no) - **gaps when not collected** - Show gaps when no metrics are collected (yes|no) -- **memory mode** - Sets memory mode for application charts (optional, default is global Netdata setting) -- **history** - Size of round-robin database (optional, only relevant with `memory mode = save`) +- **memory mode** - Ignored in the `[app]` section; application charts use the host's default memory mode +- **history** - Size of round-robin database for application charts (optional, minimum 5) ::: @@ -919,7 +927,6 @@ Start with this basic configuration: metrics = k6* private charts = yes gaps when not collected = no - memory mode = dbengine ``` @@ -957,7 +964,6 @@ Here's a complete configuration for k6: metrics = k6* private charts = yes gaps when not collected = no - memory mode = dbengine [dictionary] http_req_blocked = Blocked HTTP Requests diff --git a/src/daemon/pulse/pulse-network.c b/src/daemon/pulse/pulse-network.c index ee5024cb1b2f5a..e173d45c3c46ea 100644 --- a/src/daemon/pulse/pulse-network.c +++ b/src/daemon/pulse/pulse-network.c @@ -335,6 +335,40 @@ void pulse_network_do(bool extended __maybe_unused) { pulse_aclk_time_heatmap(); + { + // In-flight QoS1 messages vs the broker's MQTT 5.0 Receive Maximum. + // When "in flight" approaches "receive maximum" the agent is at the + // broker's window limit. Always available (not extended) since it is + // the primary signal for the MQTT 5.0 Receive Maximum behavior. + static RRDSET *st_aclk_inflight = NULL; + static RRDDIM *rd_in_flight = NULL, *rd_receive_max = NULL; + + if (unlikely(!st_aclk_inflight)) { + st_aclk_inflight = rrdset_create_localhost( + "netdata", + "aclk_mqtt_inflight", + NULL, + PULSE_NETWORK_CHART_FAMILY, + "netdata.aclk_mqtt_inflight", + "Netdata ACLK MQTT In-Flight QoS1 Window", + "messages", + "netdata", + "pulse", + PULSE_NETWORK_CHART_PRIORITY + 2, + localhost->rrd_update_every, + RRDSET_TYPE_LINE); + + rrdlabels_add(st_aclk_inflight->rrdlabels, "endpoint", "aclk", RRDLABEL_SRC_AUTO); + + rd_in_flight = rrddim_add(st_aclk_inflight, "in flight", NULL, 1, 1, RRD_ALGORITHM_ABSOLUTE); + rd_receive_max = rrddim_add(st_aclk_inflight, "receive maximum", NULL, 1, 1, RRD_ALGORITHM_ABSOLUTE); + } + + rrddim_set_by_pointer(st_aclk_inflight, rd_in_flight, (collected_number)t.mqtt.packets_waiting_puback); + rrddim_set_by_pointer(st_aclk_inflight, rd_receive_max, (collected_number)t.mqtt.rx_maximum); + rrdset_done(st_aclk_inflight); + } + if(extended) { static RRDSET *st_aclk_queue_size = NULL; static RRDDIM *rd_messages = NULL; diff --git a/src/database/rrdhost-system-info.c b/src/database/rrdhost-system-info.c index c59a25b98c661b..6705f24f99bc2c 100644 --- a/src/database/rrdhost-system-info.c +++ b/src/database/rrdhost-system-info.c @@ -6,7 +6,10 @@ #include "daemon/win_system-info.h" // coverity[ +tainted_string_sanitize_content : arg-0 ] -static inline void coverity_remove_taint(char *s __maybe_unused) { } +static inline void coverity_remove_taint(char *s __maybe_unused) { + // intentionally empty: only a marker for the Coverity taint sanitizer + // (see the annotation above); it has no runtime effect. +} void rrdhost_system_info_swap(struct rrdhost_system_info *a, struct rrdhost_system_info *b) { if(a && b) diff --git a/src/go/plugin/agent/jobmgr/funcctl/controller_test.go b/src/go/plugin/agent/jobmgr/funcctl/controller_test.go index 3b7c72084800dc..87865dfbdd652a 100644 --- a/src/go/plugin/agent/jobmgr/funcctl/controller_test.go +++ b/src/go/plugin/agent/jobmgr/funcctl/controller_test.go @@ -938,6 +938,188 @@ func TestControllerRawAgentWideModuleMethodDoesNotRequireRunningJob(t *testing.T assert.Nil(t, gotJob) } +func TestControllerRawSingleInstanceAgentWideModuleMethodUsesRunningJob(t *testing.T) { + var gotCode int + var gotResp map[string]any + var gotJob collectorapi.RuntimeJob + reg := newTestFunctionRegistry() + controller := New(Options{ + FnReg: reg, + JSONWriter: func(data []byte, code int) { + gotCode = code + require.NoError(t, json.Unmarshal(data, &gotResp)) + }, + }) + controller.RegisterModules(collectorapi.Registry{ + "mod": collectorapi.Creator{ + InstancePolicy: collectorapi.InstancePolicySingle, + Methods: func() []funcapi.MethodConfig { + return []funcapi.MethodConfig{{ + ID: "logs", + RawRequest: true, + AgentWide: true, + }} + }, + MethodHandler: func(job collectorapi.RuntimeJob) funcapi.MethodHandler { + gotJob = job + return &rawTestHandler{ + raw: func(_ context.Context, req funcapi.RawMethodRequest) *funcapi.FunctionResponse { + assert.Equal(t, "logs", req.Method) + return funcapi.RawResponse(map[string]any{ + "status": 200, + "type": "table", + }) + }, + } + }, + }, + }) + job := newTestRuntimeJob("mod", "mod", true) + controller.OnJobStart(job) + + reg.call("mod:logs", context.Background(), functions.Function{ + UID: "raw-single-agent-wide", + Timeout: time.Second, + }) + + assert.Equal(t, 200, gotCode) + assert.Equal(t, float64(200), gotResp["status"]) + assert.Same(t, job, gotJob) +} + +func TestControllerSingleInstanceAgentWideModuleMethodUsesRunningJob(t *testing.T) { + var gotCode int + var gotResp map[string]any + var gotJob collectorapi.RuntimeJob + reg := newTestFunctionRegistry() + controller := New(Options{ + FnReg: reg, + JSONWriter: func(data []byte, code int) { + gotCode = code + require.NoError(t, json.Unmarshal(data, &gotResp)) + }, + }) + controller.RegisterModules(collectorapi.Registry{ + "mod": collectorapi.Creator{ + InstancePolicy: collectorapi.InstancePolicySingle, + Methods: func() []funcapi.MethodConfig { + return []funcapi.MethodConfig{{ + ID: "status", + AgentWide: true, + }} + }, + MethodHandler: func(job collectorapi.RuntimeJob) funcapi.MethodHandler { + gotJob = job + return &rawTestHandler{ + params: func(context.Context, string) ([]funcapi.ParamConfig, error) { + return []funcapi.ParamConfig{{ + ID: "scope", + Name: "Scope", + Selection: funcapi.ParamSelect, + Options: []funcapi.ParamOption{{ + ID: "all", + Name: "All", + Default: true, + }}, + }}, nil + }, + handle: func(_ context.Context, method string, params funcapi.ResolvedParams) *funcapi.FunctionResponse { + assert.Equal(t, "status", method) + assert.Equal(t, "all", params.GetOne("scope")) + return &funcapi.FunctionResponse{ + Status: 200, + ResponseType: "table", + Help: "status", + } + }, + } + }, + }, + }) + job := newTestRuntimeJob("mod", "mod", true) + controller.OnJobStart(job) + + reg.call("mod:status", context.Background(), functions.Function{ + UID: "single-agent-wide", + Timeout: time.Second, + Payload: []byte(`{"scope":"all"}`), + }) + + assert.Equal(t, 200, gotCode) + assert.Equal(t, float64(200), gotResp["status"]) + assert.Same(t, job, gotJob) + assert.Equal(t, []any{"scope"}, gotResp["accepted_params"]) +} + +func TestControllerSingleInstanceAgentWideModuleMethodRequiresRunningJob(t *testing.T) { + tests := map[string]struct { + setup func(*Controller) + message string + }{ + "before start": { + message: "module 'mod' is not running", + }, + "after stop": { + setup: func(controller *Controller) { + job := newTestRuntimeJob("mod", "mod", true) + controller.OnJobStart(job) + controller.OnJobStop(job) + }, + message: "module 'mod' is not running", + }, + "registered but not running": { + setup: func(controller *Controller) { + controller.OnJobStart(newTestRuntimeJob("mod", "mod", false)) + }, + message: "job 'mod' is no longer running", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + var gotCode int + var gotResp map[string]any + var gotHandler bool + reg := newTestFunctionRegistry() + controller := New(Options{ + FnReg: reg, + JSONWriter: func(data []byte, code int) { + gotCode = code + require.NoError(t, json.Unmarshal(data, &gotResp)) + }, + }) + controller.RegisterModules(collectorapi.Registry{ + "mod": collectorapi.Creator{ + InstancePolicy: collectorapi.InstancePolicySingle, + Methods: func() []funcapi.MethodConfig { + return []funcapi.MethodConfig{{ + ID: "status", + AgentWide: true, + }} + }, + MethodHandler: func(collectorapi.RuntimeJob) funcapi.MethodHandler { + gotHandler = true + return &tableTestHandler{} + }, + }, + }) + if tc.setup != nil { + tc.setup(controller) + } + + reg.call("mod:status", context.Background(), functions.Function{ + UID: "single-agent-wide-missing-job", + Timeout: time.Second, + }) + + assert.Equal(t, 503, gotCode) + assert.Equal(t, float64(503), gotResp["status"]) + assert.Equal(t, tc.message, gotResp["errorMessage"]) + assert.False(t, gotHandler) + }) + } +} + func TestControllerModuleMethodRequestContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -1119,13 +1301,17 @@ func (j *testRuntimeJob) IsRunning() bool { return j.running } func (j *testRuntimeJob) Collector() any { return nil } type rawTestHandler struct { + params func(context.Context, string) ([]funcapi.ParamConfig, error) handle func(context.Context, string, funcapi.ResolvedParams) *funcapi.FunctionResponse raw func(context.Context, funcapi.RawMethodRequest) *funcapi.FunctionResponse } var _ funcapi.RawMethodHandler = (*rawTestHandler)(nil) -func (h *rawTestHandler) MethodParams(context.Context, string) ([]funcapi.ParamConfig, error) { +func (h *rawTestHandler) MethodParams(ctx context.Context, method string) ([]funcapi.ParamConfig, error) { + if h.params != nil { + return h.params(ctx, method) + } return nil, nil } diff --git a/src/go/plugin/agent/jobmgr/funcctl/dispatch.go b/src/go/plugin/agent/jobmgr/funcctl/dispatch.go index a38e5d5f14577e..bd645fce147805 100644 --- a/src/go/plugin/agent/jobmgr/funcctl/dispatch.go +++ b/src/go/plugin/agent/jobmgr/funcctl/dispatch.go @@ -164,12 +164,20 @@ func (c *Controller) makeMethodFuncHandler(moduleName, methodID string) function includeJobParam := methodRequiresJobParam(methodCfg) if !includeJobParam { + job, jobName, jobGen, ok := c.resolveAgentWideMethodJob(fn, moduleName) + if !ok { + return + } + c.executeMethodRequest(ctx, methodExecutionInput{ fn: fn, moduleName: moduleName, + jobName: jobName, jobLabel: moduleName, methodID: methodID, methodCfg: methodCfg, + job: job, + jobGen: jobGen, info: info, payload: payload, argValues: argValues, @@ -239,6 +247,22 @@ func (c *Controller) makeMethodFuncHandler(moduleName, methodID string) function } } +func (c *Controller) resolveAgentWideMethodJob(fn functions.Function, moduleName string) (collectorapi.RuntimeJob, string, uint64, bool) { + creator, ok := c.registry.getCreator(moduleName) + if !ok || creator.InstancePolicy != collectorapi.InstancePolicySingle { + return nil, "", 0, true + } + + jobName := moduleName + job, jobGen := c.registry.getJobWithGeneration(moduleName, jobName) + if job == nil { + c.respondError(fn, 503, "module '%s' is not running", moduleName) + return nil, "", 0, false + } + + return job, jobName, jobGen, true +} + func (c *Controller) handleMethodFuncInfo(moduleName, methodID string, fn functions.Function) { methodCfg, ok := c.registry.getMethod(moduleName, methodID) if !ok { diff --git a/src/go/plugin/framework/collectorapi/registry.go b/src/go/plugin/framework/collectorapi/registry.go index 7268c4ff8c77da..10d93d0c5507b2 100644 --- a/src/go/plugin/framework/collectorapi/registry.go +++ b/src/go/plugin/framework/collectorapi/registry.go @@ -67,8 +67,12 @@ type ( Methods func() []funcapi.MethodConfig // Optional: MethodHandler returns a handler for method requests. - // AgentWide module methods are dispatched with nil job. Job-bound module - // methods and JobMethods are dispatched with the selected running job. + // AgentWide module methods are dispatched with nil job, except that + // single-instance collectors receive their running canonical job. + // Job-bound module methods and JobMethods are dispatched with the + // selected running job. + // When the canonical single-instance job is not running, dispatch returns + // unavailable before calling MethodHandler. // The handler implements funcapi.MethodHandler interface with: // - MethodParams(ctx, method) for dynamic params // - Handle(ctx, method, params) for request handling diff --git a/src/go/plugin/go.d/collector/init.go b/src/go/plugin/go.d/collector/init.go index d53ad72cd653e5..cdab9cef990e24 100644 --- a/src/go/plugin/go.d/collector/init.go +++ b/src/go/plugin/go.d/collector/init.go @@ -3,6 +3,11 @@ package collector import ( + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" + snmptraps "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_traps" + _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/activemq" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/adaptecraid" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/ap" @@ -108,9 +113,6 @@ import ( _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/scaleio" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/sensors" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/smartctl" - _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp" - _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" - _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_traps" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/spigotmc" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/sql" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/squid" @@ -140,3 +142,14 @@ import ( _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/zfspool" _ "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/zookeeper" ) + +func init() { + // These collectors share SNMP state; wire them together here instead of + // exposing package-global registries from the individual collector packages. + deviceStore := ddsnmp.NewDeviceStore() + trapEnrichment := snmptopology.NewTrapEnrichmentHandle() + + snmp.Register(deviceStore) + snmptopology.Register(deviceStore, trapEnrichment) + snmptraps.Register(deviceStore, trapEnrichment) +} diff --git a/src/go/plugin/go.d/collector/init_test.go b/src/go/plugin/go.d/collector/init_test.go new file mode 100644 index 00000000000000..649ea28e153e20 --- /dev/null +++ b/src/go/plugin/go.d/collector/init_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package collector + +import ( + "reflect" + "testing" + + "github.com/netdata/netdata/go/plugins/plugin/framework/collectorapi" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" + snmptraps "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_traps" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSNMPFamilyRegistrationUsesSharedDependencies(t *testing.T) { + snmpCreator := requireCreator(t, "snmp") + topologyCreator := requireCreator(t, "snmp_topology") + trapsCreator := requireCreator(t, "snmp_traps") + + assert.NotNil(t, snmpCreator.Create) + assert.Nil(t, snmpCreator.CreateV2) + assert.NotNil(t, snmpCreator.Config) + assert.NotNil(t, snmpCreator.Methods) + assert.NotNil(t, snmpCreator.MethodHandler) + assert.Equal(t, 10, snmpCreator.Defaults.UpdateEvery) + + assert.Nil(t, topologyCreator.Create) + assert.NotNil(t, topologyCreator.CreateV2) + assert.Equal(t, collectorapi.InstancePolicySingle, topologyCreator.InstancePolicy) + assert.False(t, topologyCreator.FunctionOnly) + assert.NotNil(t, topologyCreator.Methods) + assert.NotNil(t, topologyCreator.MethodHandler) + assert.Equal(t, 60, topologyCreator.Defaults.UpdateEvery) + + assert.Nil(t, trapsCreator.Create) + assert.NotNil(t, trapsCreator.CreateV2) + assert.NotNil(t, trapsCreator.Methods) + assert.NotNil(t, trapsCreator.MethodHandler) + assert.Equal(t, 1, trapsCreator.Defaults.UpdateEvery) + + snmpCollector, ok := snmpCreator.Create().(*snmp.Collector) + require.True(t, ok) + topologyCollector, ok := topologyCreator.CreateV2().(*snmptopology.Collector) + require.True(t, ok) + trapsCollector, ok := trapsCreator.CreateV2().(*snmptraps.Collector) + require.True(t, ok) + + deviceStore := pointerField(t, snmpCollector, "deviceStore") + require.NotZero(t, deviceStore) + assert.Equal(t, deviceStore, interfacePointerField(t, topologyCollector, "deviceSource")) + assert.Equal(t, deviceStore, interfacePointerField(t, trapsCollector, "deviceLookup")) + + trapEnrichment := pointerField(t, topologyCollector, "trapEnrichment") + require.NotZero(t, trapEnrichment) + assert.Equal(t, trapEnrichment, interfacePointerField(t, trapsCollector, "topologyEnricher")) +} + +func requireCreator(t *testing.T, module string) collectorapi.Creator { + t.Helper() + creator, ok := collectorapi.DefaultRegistry.Lookup(module) + require.True(t, ok, "collector %q is not registered", module) + return creator +} + +func pointerField(t *testing.T, obj any, name string) uintptr { + t.Helper() + field := reflect.ValueOf(obj).Elem().FieldByName(name) + require.True(t, field.IsValid(), "field %q not found", name) + require.Equal(t, reflect.Pointer, field.Kind(), "field %q", name) + require.False(t, field.IsNil(), "field %q is nil", name) + return field.Pointer() +} + +func interfacePointerField(t *testing.T, obj any, name string) uintptr { + t.Helper() + field := reflect.ValueOf(obj).Elem().FieldByName(name) + require.True(t, field.IsValid(), "field %q not found", name) + require.Equal(t, reflect.Interface, field.Kind(), "field %q", name) + require.False(t, field.IsNil(), "field %q is nil", name) + + elem := field.Elem() + require.Equal(t, reflect.Pointer, elem.Kind(), "field %q concrete value", name) + require.False(t, elem.IsNil(), "field %q concrete value is nil", name) + return elem.Pointer() +} diff --git a/src/go/plugin/go.d/collector/snmp/bgp_typed_metrics_test.go b/src/go/plugin/go.d/collector/snmp/bgp_typed_metrics_test.go index a8393426c38e14..7e0a827d07c850 100644 --- a/src/go/plugin/go.d/collector/snmp/bgp_typed_metrics_test.go +++ b/src/go/plugin/go.d/collector/snmp/bgp_typed_metrics_test.go @@ -291,7 +291,7 @@ func TestBGPIntegration_PreservesFunctionCacheOnProfileBGPError(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.sysInfo = &snmputils.SysInfo{} collr.enableBGPIntegration() collr.ddSnmpColl = &mockDdSnmpCollector{pms: tc.initial} @@ -339,7 +339,7 @@ func TestBGPIntegration_RecoveryClearsStaleFunctionRows(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.sysInfo = &snmputils.SysInfo{} collr.enableBGPIntegration() @@ -401,7 +401,7 @@ func TestBGPIntegration_MixedProfileBGPErrorRefreshesSuccessfulProfiles(t *testi for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.sysInfo = &snmputils.SysInfo{} collr.enableBGPIntegration() collr.ddSnmpColl = &mockDdSnmpCollector{pms: tc.initial} @@ -457,7 +457,7 @@ func TestBGPIntegration_ExpiredFailedProfileDoesNotKeepStaleRowsWithFreshProfile for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.sysInfo = &snmputils.SysInfo{} collr.enableBGPIntegration() collr.bgp.setStaleAfter(time.Minute) diff --git a/src/go/plugin/go.d/collector/snmp/charts_test.go b/src/go/plugin/go.d/collector/snmp/charts_test.go index c5f0810fd89a7b..6d26626fd4cdbf 100644 --- a/src/go/plugin/go.d/collector/snmp/charts_test.go +++ b/src/go/plugin/go.d/collector/snmp/charts_test.go @@ -14,7 +14,7 @@ import ( ) func TestCollector_AddProfileScalarMetricChart_LabelsIncludeMetricTags(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.Hostname = "192.0.2.1" collr.sysInfo = &snmputils.SysInfo{ Name: "test-device", @@ -132,7 +132,7 @@ func TestCollector_AddLicenseCharts_LazyBySignalClass(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() collr.sysInfo = &snmputils.SysInfo{} collr.addLicenseCharts(tc.agg) collr.addLicenseCharts(tc.agg) diff --git a/src/go/plugin/go.d/collector/snmp/collect.go b/src/go/plugin/go.d/collector/snmp/collect.go index b81fbcb86ec754..e6d911be5dbe65 100644 --- a/src/go/plugin/go.d/collector/snmp/collect.go +++ b/src/go/plugin/go.d/collector/snmp/collect.go @@ -140,7 +140,7 @@ func (c *Collector) ensureInitialized() error { c.addPingCharts() } - c.registerDeviceForTopology(si) + c.registerDeviceState(si) return nil } diff --git a/src/go/plugin/go.d/collector/snmp/collector.go b/src/go/plugin/go.d/collector/snmp/collector.go index 64c48d5d9c31ae..e86288c4b17486 100644 --- a/src/go/plugin/go.d/collector/snmp/collector.go +++ b/src/go/plugin/go.d/collector/snmp/collector.go @@ -23,20 +23,32 @@ import ( //go:embed "config_schema.json" var configSchema string -func init() { - collectorapi.Register("snmp", collectorapi.Creator{ +// Register registers the SNMP collector with its shared SNMP-family device store. +func Register(store *ddsnmp.DeviceStore) { + collectorapi.Register("snmp", newCreator(store)) +} + +func newCreator(store *ddsnmp.DeviceStore) collectorapi.Creator { + if store == nil { + panic("snmp Register requires a non-nil device store") + } + return collectorapi.Creator{ JobConfigSchema: configSchema, Defaults: collectorapi.Defaults{ UpdateEvery: 10, }, - Create: func() collectorapi.CollectorV1 { return New() }, + Create: func() collectorapi.CollectorV1 { return New(store) }, Config: func() any { return &Config{} }, Methods: snmpMethods, MethodHandler: snmpFunctionHandler, - }) + } } -func New() *Collector { +// New returns an SNMP collector using the provided SNMP-family device store. +func New(store *ddsnmp.DeviceStore) *Collector { + if store == nil { + panic("snmp New requires a non-nil device store") + } c := &Collector{ Config: Config{ CreateVnode: true, @@ -70,6 +82,7 @@ func New() *Collector { seenScalarMetrics: make(map[string]bool), seenTableMetrics: make(map[string]bool), seenProfiles: make(map[string]bool), + deviceStore: store, ifaceCache: newIfaceCache(), licensing: newLicensingIntegration(), @@ -98,6 +111,7 @@ type ( seenScalarMetrics map[string]bool seenTableMetrics map[string]bool seenProfiles map[string]bool + deviceStore *ddsnmp.DeviceStore ifaceCache *ifaceCache // interface metrics cache for functions licensing *licensingIntegration @@ -193,7 +207,9 @@ func (c *Collector) Cleanup(ctx context.Context) { if c.funcRouter != nil { c.funcRouter.Cleanup(ctx) } - ddsnmp.DeviceRegistry.Unregister(c.deviceRegistryKey()) + if c.deviceStore != nil { + c.deviceStore.Unregister(c.deviceStoreKey()) + } if c.snmpClient != nil { _ = c.snmpClient.Close() } diff --git a/src/go/plugin/go.d/collector/snmp/collector_licensing_edge_test.go b/src/go/plugin/go.d/collector/snmp/collector_licensing_edge_test.go index 8de3ef781b88fd..2a14aa2bd4b16f 100644 --- a/src/go/plugin/go.d/collector/snmp/collector_licensing_edge_test.go +++ b/src/go/plugin/go.d/collector/snmp/collector_licensing_edge_test.go @@ -70,7 +70,7 @@ func TestCollector_Collect_LicensingAggregation_ReadsTypedRowsAndIgnoresPrivateM setMockClientInitExpect(mockSNMP) setMockClientSysInfoExpect(mockSNMP) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = false diff --git a/src/go/plugin/go.d/collector/snmp/collector_test.go b/src/go/plugin/go.d/collector/snmp/collector_test.go index a3e8975e1f2655..2b2239857d0fd5 100644 --- a/src/go/plugin/go.d/collector/snmp/collector_test.go +++ b/src/go/plugin/go.d/collector/snmp/collector_test.go @@ -46,6 +46,18 @@ func TestCollector_ConfigurationSerialize(t *testing.T) { collecttest.TestConfigurationSerialize(t, &Collector{}, dataConfigJSON, dataConfigYAML) } +func TestCollectorCreatorRequiresDeviceStore(t *testing.T) { + require.PanicsWithValue(t, "snmp Register requires a non-nil device store", func() { + _ = newCreator(nil) + }) +} + +func TestCollectorNewRequiresDeviceStore(t *testing.T) { + require.PanicsWithValue(t, "snmp New requires a non-nil device store", func() { + _ = New(nil) + }) +} + func TestCollector_Init(t *testing.T) { tests := map[string]struct { prepareSNMP func() *Collector @@ -54,13 +66,13 @@ func TestCollector_Init(t *testing.T) { "fail with default config": { wantFail: true, prepareSNMP: func() *Collector { - return New() + return newTestSNMPCollector() }, }, "fail when using SNMPv3 but 'user.name' not set": { wantFail: true, prepareSNMP: func() *Collector { - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV3Config() collr.User.Name = "" return collr @@ -69,7 +81,7 @@ func TestCollector_Init(t *testing.T) { "success when using SNMPv1 with valid config": { wantFail: false, prepareSNMP: func() *Collector { - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV1Config() return collr }, @@ -77,7 +89,7 @@ func TestCollector_Init(t *testing.T) { "success when using SNMPv2 with valid config": { wantFail: false, prepareSNMP: func() *Collector { - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() return collr }, @@ -85,7 +97,7 @@ func TestCollector_Init(t *testing.T) { "success when using SNMPv3 with valid config": { wantFail: false, prepareSNMP: func() *Collector { - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV3Config() return collr }, @@ -108,7 +120,7 @@ func TestCollector_Init(t *testing.T) { func TestCollector_InitPassesSharedPingerConfig(t *testing.T) { var gotCfg pinger.Config - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.PingOnly = true collr.Ping.Network = "ip6" @@ -139,9 +151,14 @@ func TestCollector_Cleanup(t *testing.T) { tests := map[string]struct { prepareSNMP func(t *testing.T, m *snmpmock.MockHandler) *Collector }{ + "cleanup call does not panic on zero value collector": { + prepareSNMP: func(t *testing.T, m *snmpmock.MockHandler) *Collector { + return &Collector{} + }, + }, "cleanup call does not panic if snmpClient not initialized": { prepareSNMP: func(t *testing.T, m *snmpmock.MockHandler) *Collector { - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.newSnmpClient = func() gosnmp.Handler { return m } setMockClientInitExpect(m) @@ -167,6 +184,46 @@ func TestCollector_Cleanup(t *testing.T) { } } +func TestCollector_CollectRegistersAndCleanupUnregistersDevice(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockSNMP := snmpmock.NewMockHandler(ctrl) + setMockClientInitExpect(mockSNMP) + setMockClientSysInfoExpect(mockSNMP) + mockSNMP.EXPECT().Close().Return(nil).AnyTimes() + + deviceStore := ddsnmp.NewDeviceStore() + collr := New(deviceStore) + collr.Config = prepareV2Config() + collr.CreateVnode = false + collr.Ping.Enabled = false + collr.snmpProfiles = []*ddsnmp.Profile{{}} + collr.newSnmpClient = func() gosnmp.Handler { return mockSNMP } + collr.newDdSnmpColl = func(ddsnmpcollector.Config) ddCollector { + return &mockDdSnmpCollector{} + } + + require.NoError(t, collr.Init(context.Background())) + require.NoError(t, collr.Check(context.Background())) + require.Empty(t, deviceStore.Devices()) + + _ = collr.Collect(context.Background()) + + devices := deviceStore.Devices() + require.Len(t, devices, 1) + assert.Equal(t, "192.0.2.1", devices[0].Hostname) + assert.Equal(t, 161, devices[0].Port) + assert.Equal(t, gosnmp.Version2c.String(), devices[0].SNMPVersion) + assert.Equal(t, "mock sysName", devices[0].SysName) + assert.Equal(t, "mock sysDescr", devices[0].SysDescr) + assert.Equal(t, "mock sysContact", devices[0].SysContact) + assert.Equal(t, "mock sysLocation", devices[0].SysLocation) + + collr.Cleanup(context.Background()) + require.Empty(t, deviceStore.Devices()) +} + func TestCollector_Check(t *testing.T) { tests := map[string]struct { prepare func(m *snmpmock.MockHandler) *Collector @@ -178,7 +235,7 @@ func TestCollector_Check(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.CreateVnode = false c.Ping.Enabled = false @@ -193,7 +250,7 @@ func TestCollector_Check(t *testing.T) { setMockClientSetterExpect(m) m.EXPECT().Connect().Return(errors.New("connect failed")).AnyTimes() - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.CreateVnode = false c.Ping.Enabled = false @@ -214,7 +271,7 @@ func TestCollector_Check(t *testing.T) { WalkAll(gomock.Any()). Return(nil, errors.New("walk failed")) - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.CreateVnode = false c.Ping.Enabled = false @@ -229,7 +286,7 @@ func TestCollector_Check(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.PingOnly = true c.CreateVnode = false @@ -247,7 +304,7 @@ func TestCollector_Check(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.PingOnly = true c.CreateVnode = false @@ -265,7 +322,7 @@ func TestCollector_Check(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - c := New() + c := newTestSNMPCollector() c.Config = prepareV2Config() c.PingOnly = true c.CreateVnode = false @@ -310,7 +367,7 @@ func TestCollector_CheckPingOnlyUsesReadOnlyProbing(t *testing.T) { pingClient := &mockPingClient{sample: pingSuccessSample("192.0.2.1")} - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.PingOnly = true collr.CreateVnode = false @@ -340,7 +397,7 @@ func TestCollector_Collect(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = false @@ -400,7 +457,7 @@ func TestCollector_Collect(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = false @@ -469,7 +526,7 @@ func TestCollector_Collect(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.PingOnly = true collr.CreateVnode = false @@ -495,7 +552,7 @@ func TestCollector_Collect(t *testing.T) { setMockClientInitExpect(m) setMockClientSysInfoExpect(m) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.PingOnly = true collr.CreateVnode = false @@ -537,7 +594,7 @@ func TestCollector_CollectPingOnlyUsesTrackingProbing(t *testing.T) { pingClient := &mockPingClient{sample: pingSuccessSample("192.0.2.1")} - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.PingOnly = true collr.CreateVnode = false @@ -579,7 +636,7 @@ func TestCollector_CollectMixedModeCollectsSNMPAndPingMetrics(t *testing.T) { pingClient := &mockPingClient{sample: pingSuccessSample("192.0.2.1")} - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = true @@ -896,7 +953,7 @@ func TestCollector_Collect_LicensingAggregation(t *testing.T) { setMockClientSysInfoExpect(mockSNMP) now := time.Now().UTC() - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = false diff --git a/src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry_test.go b/src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry_test.go deleted file mode 100644 index 56bcf1cf0c338e..00000000000000 --- a/src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry_test.go +++ /dev/null @@ -1,83 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later - -package ddsnmp - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func TestDeviceRegistryDeviceByHostname(t *testing.T) { - reg := &deviceRegistry{devices: make(map[string]DeviceConnectionInfo)} - reg.Register("switch-a", DeviceConnectionInfo{ - Hostname: "192.0.2.10", - SysName: "switch-a", - ManualProfiles: []string{"profile-a"}, - VnodeLabels: map[string]string{"site": "lab"}, - }) - - dev, ok := reg.DeviceByHostname("::ffff:192.0.2.10") - require.True(t, ok) - require.Equal(t, "switch-a", dev.SysName) - - dev.ManualProfiles[0] = "changed" - dev.VnodeLabels["site"] = "changed" - - again, ok := reg.DeviceByHostname("192.0.2.10") - require.True(t, ok) - require.Equal(t, []string{"profile-a"}, again.ManualProfiles) - require.Equal(t, "lab", again.VnodeLabels["site"]) -} - -func TestDeviceRegistryDeviceByHostnameNoMatch(t *testing.T) { - reg := &deviceRegistry{devices: make(map[string]DeviceConnectionInfo)} - reg.Register("switch-a", DeviceConnectionInfo{Hostname: "switch-a.example.com"}) - - _, ok := reg.DeviceByHostname("switch-b.example.com") - require.False(t, ok) -} - -func TestDeviceRegistryDeviceByHostnameMatchesDNSCaseInsensitive(t *testing.T) { - reg := &deviceRegistry{devices: make(map[string]DeviceConnectionInfo)} - reg.Register("switch-a", DeviceConnectionInfo{Hostname: "Switch-A.Example.COM"}) - - _, ok := reg.DeviceByHostname("switch-a.example.com") - require.True(t, ok) -} - -func TestDeviceRegistryDeviceByHostnameIndexUpdatesOnRegisterAndUnregister(t *testing.T) { - reg := &deviceRegistry{devices: make(map[string]DeviceConnectionInfo)} - reg.Register("switch-a", DeviceConnectionInfo{Hostname: "192.0.2.10", SysName: "switch-a"}) - - dev, ok := reg.DeviceByHostname("192.0.2.10") - require.True(t, ok) - require.Equal(t, "switch-a", dev.SysName) - - reg.Register("switch-a", DeviceConnectionInfo{Hostname: "192.0.2.11", SysName: "switch-a-renumbered"}) - - _, ok = reg.DeviceByHostname("192.0.2.10") - require.False(t, ok) - dev, ok = reg.DeviceByHostname("192.0.2.11") - require.True(t, ok) - require.Equal(t, "switch-a-renumbered", dev.SysName) - - reg.Unregister("switch-a") - _, ok = reg.DeviceByHostname("192.0.2.11") - require.False(t, ok) -} - -func TestDeviceRegistryDevicesByHostnameReturnsAllMatches(t *testing.T) { - reg := &deviceRegistry{devices: make(map[string]DeviceConnectionInfo)} - reg.Register("switch-b", DeviceConnectionInfo{Hostname: "192.0.2.10", SysName: "switch-b"}) - reg.Register("switch-a", DeviceConnectionInfo{Hostname: "::ffff:192.0.2.10", SysName: "switch-a"}) - - devices := reg.DevicesByHostname("192.0.2.10") - require.Len(t, devices, 2) - require.Equal(t, "switch-a", devices[0].SysName) - require.Equal(t, "switch-b", devices[1].SysName) - - devices[0].SysName = "changed" - again := reg.DevicesByHostname("192.0.2.10") - require.Equal(t, "switch-a", again[0].SysName) -} diff --git a/src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry.go b/src/go/plugin/go.d/collector/snmp/ddsnmp/device_store.go similarity index 53% rename from src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry.go rename to src/go/plugin/go.d/collector/snmp/ddsnmp/device_store.go index 0aa29cf0c695cc..97dc19725c0ecd 100644 --- a/src/go/plugin/go.d/collector/snmp/ddsnmp/device_registry.go +++ b/src/go/plugin/go.d/collector/snmp/ddsnmp/device_store.go @@ -11,7 +11,7 @@ import ( ) // DeviceConnectionInfo holds SNMP connection parameters for a device. -// Registered by SNMP collector jobs, consumed by the topology collector. +// Registered by SNMP collector jobs, consumed by SNMP-family modules. type DeviceConnectionInfo struct { Hostname string Port int @@ -45,79 +45,70 @@ type DeviceConnectionInfo struct { VnodeLabels map[string]string } -// DeviceRegistry is a global registry where SNMP jobs register their connection -// info so the topology collector can discover which devices to poll. -var DeviceRegistry = &deviceRegistry{ - devices: make(map[string]DeviceConnectionInfo), - byHostname: make(map[string]map[string]struct{}), +// NewDeviceStore returns an empty SNMP device connection-state store. +func NewDeviceStore() *DeviceStore { + return &DeviceStore{ + devices: make(map[string]DeviceConnectionInfo), + byHostname: make(map[string]map[string]struct{}), + } } -type deviceRegistry struct { +// DeviceStore holds SNMP device connection state shared between SNMP-family modules. +type DeviceStore struct { mu sync.RWMutex devices map[string]DeviceConnectionInfo byHostname map[string]map[string]struct{} } -// Register adds or updates a device in the registry. +// Register adds or updates a device in the store. // Reference types are deep-copied to prevent data races with the caller. -func (r *deviceRegistry) Register(key string, info DeviceConnectionInfo) { - r.mu.Lock() - r.ensureMapsLocked() - if old, ok := r.devices[key]; ok { - r.removeHostnameIndexLocked(key, old.Hostname) - } - r.devices[key] = cloneDeviceConnectionInfo(info) - r.addHostnameIndexLocked(key, info.Hostname) - r.mu.Unlock() +func (s *DeviceStore) Register(key string, info DeviceConnectionInfo) { + s.mu.Lock() + s.ensureMapsLocked() + if old, ok := s.devices[key]; ok { + s.removeHostnameIndexLocked(key, old.Hostname) + } + s.devices[key] = cloneDeviceConnectionInfo(info) + s.addHostnameIndexLocked(key, info.Hostname) + s.mu.Unlock() } -// Unregister removes a device from the registry. -func (r *deviceRegistry) Unregister(key string) { - r.mu.Lock() - if old, ok := r.devices[key]; ok { - r.removeHostnameIndexLocked(key, old.Hostname) +// Unregister removes a device from the store. +func (s *DeviceStore) Unregister(key string) { + s.mu.Lock() + if old, ok := s.devices[key]; ok { + s.removeHostnameIndexLocked(key, old.Hostname) } - delete(r.devices, key) - r.mu.Unlock() + delete(s.devices, key) + s.mu.Unlock() } // Devices returns a deep-copied snapshot of all registered devices. -func (r *deviceRegistry) Devices() []DeviceConnectionInfo { - r.mu.RLock() - defer r.mu.RUnlock() +func (s *DeviceStore) Devices() []DeviceConnectionInfo { + s.mu.RLock() + defer s.mu.RUnlock() - devices := make([]DeviceConnectionInfo, 0, len(r.devices)) - for _, info := range r.devices { + devices := make([]DeviceConnectionInfo, 0, len(s.devices)) + for _, info := range s.devices { devices = append(devices, cloneDeviceConnectionInfo(info)) } return devices } -// DeviceByHostname returns a deep-copied registered device whose configured -// hostname matches the provided value. IP literals are normalized before -// comparison; DNS names are matched case-insensitively. -func (r *deviceRegistry) DeviceByHostname(hostname string) (DeviceConnectionInfo, bool) { - devices := r.DevicesByHostname(hostname) - if len(devices) == 0 { - return DeviceConnectionInfo{}, false - } - return devices[0], true -} - // DevicesByHostname returns all deep-copied registered devices whose configured // hostname matches the provided value. IP literals are normalized before // comparison; DNS names are matched case-insensitively. -func (r *deviceRegistry) DevicesByHostname(hostname string) []DeviceConnectionInfo { +func (s *DeviceStore) DevicesByHostname(hostname string) []DeviceConnectionInfo { hostnameKey := deviceHostnameIndexKey(hostname) if hostnameKey == "" { return nil } - r.mu.RLock() - defer r.mu.RUnlock() + s.mu.RLock() + defer s.mu.RUnlock() - if r.byHostname != nil { - keySet := r.byHostname[hostnameKey] + if s.byHostname != nil { + keySet := s.byHostname[hostnameKey] if len(keySet) == 0 { return nil } @@ -129,7 +120,7 @@ func (r *deviceRegistry) DevicesByHostname(hostname string) []DeviceConnectionIn devices := make([]DeviceConnectionInfo, 0, len(keys)) for _, key := range keys { - info, ok := r.devices[key] + info, ok := s.devices[key] if ok { devices = append(devices, cloneDeviceConnectionInfo(info)) } @@ -138,7 +129,7 @@ func (r *deviceRegistry) DevicesByHostname(hostname string) []DeviceConnectionIn } devices := make([]DeviceConnectionInfo, 0, 1) - for _, info := range r.devices { + for _, info := range s.devices { if deviceHostnameIndexKey(info.Hostname) == hostnameKey { devices = append(devices, cloneDeviceConnectionInfo(info)) } @@ -146,13 +137,6 @@ func (r *deviceRegistry) DevicesByHostname(hostname string) []DeviceConnectionIn return devices } -// Len returns the number of registered devices. -func (r *deviceRegistry) Len() int { - r.mu.RLock() - defer r.mu.RUnlock() - return len(r.devices) -} - func cloneDeviceConnectionInfo(info DeviceConnectionInfo) DeviceConnectionInfo { dev := info if info.ManualProfiles != nil { @@ -166,46 +150,46 @@ func cloneDeviceConnectionInfo(info DeviceConnectionInfo) DeviceConnectionInfo { return dev } -func (r *deviceRegistry) ensureMapsLocked() { - if r.devices == nil { - r.devices = make(map[string]DeviceConnectionInfo) +func (s *DeviceStore) ensureMapsLocked() { + if s.devices == nil { + s.devices = make(map[string]DeviceConnectionInfo) } - if r.byHostname == nil { - r.byHostname = make(map[string]map[string]struct{}) - for key, info := range r.devices { - r.addHostnameIndexLocked(key, info.Hostname) + if s.byHostname == nil { + s.byHostname = make(map[string]map[string]struct{}) + for key, info := range s.devices { + s.addHostnameIndexLocked(key, info.Hostname) } } } -func (r *deviceRegistry) addHostnameIndexLocked(key, hostname string) { +func (s *DeviceStore) addHostnameIndexLocked(key, hostname string) { hostnameKey := deviceHostnameIndexKey(hostname) if hostnameKey == "" { return } - if r.byHostname == nil { - r.byHostname = make(map[string]map[string]struct{}) + if s.byHostname == nil { + s.byHostname = make(map[string]map[string]struct{}) } - keySet := r.byHostname[hostnameKey] + keySet := s.byHostname[hostnameKey] if keySet == nil { keySet = make(map[string]struct{}) - r.byHostname[hostnameKey] = keySet + s.byHostname[hostnameKey] = keySet } keySet[key] = struct{}{} } -func (r *deviceRegistry) removeHostnameIndexLocked(key, hostname string) { +func (s *DeviceStore) removeHostnameIndexLocked(key, hostname string) { hostnameKey := deviceHostnameIndexKey(hostname) - if hostnameKey == "" || r.byHostname == nil { + if hostnameKey == "" || s.byHostname == nil { return } - keySet := r.byHostname[hostnameKey] + keySet := s.byHostname[hostnameKey] if keySet == nil { return } delete(keySet, key) if len(keySet) == 0 { - delete(r.byHostname, hostnameKey) + delete(s.byHostname, hostnameKey) } } diff --git a/src/go/plugin/go.d/collector/snmp/ddsnmp/device_store_test.go b/src/go/plugin/go.d/collector/snmp/ddsnmp/device_store_test.go new file mode 100644 index 00000000000000..d22434bd22357d --- /dev/null +++ b/src/go/plugin/go.d/collector/snmp/ddsnmp/device_store_test.go @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package ddsnmp + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDeviceStoreDevicesByHostname(t *testing.T) { + store := NewDeviceStore() + store.Register("switch-a", DeviceConnectionInfo{ + Hostname: "192.0.2.10", + SysName: "switch-a", + ManualProfiles: []string{"profile-a"}, + VnodeLabels: map[string]string{"site": "lab"}, + }) + + devices := store.DevicesByHostname("::ffff:192.0.2.10") + require.Len(t, devices, 1) + dev := devices[0] + require.Equal(t, "switch-a", dev.SysName) + + dev.ManualProfiles[0] = "changed" + dev.VnodeLabels["site"] = "changed" + + again := store.DevicesByHostname("192.0.2.10") + require.Len(t, again, 1) + require.Equal(t, []string{"profile-a"}, again[0].ManualProfiles) + require.Equal(t, "lab", again[0].VnodeLabels["site"]) +} + +func TestDeviceStoreDevicesByHostnameNoMatch(t *testing.T) { + store := NewDeviceStore() + store.Register("switch-a", DeviceConnectionInfo{Hostname: "switch-a.example.com"}) + + require.Empty(t, store.DevicesByHostname("switch-b.example.com")) +} + +func TestDeviceStoreDevicesByHostnameMatchesDNSCaseInsensitive(t *testing.T) { + store := NewDeviceStore() + store.Register("switch-a", DeviceConnectionInfo{Hostname: "Switch-A.Example.COM"}) + + require.Len(t, store.DevicesByHostname("switch-a.example.com"), 1) +} + +func TestDeviceStoreDevicesByHostnameIndexUpdatesOnRegisterAndUnregister(t *testing.T) { + store := NewDeviceStore() + store.Register("switch-a", DeviceConnectionInfo{Hostname: "192.0.2.10", SysName: "switch-a"}) + + devices := store.DevicesByHostname("192.0.2.10") + require.Len(t, devices, 1) + dev := devices[0] + require.Equal(t, "switch-a", dev.SysName) + + store.Register("switch-a", DeviceConnectionInfo{Hostname: "192.0.2.11", SysName: "switch-a-renumbered"}) + + require.Empty(t, store.DevicesByHostname("192.0.2.10")) + devices = store.DevicesByHostname("192.0.2.11") + require.Len(t, devices, 1) + dev = devices[0] + require.Equal(t, "switch-a-renumbered", dev.SysName) + + store.Unregister("switch-a") + require.Empty(t, store.DevicesByHostname("192.0.2.11")) +} + +func TestDeviceStoreDevicesByHostnameReturnsAllMatches(t *testing.T) { + store := NewDeviceStore() + store.Register("switch-b", DeviceConnectionInfo{Hostname: "192.0.2.10", SysName: "switch-b"}) + store.Register("switch-a", DeviceConnectionInfo{Hostname: "::ffff:192.0.2.10", SysName: "switch-a"}) + + devices := store.DevicesByHostname("192.0.2.10") + require.Len(t, devices, 2) + require.Equal(t, "switch-a", devices[0].SysName) + require.Equal(t, "switch-b", devices[1].SysName) + + devices[0].SysName = "changed" + again := store.DevicesByHostname("192.0.2.10") + require.Equal(t, "switch-a", again[0].SysName) +} + +func TestDeviceStoreRegisterClonesReferenceFields(t *testing.T) { + store := NewDeviceStore() + info := DeviceConnectionInfo{ + Hostname: "192.0.2.10", + ManualProfiles: []string{"profile-a"}, + VnodeLabels: map[string]string{"site": "lab"}, + } + + store.Register("switch-a", info) + info.ManualProfiles[0] = "changed" + info.VnodeLabels["site"] = "changed" + + devices := store.Devices() + require.Len(t, devices, 1) + require.Equal(t, []string{"profile-a"}, devices[0].ManualProfiles) + require.Equal(t, "lab", devices[0].VnodeLabels["site"]) +} diff --git a/src/go/plugin/go.d/collector/snmp/topology_device_registry.go b/src/go/plugin/go.d/collector/snmp/device_state.go similarity index 83% rename from src/go/plugin/go.d/collector/snmp/topology_device_registry.go rename to src/go/plugin/go.d/collector/snmp/device_state.go index ca292231b36995..e479ad6050a64c 100644 --- a/src/go/plugin/go.d/collector/snmp/topology_device_registry.go +++ b/src/go/plugin/go.d/collector/snmp/device_state.go @@ -42,14 +42,17 @@ func (c *Collector) vnodeLabels() map[string]string { return nil } -func (c *Collector) deviceRegistryKey() string { +func (c *Collector) deviceStoreKey() string { return fmt.Sprintf("%p:%s:%d", c, c.Hostname, c.Options.Port) } -// registerDeviceForTopology exposes the already-configured SNMP job to the -// snmp_topology collector without duplicating job configuration. -func (c *Collector) registerDeviceForTopology(si *snmputils.SysInfo) { - ddsnmp.DeviceRegistry.Register(c.deviceRegistryKey(), ddsnmp.DeviceConnectionInfo{ +// registerDeviceState exposes the already-configured SNMP job to SNMP-family +// consumers without duplicating job configuration. +func (c *Collector) registerDeviceState(si *snmputils.SysInfo) { + if c.deviceStore == nil { + return + } + c.deviceStore.Register(c.deviceStoreKey(), ddsnmp.DeviceConnectionInfo{ Hostname: c.Hostname, Port: c.Options.Port, SNMPVersion: c.Options.Version, diff --git a/src/go/plugin/go.d/collector/snmp/func_bgp_peers_test.go b/src/go/plugin/go.d/collector/snmp/func_bgp_peers_test.go index de860777001a6f..f12a5bfcce4cde 100644 --- a/src/go/plugin/go.d/collector/snmp/func_bgp_peers_test.go +++ b/src/go/plugin/go.d/collector/snmp/func_bgp_peers_test.go @@ -428,7 +428,7 @@ func TestCollector_BGPFunctionHandlerIsAlwaysRegistered(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - collr := New() + collr := newTestSNMPCollector() if tc.prepare != nil { tc.prepare(collr) } @@ -453,7 +453,7 @@ func TestCollectSNMP_HidesBGPDiagnosticsButKeepsFunctionCache(t *testing.T) { setMockClientInitExpect(mockSNMP) setMockClientSysInfoExpect(mockSNMP) - collr := New() + collr := newTestSNMPCollector() collr.Config = prepareV2Config() collr.CreateVnode = false collr.Ping.Enabled = false diff --git a/src/go/plugin/go.d/collector/snmp/test_helpers_test.go b/src/go/plugin/go.d/collector/snmp/test_helpers_test.go new file mode 100644 index 00000000000000..6b31949d8e5e57 --- /dev/null +++ b/src/go/plugin/go.d/collector/snmp/test_helpers_test.go @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package snmp + +import "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + +func newTestSNMPCollector() *Collector { + return New(ddsnmp.NewDeviceStore()) +} diff --git a/src/go/plugin/go.d/collector/snmp_topology/charts_test.go b/src/go/plugin/go.d/collector/snmp_topology/charts_test.go index e89373b9293bfe..053c78f01e2dd4 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/charts_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/charts_test.go @@ -13,7 +13,7 @@ import ( ) func TestCollector_ChartTemplateYAML(t *testing.T) { - raw := New().ChartTemplateYAML() + raw := newTestSNMPTopologyCollector().ChartTemplateYAML() collecttest.AssertChartTemplateSchema(t, raw) spec, err := charttpl.DecodeYAML([]byte(raw)) diff --git a/src/go/plugin/go.d/collector/snmp_topology/collector.go b/src/go/plugin/go.d/collector/snmp_topology/collector.go index 5725401c26f4bd..f96c4ccad7760a 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/collector.go +++ b/src/go/plugin/go.d/collector/snmp_topology/collector.go @@ -25,32 +25,52 @@ import ( //go:embed "config_schema.json" var configSchema string -func init() { - collectorapi.Register("snmp_topology", collectorapi.Creator{ +// Register registers the SNMP topology collector with shared SNMP-family state. +func Register(deviceStore *ddsnmp.DeviceStore, trapEnrichment *TrapEnrichmentHandle) { + collectorapi.Register("snmp_topology", newCreator(deviceStore, trapEnrichment)) +} + +func newCreator(deviceStore *ddsnmp.DeviceStore, trapEnrichment *TrapEnrichmentHandle) collectorapi.Creator { + if deviceStore == nil { + panic("snmp_topology Register requires a non-nil device store") + } + if trapEnrichment == nil { + panic("snmp_topology Register requires a non-nil trap enrichment handle") + } + return collectorapi.Creator{ JobConfigSchema: configSchema, Defaults: collectorapi.Defaults{ UpdateEvery: 60, }, - CreateV2: func() collectorapi.CollectorV2 { return New() }, + CreateV2: func() collectorapi.CollectorV2 { return New(deviceStore, trapEnrichment) }, Config: func() any { return &Config{} }, InstancePolicy: collectorapi.InstancePolicySingle, Methods: topologyMethods, MethodHandler: topologyFunctionHandler, - }) + } } -func New() *Collector { - store := metrix.NewCollectorStore() +// New returns an SNMP topology collector using the provided SNMP-family state. +func New(deviceStore *ddsnmp.DeviceStore, trapEnrichment *TrapEnrichmentHandle) *Collector { + if deviceStore == nil { + panic("snmp_topology New requires a non-nil device store") + } + if trapEnrichment == nil { + panic("snmp_topology New requires a non-nil trap enrichment handle") + } + metricStore := metrix.NewCollectorStore() return &Collector{ deviceCaches: make(map[string]*topologyCache), deviceLastCollected: make(map[string]time.Time), - registeredDevices: ddsnmp.DeviceRegistry.Devices, + topologyRegistry: newTopologyRegistry(), + deviceSource: deviceStore, + trapEnrichment: trapEnrichment, newSnmpClient: gosnmp.NewHandler, newDdSnmpColl: func(cfg ddsnmpcollector.Config) ddCollector { return ddsnmpcollector.New(cfg) }, - store: store, - metrics: newCollectorMetrics(store), + store: metricStore, + metrics: newCollectorMetrics(metricStore), } } @@ -62,6 +82,9 @@ type ( deviceCaches map[string]*topologyCache // one cache per SNMP device deviceLastCollected map[string]time.Time // last collection time per device topologyCache *topologyCache // current device cache (set during refreshDeviceTopology) + topologyRegistry *topologyRegistry + deviceSource deviceSource + trapEnrichment *TrapEnrichmentHandle refreshMu sync.Mutex statsMu sync.RWMutex @@ -70,10 +93,12 @@ type ( store metrix.CollectorStore metrics *collectorMetrics - registeredDevices func() []ddsnmp.DeviceConnectionInfo - topologyProfiles func(ddsnmp.DeviceConnectionInfo) []*ddsnmp.Profile - newSnmpClient func() gosnmp.Handler - newDdSnmpColl func(ddsnmpcollector.Config) ddCollector + topologyProfiles func(ddsnmp.DeviceConnectionInfo) []*ddsnmp.Profile + newSnmpClient func() gosnmp.Handler + newDdSnmpColl func(ddsnmpcollector.Config) ddCollector + } + deviceSource interface { + Devices() []ddsnmp.DeviceConnectionInfo } ddCollector interface { Collect() ([]*ddsnmp.ProfileMetrics, error) @@ -103,6 +128,9 @@ func (c *Collector) Run(ctx context.Context) error { if err := ctx.Err(); err != nil { return nil } + c.publishTrapTopologyEnrichment() + defer c.unpublishTrapTopologyEnrichment() + c.refreshTopologyRecovering(ctx) ticker := time.NewTicker(c.deviceCheckEvery()) @@ -138,11 +166,13 @@ func (c *Collector) refreshEvery() time.Duration { } func (c *Collector) Cleanup(context.Context) { + c.unpublishTrapTopologyEnrichment() + c.refreshMu.Lock() defer c.refreshMu.Unlock() for key, cache := range c.deviceCaches { - snmpTopologyRegistry.unregister(cache) + c.topologyRegistry.unregister(cache) delete(c.deviceCaches, key) delete(c.deviceLastCollected, key) } @@ -196,10 +226,10 @@ func (c *Collector) refreshTopology(ctx context.Context) refreshStats { } func (c *Collector) getRegisteredDevices() []ddsnmp.DeviceConnectionInfo { - if c.registeredDevices == nil { + if c.deviceSource == nil { return nil } - return c.registeredDevices() + return c.deviceSource.Devices() } func (c *Collector) refreshTopologyRecovering(ctx context.Context) { @@ -328,7 +358,7 @@ func (c *Collector) getOrCreateDeviceCache(key string) *topologyCache { if !ok { cache = newTopologyCache() c.deviceCaches[key] = cache - snmpTopologyRegistry.register(cache) + c.topologyRegistry.register(cache) } return cache } @@ -345,7 +375,7 @@ func (c *Collector) newDeviceCollectionCache(dev ddsnmp.DeviceConnectionInfo) *t func (c *Collector) pruneStaleDeviceCaches(seen map[string]bool) { for key, cache := range c.deviceCaches { if !seen[key] { - snmpTopologyRegistry.unregister(cache) + c.topologyRegistry.unregister(cache) delete(c.deviceCaches, key) delete(c.deviceLastCollected, key) } @@ -428,5 +458,12 @@ func topologyMethods() []funcapi.MethodConfig { } func topologyFunctionHandler(job collectorapi.RuntimeJob) funcapi.MethodHandler { - return &funcTopology{} + if job == nil { + return nil + } + coll, ok := job.Collector().(*Collector) + if !ok || coll == nil { + return nil + } + return &funcTopology{registry: coll.topologyRegistry} } diff --git a/src/go/plugin/go.d/collector/snmp_topology/collector_refresh_test.go b/src/go/plugin/go.d/collector/snmp_topology/collector_refresh_test.go index 95ac1cc78598fd..a50a51edae33f3 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/collector_refresh_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/collector_refresh_test.go @@ -18,15 +18,35 @@ import ( "github.com/netdata/netdata/go/plugins/plugin/go.d/pkg/snmputils" ) +func TestCollectorGetRegisteredDevicesUsesInjectedDeviceStore(t *testing.T) { + coll, store := newTestSNMPTopologyCollectorWithStore() + registerTestDeviceState(store, ddsnmp.DeviceConnectionInfo{ + Hostname: "192.0.2.10", + Port: 161, + ManualProfiles: []string{"profile-a"}, + VnodeLabels: map[string]string{"site": "lab"}, + }) + + devices := coll.getRegisteredDevices() + require.Len(t, devices, 1) + require.Equal(t, "192.0.2.10", devices[0].Hostname) + + devices[0].ManualProfiles[0] = "changed" + devices[0].VnodeLabels["site"] = "changed" + + again := coll.getRegisteredDevices() + require.Len(t, again, 1) + require.Equal(t, []string{"profile-a"}, again[0].ManualProfiles) + require.Equal(t, "lab", again[0].VnodeLabels["site"]) +} + func TestCollectorValidationLifecycleDoesNotStartPolling(t *testing.T) { - coll := New() + coll, store := newTestSNMPTopologyCollectorWithStore() coll.UpdateEvery = 3600 - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - return []ddsnmp.DeviceConnectionInfo{{ - Hostname: "192.0.2.10", - Port: 161, - }} - } + registerTestDeviceState(store, ddsnmp.DeviceConnectionInfo{ + Hostname: "192.0.2.10", + Port: 161, + }) coll.newSnmpClient = func() gosnmp.Handler { t.Fatal("validation lifecycle must not start topology polling") return nil @@ -39,14 +59,12 @@ func TestCollectorValidationLifecycleDoesNotStartPolling(t *testing.T) { } func TestCollectorRunRefreshesImmediatelyBeforeUpdateEvery(t *testing.T) { - coll := New() + coll, store := newTestSNMPTopologyCollectorWithStore() coll.UpdateEvery = 3600 - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - return []ddsnmp.DeviceConnectionInfo{{ - Hostname: "192.0.2.10", - Port: 161, - }} - } + registerTestDeviceState(store, ddsnmp.DeviceConnectionInfo{ + Hostname: "192.0.2.10", + Port: 161, + }) refreshed := make(chan struct{}, 1) coll.newSnmpClient = func() gosnmp.Handler { @@ -87,7 +105,7 @@ func TestCollectorRunRefreshesImmediatelyBeforeUpdateEvery(t *testing.T) { } func TestCollectorRunStopsOnContextCancel(t *testing.T) { - coll := New() + coll := newTestSNMPTopologyCollector() coll.UpdateEvery = 3600 ctx, cancel := context.WithCancel(context.Background()) @@ -106,13 +124,11 @@ func TestCollectorRunStopsOnContextCancel(t *testing.T) { } func TestCollectorRunDoesNotPollWhenContextAlreadyCanceled(t *testing.T) { - coll := New() - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - return []ddsnmp.DeviceConnectionInfo{{ - Hostname: "192.0.2.10", - Port: 161, - }} - } + coll, store := newTestSNMPTopologyCollectorWithStore() + registerTestDeviceState(store, ddsnmp.DeviceConnectionInfo{ + Hostname: "192.0.2.10", + Port: 161, + }) coll.newSnmpClient = func() gosnmp.Handler { t.Fatal("Run must not poll with an already canceled context") return nil @@ -125,33 +141,25 @@ func TestCollectorRunDoesNotPollWhenContextAlreadyCanceled(t *testing.T) { } func TestCollectorPruneStaleDeviceCachesRemovesLastDeviceCache(t *testing.T) { - previousRegistry := snmpTopologyRegistry - registry := newTopologyRegistry() - snmpTopologyRegistry = registry - t.Cleanup(func() { snmpTopologyRegistry = previousRegistry }) - - coll := New() + coll := newTestSNMPTopologyCollector() cache := newTopologyCache() coll.deviceCaches["gone:161"] = cache coll.deviceLastCollected["gone:161"] = time.Now() - registry.register(cache) + coll.topologyRegistry.register(cache) - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { return nil } coll.refreshTopology(context.Background()) require.Empty(t, coll.deviceCaches) require.Empty(t, coll.deviceLastCollected) - require.False(t, topologyRegistryHasCache(registry, cache)) + require.False(t, topologyRegistryHasCache(coll.topologyRegistry, cache)) } func TestCollectorRefreshTopologyRecoveringHandlesPanic(t *testing.T) { - coll := New() - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - return []ddsnmp.DeviceConnectionInfo{{ - Hostname: "192.0.2.10", - Port: 161, - }} - } + coll, store := newTestSNMPTopologyCollectorWithStore() + registerTestDeviceState(store, ddsnmp.DeviceConnectionInfo{ + Hostname: "192.0.2.10", + Port: 161, + }) coll.newSnmpClient = func() gosnmp.Handler { panic("boom") } @@ -197,11 +205,9 @@ func TestCollectorRunCancelsInFlightRefresh(t *testing.T) { return nil }).AnyTimes() - coll := New() + coll, store := newTestSNMPTopologyCollectorWithStore() coll.UpdateEvery = 3600 - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - return []ddsnmp.DeviceConnectionInfo{dev} - } + registerTestDeviceState(store, dev) coll.newSnmpClient = func() gosnmp.Handler { return mockHandler } coll.topologyProfiles = func(ddsnmp.DeviceConnectionInfo) []*ddsnmp.Profile { return []*ddsnmp.Profile{{}} @@ -263,7 +269,7 @@ func TestCollectorCancelsInFlightVLANContextRefresh(t *testing.T) { return nil }).AnyTimes() - coll := New() + coll := newTestSNMPTopologyCollector() coll.newSnmpClient = func() gosnmp.Handler { return mockHandler } collectStarted := make(chan struct{}) coll.newDdSnmpColl = func(ddsnmpcollector.Config) ddCollector { @@ -297,7 +303,7 @@ func TestCollectorCancelsInFlightVLANContextRefresh(t *testing.T) { } func TestCollectorNewDeviceCollectionCacheUsesEffectiveDeviceCheckEvery(t *testing.T) { - coll := New() + coll := newTestSNMPTopologyCollector() cache := coll.newDeviceCollectionCache(ddsnmp.DeviceConnectionInfo{Hostname: "switch-a"}) @@ -305,11 +311,6 @@ func TestCollectorNewDeviceCollectionCacheUsesEffectiveDeviceCheckEvery(t *testi } func TestCollector_RefreshKeepsPublishedSnapshotWhileCollectionRuns(t *testing.T) { - previousRegistry := snmpTopologyRegistry - registry := newTopologyRegistry() - snmpTopologyRegistry = registry - t.Cleanup(func() { snmpTopologyRegistry = previousRegistry }) - ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -324,14 +325,14 @@ func TestCollector_RefreshKeepsPublishedSnapshotWhileCollectionRuns(t *testing.T key := "10.0.0.10:161" published := newTopologyCache() seedPublishedEndpointSnapshot(published) - registry.register(published) started := make(chan struct{}) release := make(chan struct{}) done := make(chan struct{}) - coll := New() + coll := newTestSNMPTopologyCollector() coll.deviceCaches[key] = published + coll.topologyRegistry.register(published) coll.newSnmpClient = func() gosnmp.Handler { return mockHandler } coll.newDdSnmpColl = func(ddsnmpcollector.Config) ddCollector { return &blockingTopologyCollector{ diff --git a/src/go/plugin/go.d/collector/snmp_topology/config.go b/src/go/plugin/go.d/collector/snmp_topology/config.go index 7109f1beda564b..4982afd28e98e7 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/config.go +++ b/src/go/plugin/go.d/collector/snmp_topology/config.go @@ -5,7 +5,7 @@ package snmptopology import "github.com/netdata/netdata/go/plugins/pkg/confopt" // Config for the snmp_topology module. -// This module has a single global collector instance; devices come from the SNMP device registry. +// This module has a single process-level collector instance; devices come from the injected SNMP device store. type Config struct { UpdateEvery int `yaml:"update_every,omitempty" json:"update_every"` RefreshEvery confopt.LongDuration `yaml:"refresh_every,omitempty" json:"refresh_every,omitempty"` diff --git a/src/go/plugin/go.d/collector/snmp_topology/func_topology.go b/src/go/plugin/go.d/collector/snmp_topology/func_topology.go index 22671414d84c28..43e5a3f8b26b2a 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/func_topology.go +++ b/src/go/plugin/go.d/collector/snmp_topology/func_topology.go @@ -7,7 +7,9 @@ import "github.com/netdata/netdata/go/plugins/pkg/funcapi" // Compile-time interface check. var _ funcapi.MethodHandler = (*funcTopology)(nil) -type funcTopology struct{} +type funcTopology struct { + registry *topologyRegistry +} const ( topologyFunctionName = "snmp:topology:snmp" diff --git a/src/go/plugin/go.d/collector/snmp_topology/func_topology_handler.go b/src/go/plugin/go.d/collector/snmp_topology/func_topology_handler.go index 5134d16e2a32a0..d9ad19bf3fc55f 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/func_topology_handler.go +++ b/src/go/plugin/go.d/collector/snmp_topology/func_topology_handler.go @@ -18,7 +18,7 @@ func (f *funcTopology) MethodParams(_ context.Context, method string) ([]funcapi topologyNodesIdentityParamConfig(), topologyMapTypeParamConfig(), topologyInferenceStrategyParamConfig(), - topologyManagedFocusParamConfig(topologyManagedFocusParamOptions()), + topologyManagedFocusParamConfig(topologyManagedFocusParamOptions(f.registry)), topologyDepthParamConfig(), }, nil } @@ -30,13 +30,13 @@ func (f *funcTopology) Handle(_ context.Context, method string, params funcapi.R return funcapi.NotFoundResponse(method) } - if snmpTopologyRegistry == nil { + if f.registry == nil { return funcapi.UnavailableResponse("topology data not available yet, please retry after topology refresh") } options := resolveTopologyQueryOptions(params) - options.ResolveDNSName = resolveTopologyReverseDNSNameCached // never block on network I/O - data, ok := snmpTopologyRegistry.snapshotWithOptions(options) + options.ResolveDNSName = resolveTopologyReverseDNSNameNoop // never block on network I/O + data, ok := f.registry.snapshotWithOptions(options) if !ok { return funcapi.UnavailableResponse("topology data not available yet, please retry after topology refresh") } @@ -53,13 +53,13 @@ func (f *funcTopology) Handle(_ context.Context, method string, params funcapi.R } } -func topologyManagedFocusParamOptions() []funcapi.ParamOption { - if snmpTopologyRegistry == nil { +func topologyManagedFocusParamOptions(registry *topologyRegistry) []funcapi.ParamOption { + if registry == nil { return nil } options := make([]funcapi.ParamOption, 0) - for _, target := range snmpTopologyRegistry.managedDeviceFocusTargets() { + for _, target := range registry.managedDeviceFocusTargets() { if strings.TrimSpace(target.Value) == "" { continue } diff --git a/src/go/plugin/go.d/collector/snmp_topology/func_topology_presentation_test.go b/src/go/plugin/go.d/collector/snmp_topology/func_topology_presentation_test.go index 0e41515dd8580b..c54bd80e700a0b 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/func_topology_presentation_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/func_topology_presentation_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/netdata/netdata/go/plugins/plugin/framework/collectorapi" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" "github.com/stretchr/testify/require" ) @@ -20,8 +21,7 @@ func TestSNMPTopologyMethodConfigDoesNotUseLegacyPresentation(t *testing.T) { } func TestSNMPTopologyCreatorOwnsTopologyFunction(t *testing.T) { - creator, ok := collectorapi.DefaultRegistry.Lookup("snmp_topology") - require.True(t, ok) + creator := newCreator(ddsnmp.NewDeviceStore(), NewTrapEnrichmentHandle()) require.Nil(t, creator.Create) require.NotNil(t, creator.CreateV2) require.Equal(t, collectorapi.InstancePolicySingle, creator.InstancePolicy) @@ -35,5 +35,38 @@ func TestSNMPTopologyCreatorOwnsTopologyFunction(t *testing.T) { require.Equal(t, topologyMethodID, methods[0].ID) require.Equal(t, topologyFunctionName, methods[0].FunctionName) require.True(t, methods[0].AgentWide) - require.IsType(t, &funcTopology{}, creator.MethodHandler(nil)) + + coll := newTestSNMPTopologyCollector() + handler := creator.MethodHandler(&topologyRuntimeJobForTest{collector: coll}) + require.IsType(t, &funcTopology{}, handler) + require.Same(t, coll.topologyRegistry, handler.(*funcTopology).registry) + require.Nil(t, creator.MethodHandler(nil)) +} + +func TestSNMPTopologyCreatorRequiresSharedDependencies(t *testing.T) { + require.PanicsWithValue(t, "snmp_topology Register requires a non-nil device store", func() { + _ = newCreator(nil, NewTrapEnrichmentHandle()) + }) + require.PanicsWithValue(t, "snmp_topology Register requires a non-nil trap enrichment handle", func() { + _ = newCreator(ddsnmp.NewDeviceStore(), nil) + }) +} + +func TestSNMPTopologyNewRequiresSharedDependencies(t *testing.T) { + require.PanicsWithValue(t, "snmp_topology New requires a non-nil device store", func() { + _ = New(nil, NewTrapEnrichmentHandle()) + }) + require.PanicsWithValue(t, "snmp_topology New requires a non-nil trap enrichment handle", func() { + _ = New(ddsnmp.NewDeviceStore(), nil) + }) +} + +type topologyRuntimeJobForTest struct { + collector *Collector } + +func (j *topologyRuntimeJobForTest) FullName() string { return "snmp_topology" } +func (j *topologyRuntimeJobForTest) ModuleName() string { return "snmp_topology" } +func (j *topologyRuntimeJobForTest) Name() string { return "snmp_topology" } +func (j *topologyRuntimeJobForTest) IsRunning() bool { return true } +func (j *topologyRuntimeJobForTest) Collector() any { return j.collector } diff --git a/src/go/plugin/go.d/collector/snmp_topology/func_topology_test.go b/src/go/plugin/go.d/collector/snmp_topology/func_topology_test.go index e0dc21d2a47270..61957e4af669f3 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/func_topology_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/func_topology_test.go @@ -70,13 +70,7 @@ func TestTopologyMethodConfigIncludesSelectors(t *testing.T) { } func TestFuncTopology_MethodParams(t *testing.T) { - prev := snmpTopologyRegistry - t.Cleanup(func() { - snmpTopologyRegistry = prev - }) - registry := newTopologyRegistry() - snmpTopologyRegistry = registry registry.register(newTestTopologyCacheLLDP( "agent-test", time.Now().UTC(), @@ -90,7 +84,7 @@ func TestFuncTopology_MethodParams(t *testing.T) { "Gi0/2", )) - f := &funcTopology{} + f := &funcTopology{registry: registry} params, err := f.MethodParams(context.Background(), topologyMethodID) require.NoError(t, err) @@ -110,13 +104,7 @@ func TestFuncTopology_MethodParams(t *testing.T) { } func TestFuncTopology_Handle_DefaultStrictL2(t *testing.T) { - prev := snmpTopologyRegistry - t.Cleanup(func() { - snmpTopologyRegistry = prev - }) - registry := newTopologyRegistry() - snmpTopologyRegistry = registry registry.register(newTestTopologyCacheLLDP( "agent-test", time.Now().UTC(), @@ -130,7 +118,7 @@ func TestFuncTopology_Handle_DefaultStrictL2(t *testing.T) { "Gi0/2", )) - f := &funcTopology{} + f := &funcTopology{registry: registry} resp := f.Handle(context.Background(), topologyMethodID, nil) require.NotNil(t, resp) assert.Equal(t, 200, resp.Status) @@ -150,13 +138,7 @@ func TestFuncTopology_Handle_DefaultStrictL2(t *testing.T) { } func TestFuncTopology_Handle_AcceptsSelectorParams(t *testing.T) { - prev := snmpTopologyRegistry - t.Cleanup(func() { - snmpTopologyRegistry = prev - }) - registry := newTopologyRegistry() - snmpTopologyRegistry = registry registry.register(newTestTopologyCacheLLDP( "agent-test", time.Now().UTC(), @@ -170,7 +152,7 @@ func TestFuncTopology_Handle_AcceptsSelectorParams(t *testing.T) { "Gi0/2", )) - f := &funcTopology{} + f := &funcTopology{registry: registry} cfg := []funcapi.ParamConfig{ topologyNodesIdentityParamConfig(), topologyMapTypeParamConfig(), @@ -197,13 +179,7 @@ func TestFuncTopology_Handle_AcceptsSelectorParams(t *testing.T) { } func TestFuncTopology_Handle_UnknownSelectorsFallbackToDefaults(t *testing.T) { - prev := snmpTopologyRegistry - t.Cleanup(func() { - snmpTopologyRegistry = prev - }) - registry := newTopologyRegistry() - snmpTopologyRegistry = registry registry.register(newTestTopologyCacheLLDP( "agent-test", time.Now().UTC(), @@ -217,7 +193,7 @@ func TestFuncTopology_Handle_UnknownSelectorsFallbackToDefaults(t *testing.T) { "Gi0/2", )) - f := &funcTopology{} + f := &funcTopology{registry: registry} cfg := []funcapi.ParamConfig{ topologyNodesIdentityParamConfig(), topologyMapTypeParamConfig(), diff --git a/src/go/plugin/go.d/collector/snmp_topology/metrix_test.go b/src/go/plugin/go.d/collector/snmp_topology/metrix_test.go index 213c497fd166a3..524abbdaf6f2ee 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/metrix_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/metrix_test.go @@ -15,7 +15,7 @@ import ( ) func TestCollector_WriteInternalMetrics(t *testing.T) { - coll := New() + coll := newTestSNMPTopologyCollector() now := time.Unix(100, 0) coll.recordRefreshStats(refreshStats{ hasDeviceCounts: true, @@ -42,11 +42,8 @@ func TestCollector_WriteInternalMetrics(t *testing.T) { } func TestCollectorCollectWritesInternalMetrics(t *testing.T) { - coll := New() - coll.registeredDevices = func() []ddsnmp.DeviceConnectionInfo { - t.Fatal("Collect must not poll SNMP devices") - return nil - } + coll := newTestSNMPTopologyCollector() + coll.deviceSource = fatalDeviceSource{t: t} coll.recordRefreshStats(refreshStats{ hasDeviceCounts: true, registeredDevices: 1, @@ -79,3 +76,13 @@ func requireMetricValue(t *testing.T, reader metrix.Reader, name string, want me require.True(t, ok, "metric %s not found", name) require.Equal(t, want, got) } + +type fatalDeviceSource struct { + t *testing.T +} + +func (s fatalDeviceSource) Devices() []ddsnmp.DeviceConnectionInfo { + s.t.Helper() + s.t.Fatal("Collect must not poll SNMP devices") + return nil +} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_cache_test.go b/src/go/plugin/go.d/collector/snmp_topology/topology_cache_test.go index b6eced3b6374fd..36555ff0cfae77 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_cache_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_cache_test.go @@ -90,14 +90,13 @@ func TestTopologyCache_LldpSnapshot(t *testing.T) { tagLldpRemPortIDSubtype: "5", tagLldpRemPortDesc: "downlink", tagLldpRemSysName: "sw2", + tagLldpRemMgmtAddr: "10.0.0.2", }, }) coll.finalizeTopologyCache() - coll.topologyCache.mu.RLock() - data, ok := coll.topologyCache.snapshot() - coll.topologyCache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(coll.topologyCache) require.True(t, ok) require.Len(t, data.Actors, 2) @@ -105,7 +104,7 @@ func TestTopologyCache_LldpSnapshot(t *testing.T) { link := data.Links[0] assert.Equal(t, "lldp", link.Protocol) - assert.Equal(t, "bidirectional", link.Direction) + assert.Equal(t, "unidirectional", link.Direction) assert.Equal(t, "Gi0/1", link.Src.Attributes["port_id"]) assert.Equal(t, "Gi0/2", link.Dst.Attributes["port_id"]) assert.Equal(t, "sw2", link.Dst.Attributes["sys_name"]) @@ -130,15 +129,13 @@ func TestTopologyCache_CdpSnapshot(t *testing.T) { address: "10.0.0.3", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(cache) require.True(t, ok) require.Len(t, data.Actors, 2) require.Len(t, data.Links, 1) assert.Equal(t, "cdp", data.Links[0].Protocol) - assert.Equal(t, "bidirectional", data.Links[0].Direction) + assert.Equal(t, "unidirectional", data.Links[0].Direction) assert.Equal(t, "Gi0/2", data.Links[0].Src.Attributes["if_name"]) assert.Equal(t, "Gi0/3", data.Links[0].Dst.Attributes["port_id"]) } @@ -291,14 +288,12 @@ func TestTopologyCache_CdpSnapshotHexAddress(t *testing.T) { address: "0a000003", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(cache) require.True(t, ok) require.Len(t, data.Links, 1) assert.Equal(t, "cdp", data.Links[0].Protocol) - assert.Equal(t, "bidirectional", data.Links[0].Direction) + assert.Equal(t, "unidirectional", data.Links[0].Direction) assert.True(t, linkHasRawAddressMetric(data.Links[0], "0a000003")) remote := findDeviceActorBySysName(data, "sw3") @@ -338,14 +333,14 @@ func TestTopologyCache_CdpSnapshotRawAddressWithoutIP(t *testing.T) { address: "edge-sw3.mgmt.local", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + options := defaultTopologyQueryOptionsForTest() + options.EliminateNonIPInferred = false + data, ok := snapshotTopologyCacheForTestWithOptions(cache, options) require.True(t, ok) require.Len(t, data.Links, 1) assert.Equal(t, "cdp", data.Links[0].Protocol) - assert.Equal(t, "bidirectional", data.Links[0].Direction) + assert.Equal(t, "unidirectional", data.Links[0].Direction) assert.True(t, linkHasRawAddressMetric(data.Links[0], "edge-sw3.mgmt.local")) } @@ -378,9 +373,38 @@ func TestTopologyCache_SnapshotBidirectionalPairMetadata(t *testing.T) { managementAddr: "10.0.0.2", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + remoteCache := newTopologyCache() + remoteCache.updateTime = cache.updateTime + remoteCache.lastUpdate = cache.lastUpdate + remoteCache.agentID = "agent2" + remoteCache.localDevice = topologyDevice{ + ChassisID: "aa:bb:cc:dd:ee:ff", + ChassisIDType: "macAddress", + SysName: "sw2", + ManagementIP: "10.0.0.2", + } + remoteCache.lldpLocPorts["2"] = &lldpLocPort{ + portNum: "2", + portID: "Gi0/2", + portIDSubtype: "interfaceName", + portDesc: "downlink", + } + remoteCache.lldpRemotes["2:1"] = &lldpRemote{ + localPortNum: "2", + remIndex: "1", + chassisID: "00:11:22:33:44:55", + chassisIDSubtype: "macAddress", + portID: "Gi0/1", + portIDSubtype: "interfaceName", + portDesc: "uplink", + sysName: "sw1", + managementAddr: "10.0.0.1", + } + + registry := newTopologyRegistry() + registry.register(cache) + registry.register(remoteCache) + data, ok := snapshotTopologyRegistryForTest(registry) require.True(t, ok) require.Len(t, data.Links, 1) @@ -427,9 +451,7 @@ func TestTopologyCache_SnapshotMergesRemoteIdentityAcrossProtocols(t *testing.T) address: "10.0.0.2", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(cache) require.True(t, ok) require.Equal(t, 2, countDeviceActors(data)) @@ -525,9 +547,7 @@ func TestTopologyCache_LLDPManagementAddressesAndCaps(t *testing.T) { coll.finalizeTopologyCache() - coll.topologyCache.mu.RLock() - data, ok := coll.topologyCache.snapshot() - coll.topologyCache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(coll.topologyCache) require.True(t, ok) require.Greater(t, len(data.Actors), 1) @@ -562,9 +582,7 @@ func TestTopologyCache_CDPManagementAddresses(t *testing.T) { tagCdpSecondaryMgmtAddr: "0a000004", }) - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(cache) require.True(t, ok) require.True(t, containsMgmtAddr(data, map[string]struct{}{"10.0.0.3": {}, "10.0.0.4": {}})) @@ -602,9 +620,9 @@ func TestTopologyCache_FDBAndARPEnrichment(t *testing.T) { tagArpState: "reachable", }) - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + options := defaultTopologyQueryOptionsForTest() + options.MapType = topologyMapTypeAllDevicesLowConfidence + data, ok := snapshotTopologyCacheForTestWithOptions(cache, options) require.True(t, ok) require.GreaterOrEqual(t, len(data.Actors), 2) @@ -1034,9 +1052,9 @@ func TestTopologyCache_SnapshotDeterministicEndpointIPSelection(t *testing.T) { expectedIPs := []string{"10.20.4.205", "10.20.4.60"} for range 25 { - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + options := defaultTopologyQueryOptionsForTest() + options.MapType = topologyMapTypeAllDevicesLowConfidence + data, ok := snapshotTopologyCacheForTestWithOptions(cache, options) require.True(t, ok) ep := findActorByMAC(data, "d8:5e:d3:0e:c5:e6") @@ -1079,9 +1097,7 @@ func TestTopologyCache_SnapshotDeterministicOrdering(t *testing.T) { address: "10.0.0.3", } - cache.mu.RLock() - data, ok := cache.snapshot() - cache.mu.RUnlock() + data, ok := snapshotTopologyCacheForTest(cache) require.True(t, ok) require.NotEmpty(t, data.Actors) @@ -1104,69 +1120,6 @@ func TestTopologyCache_SnapshotDeterministicOrdering(t *testing.T) { assert.Equal(t, expectedLinkOrder, linkOrder) } -func TestTopologyCache_BuildEngineObservations_SeparatesProtocolSpecificRemoteObservations(t *testing.T) { - cache := newTopologyCache() - cache.localDevice = topologyDevice{ - ChassisID: "00:11:22:33:44:55", - ChassisIDType: "macAddress", - SysName: "sw-a", - ManagementIP: "10.0.0.1", - } - cache.lldpLocPorts["1"] = &lldpLocPort{ - portNum: "1", - portID: "Gi0/1", - portIDSubtype: "interfaceName", - portDesc: "uplink", - } - cache.lldpRemotes["1:1"] = &lldpRemote{ - localPortNum: "1", - remIndex: "1", - chassisID: "aa:bb:cc:dd:ee:ff", - chassisIDSubtype: "macAddress", - portID: "Gi0/2", - portIDSubtype: "interfaceName", - portDesc: "downlink", - sysName: "sw-b", - managementAddr: "10.0.0.2", - } - cache.cdpRemotes["1:1"] = &cdpRemote{ - ifIndex: "1", - ifName: "Gi0/1", - deviceID: "sw-b", - sysName: "switch-b", - devicePort: "Gi0/2", - address: "10.0.0.2", - } - - observations, localDeviceID := cache.buildEngineObservations(cache.localDevice) - require.Equal(t, "macAddress:00:11:22:33:44:55", localDeviceID) - require.Len(t, observations, 3) - require.Equal(t, localDeviceID, observations[0].DeviceID) - - var lldpObservation *topologyengine.L2Observation - var cdpObservation *topologyengine.L2Observation - for i := 1; i < len(observations); i++ { - observation := &observations[i] - switch { - case len(observation.LLDPRemotes) > 0: - lldpObservation = observation - case len(observation.CDPRemotes) > 0: - cdpObservation = observation - } - } - - require.NotNil(t, lldpObservation) - require.NotNil(t, cdpObservation) - require.Equal(t, lldpObservation.DeviceID, cdpObservation.DeviceID) - require.Equal(t, "macAddress:aa:bb:cc:dd:ee:ff", lldpObservation.DeviceID) - require.Equal(t, "10.0.0.2", lldpObservation.ManagementIP) - require.Equal(t, "10.0.0.2", cdpObservation.ManagementIP) - require.Equal(t, "sw-b", lldpObservation.Hostname) - require.Equal(t, "switch-b", cdpObservation.Hostname) - require.Len(t, lldpObservation.LLDPRemotes, 1) - require.Len(t, cdpObservation.CDPRemotes, 1) -} - func TestTopologyObservationIdentityResolver_ReusesStableRemoteIdentityAcrossSignals(t *testing.T) { resolver := newTopologyObservationIdentityResolver(topologyengine.L2Observation{ DeviceID: "macAddress:00:11:22:33:44:55", @@ -1626,6 +1579,9 @@ func linkHasRawAddressMetric(link topologyLink, raw string) bool { if raw == "" || len(link.Metrics) == 0 { return false } + if value, ok := link.Metrics["remote_address_raw"].(string); ok && value == raw { + return true + } srcRaw, srcOK := link.Metrics["src_remote_address_raw"].(string) dstRaw, dstOK := link.Metrics["dst_remote_address_raw"].(string) return (srcOK && srcRaw == raw) || (dstOK && dstRaw == raw) diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_dns.go b/src/go/plugin/go.d/collector/snmp_topology/topology_dns.go index 059da2e0e82f43..936d20ebd7e739 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_dns.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_dns.go @@ -2,140 +2,8 @@ package snmptopology -import ( - "context" - "net" - "net/netip" - "sort" - "strings" - "sync" - "time" -) - -const ( - topologyReverseDNSTimeout = 50 * time.Millisecond - topologyReverseDNSCacheTTL = 10 * time.Minute - topologyReverseDNSNegTTL = 30 * time.Second -) - -type topologyReverseDNSCacheEntry struct { - name string - expiresAt time.Time -} - -type topologyReverseDNSResolver struct { - mu sync.RWMutex - timeout time.Duration - ttl time.Duration - cache map[string]topologyReverseDNSCacheEntry -} - -func newTopologyReverseDNSResolver(timeout, ttl time.Duration) *topologyReverseDNSResolver { - return &topologyReverseDNSResolver{ - timeout: timeout, - ttl: ttl, - cache: make(map[string]topologyReverseDNSCacheEntry), - } -} - -// lookupCached returns the cached result for ip without performing any network I/O. -// Returns "" when the IP has never been resolved or its cache entry has expired. -func (r *topologyReverseDNSResolver) lookupCached(ip string) string { - if r == nil { - return "" - } - addr, err := netip.ParseAddr(strings.TrimSpace(ip)) - if err != nil || !addr.IsValid() { - return "" - } - ip = addr.Unmap().String() - - r.mu.RLock() - entry, ok := r.cache[ip] - r.mu.RUnlock() - if ok && time.Now().Before(entry.expiresAt) { - return entry.name - } +// resolveTopologyReverseDNSNameNoop is the non-blocking DNS resolver hook used +// during function responses. SNMP topology has no reverse-DNS warmer today. +func resolveTopologyReverseDNSNameNoop(_ string) string { return "" } - -func (r *topologyReverseDNSResolver) lookup(ip string) string { - if r == nil { - return "" - } - addr, err := netip.ParseAddr(strings.TrimSpace(ip)) - if err != nil || !addr.IsValid() { - return "" - } - ip = addr.Unmap().String() - now := time.Now() - - r.mu.RLock() - entry, ok := r.cache[ip] - r.mu.RUnlock() - if ok && now.Before(entry.expiresAt) { - return entry.name - } - - ctx, cancel := context.WithTimeout(context.Background(), r.timeout) - defer cancel() - names, err := net.DefaultResolver.LookupAddr(ctx, ip) - resolved := "" - if err == nil { - resolved = topologyNormalizeReverseDNSName(names) - } - ttl := r.ttl - if resolved == "" && topologyReverseDNSNegTTL > 0 { - ttl = topologyReverseDNSNegTTL - } - - r.mu.Lock() - r.cache[ip] = topologyReverseDNSCacheEntry{ - name: resolved, - expiresAt: now.Add(ttl), - } - r.mu.Unlock() - - return resolved -} - -func topologyNormalizeReverseDNSName(names []string) string { - if len(names) == 0 { - return "" - } - seen := make(map[string]struct{}, len(names)) - out := make([]string, 0, len(names)) - for _, name := range names { - name = strings.TrimSpace(name) - name = strings.TrimSuffix(name, ".") - name = strings.ToLower(name) - if name == "" { - continue - } - if _, ok := seen[name]; ok { - continue - } - seen[name] = struct{}{} - out = append(out, name) - } - if len(out) == 0 { - return "" - } - sort.Strings(out) - return out[0] -} - -var defaultTopologyReverseDNSResolver = newTopologyReverseDNSResolver(topologyReverseDNSTimeout, topologyReverseDNSCacheTTL) - -// resolveTopologyReverseDNSName performs a live DNS lookup (with cache). -// Used while building topology snapshots to warm the cache. -func resolveTopologyReverseDNSName(ip string) string { - return defaultTopologyReverseDNSResolver.lookup(ip) -} - -// resolveTopologyReverseDNSNameCached returns a cached DNS name if available, -// or an empty string if the IP has not been resolved yet. Never blocks on network I/O. -// Used during function responses to avoid external calls. -func resolveTopologyReverseDNSNameCached(ip string) string { - return defaultTopologyReverseDNSResolver.lookupCached(ip) -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_integration_test.go b/src/go/plugin/go.d/collector/snmp_topology/topology_integration_test.go index 77ed7445487639..fd3264df2bdac0 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_integration_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_integration_test.go @@ -92,11 +92,11 @@ func TestTopologyIntegrationWithSnmpsimV3(t *testing.T) { func collectTopologySnapshotFromDevice(t *testing.T, dev ddsnmp.DeviceConnectionInfo) topologyData { t.Helper() + deviceStore := ddsnmp.NewDeviceStore() deviceKey := "integration:" + dev.SNMPVersion + ":" + dev.SysName - ddsnmp.DeviceRegistry.Register(deviceKey, dev) - defer ddsnmp.DeviceRegistry.Unregister(deviceKey) + deviceStore.Register(deviceKey, dev) - coll := New() + coll := New(deviceStore, NewTrapEnrichmentHandle()) coll.Config = Config{UpdateEvery: 3600} require.NoError(t, coll.Init(context.Background())) defer coll.Cleanup(context.Background()) @@ -111,11 +111,12 @@ func collectTopologySnapshotFromDevice(t *testing.T, dev ddsnmp.DeviceConnection if cache == nil { return false } - cache.mu.RLock() - defer cache.mu.RUnlock() var ok bool - snapshot, ok = cache.snapshot() + options := defaultTopologyQueryOptionsForTest() + options.CollapseActorsByIP = false + options.EliminateNonIPInferred = false + snapshot, ok = snapshotTopologyCacheForTestWithOptions(cache, options) return ok }, 5*time.Second, 100*time.Millisecond, "topology snapshot did not become available for %q", dev.SysName) diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote.go b/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote.go deleted file mode 100644 index e730efec5bf682..00000000000000 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote.go +++ /dev/null @@ -1,109 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later - -package snmptopology - -import ( - "sort" - "strings" - - topologyengine "github.com/netdata/netdata/go/plugins/pkg/l2topology" -) - -type topologyRemoteObservationBuilder struct { - cache *topologyCache - local topologyDevice - localObservation topologyengine.L2Observation - localManagementIP string - localSysName string - localGlobalID string - - resolver *topologyObservationIdentityResolver - remoteObservations map[string]*topologyengine.L2Observation - remoteOrder []string - remoteManagementByID map[string]string - remoteChassisByID map[string]string -} - -func newTopologyRemoteObservationBuilder(cache *topologyCache, local topologyDevice, localObservation topologyengine.L2Observation) *topologyRemoteObservationBuilder { - localManagementIP := normalizeIPAddress(local.ManagementIP) - if localManagementIP == "" { - localManagementIP = pickManagementIP(local.ManagementAddresses) - } - - localGlobalID := strings.TrimSpace(localObservation.Hostname) - if localGlobalID == "" { - localGlobalID = localObservation.DeviceID - } - - return &topologyRemoteObservationBuilder{ - cache: cache, - local: local, - localObservation: localObservation, - localManagementIP: localManagementIP, - localSysName: strings.TrimSpace(local.SysName), - localGlobalID: localGlobalID, - resolver: newTopologyObservationIdentityResolver(localObservation), - remoteObservations: make(map[string]*topologyengine.L2Observation), - remoteOrder: make([]string, 0, len(cache.lldpRemotes)+len(cache.cdpRemotes)), - remoteManagementByID: make(map[string]string), - remoteChassisByID: make(map[string]string), - } -} - -func (c *topologyCache) buildEngineObservations(local topologyDevice) ([]topologyengine.L2Observation, string) { - localObservation := c.buildEngineObservation(local) - localObservation.DeviceID = strings.TrimSpace(localObservation.DeviceID) - if localObservation.DeviceID == "" { - return nil, "" - } - - builder := newTopologyRemoteObservationBuilder(c, local, localObservation) - builder.collectLLDPRemoteObservations() - builder.collectCDPRemoteObservations() - - return builder.observations(), localObservation.DeviceID -} - -func (b *topologyRemoteObservationBuilder) observations() []topologyengine.L2Observation { - observations := make([]topologyengine.L2Observation, 0, 1+len(b.remoteObservations)) - observations = append(observations, b.localObservation) - - sort.Strings(b.remoteOrder) - for _, key := range b.remoteOrder { - entry := b.remoteObservations[key] - if entry == nil { - continue - } - if entry.ManagementIP == "" { - entry.ManagementIP = b.remoteManagementByID[entry.DeviceID] - } - if entry.ChassisID == "" { - entry.ChassisID = b.remoteChassisByID[entry.DeviceID] - } - if len(entry.LLDPRemotes) == 0 && len(entry.CDPRemotes) == 0 { - continue - } - if entry.Hostname == "" { - entry.Hostname = entry.DeviceID - } - observations = append(observations, *entry) - } - - return observations -} - -func selectTopologyRemoteHostname(current, candidate, deviceID string) string { - current = strings.TrimSpace(current) - candidate = strings.TrimSpace(candidate) - deviceID = strings.TrimSpace(deviceID) - if candidate == "" { - if current != "" { - return current - } - return deviceID - } - if current == "" || current == deviceID { - return candidate - } - return current -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_cdp.go b/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_cdp.go deleted file mode 100644 index 114d63bb110115..00000000000000 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_cdp.go +++ /dev/null @@ -1,74 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later - -package snmptopology - -import ( - "sort" - "strings" - - topologyengine "github.com/netdata/netdata/go/plugins/pkg/l2topology" -) - -func (b *topologyRemoteObservationBuilder) collectCDPRemoteObservations() { - keys := make([]string, 0, len(b.cache.cdpRemotes)) - for key := range b.cache.cdpRemotes { - keys = append(keys, key) - } - sort.Strings(keys) - - for _, key := range keys { - remote := b.cache.cdpRemotes[key] - if remote == nil { - continue - } - - remoteDeviceToken := strings.TrimSpace(remote.deviceID) - remoteSysName := strings.TrimSpace(remote.sysName) - remoteManagementIP := normalizeIPAddress(remote.address) - if remoteManagementIP == "" { - remoteManagementIP = pickManagementIP(remote.managementAddrs) - } - if remoteManagementIP == "" && remoteDeviceToken == "" && remoteSysName == "" { - continue - } - - remoteDeviceID := b.resolver.resolve( - []string{remoteDeviceToken, remoteSysName}, - "", - "", - remoteManagementIP, - ) - if remoteDeviceID == "" || remoteDeviceID == b.localObservation.DeviceID { - continue - } - b.updateRemoteIdentity(remoteDeviceID, remoteManagementIP, "") - - remoteIfName := strings.TrimSpace(remote.devicePort) - localIfName := strings.TrimSpace(remote.ifName) - if localIfName == "" && strings.TrimSpace(remote.ifIndex) != "" { - localIfName = strings.TrimSpace(b.cache.ifNamesByIndex[remote.ifIndex]) - } - if remoteIfName == "" || localIfName == "" { - continue - } - - remoteObservation := b.ensureRemoteObservation( - "cdp", - remoteDeviceID, - firstNonEmpty(remoteSysName, remoteDeviceToken, remoteDeviceID), - remoteManagementIP, - "", - ) - if remoteObservation == nil { - continue - } - - remoteObservation.CDPRemotes = append(remoteObservation.CDPRemotes, topologyengine.CDPRemoteObservation{ - LocalIfName: remoteIfName, - DeviceID: b.localGlobalID, - SysName: b.localSysName, - DevicePort: localIfName, - Address: b.localManagementIP, - }) - } -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_identity.go b/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_identity.go deleted file mode 100644 index bfa439191a1938..00000000000000 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_identity.go +++ /dev/null @@ -1,55 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later - -package snmptopology - -import ( - "strings" - - topologyengine "github.com/netdata/netdata/go/plugins/pkg/l2topology" -) - -func (b *topologyRemoteObservationBuilder) updateRemoteIdentity(deviceID, managementIP, chassisID string) { - deviceID = strings.TrimSpace(deviceID) - if deviceID == "" { - return - } - if managementIP = canonicalObservationIP(managementIP); managementIP != "" { - if _, ok := b.remoteManagementByID[deviceID]; !ok { - b.remoteManagementByID[deviceID] = managementIP - } - } - if chassisID = strings.TrimSpace(chassisID); chassisID != "" { - if _, ok := b.remoteChassisByID[deviceID]; !ok { - b.remoteChassisByID[deviceID] = chassisID - } - } -} - -func (b *topologyRemoteObservationBuilder) ensureRemoteObservation(protocol, deviceID, hostname, managementIP, chassisID string) *topologyengine.L2Observation { - deviceID = strings.TrimSpace(deviceID) - if deviceID == "" { - return nil - } - - key := protocol + "|" + deviceID - entry := b.remoteObservations[key] - if entry == nil { - entry = &topologyengine.L2Observation{ - DeviceID: deviceID, - Inferred: true, - } - b.remoteObservations[key] = entry - b.remoteOrder = append(b.remoteOrder, key) - } - - entry.Hostname = selectTopologyRemoteHostname(entry.Hostname, hostname, deviceID) - b.updateRemoteIdentity(deviceID, managementIP, chassisID) - if entry.ManagementIP == "" { - entry.ManagementIP = b.remoteManagementByID[deviceID] - } - if entry.ChassisID == "" { - entry.ChassisID = b.remoteChassisByID[deviceID] - } - b.resolver.register(deviceID, []string{entry.Hostname}, entry.ChassisID, entry.ManagementIP) - return entry -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_lldp.go b/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_lldp.go deleted file mode 100644 index bea84a06e5a196..00000000000000 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_observation_remote_lldp.go +++ /dev/null @@ -1,85 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later - -package snmptopology - -import ( - "sort" - "strings" - - topologyengine "github.com/netdata/netdata/go/plugins/pkg/l2topology" -) - -func (b *topologyRemoteObservationBuilder) collectLLDPRemoteObservations() { - keys := make([]string, 0, len(b.cache.lldpRemotes)) - for key := range b.cache.lldpRemotes { - keys = append(keys, key) - } - sort.Strings(keys) - - for _, key := range keys { - remote := b.cache.lldpRemotes[key] - if remote == nil { - continue - } - - remoteSysName := strings.TrimSpace(remote.sysName) - remoteChassisID := strings.TrimSpace(remote.chassisID) - remoteManagementIP := normalizeIPAddress(remote.managementAddr) - if remoteManagementIP == "" { - remoteManagementIP = pickManagementIP(remote.managementAddrs) - } - - remoteDeviceID := b.resolver.resolve( - []string{remoteSysName}, - remoteChassisID, - strings.TrimSpace(remote.chassisIDSubtype), - remoteManagementIP, - ) - if remoteDeviceID == "" || remoteDeviceID == b.localObservation.DeviceID { - continue - } - b.updateRemoteIdentity(remoteDeviceID, remoteManagementIP, remoteChassisID) - - remoteObservation := b.ensureRemoteObservation( - "lldp", - remoteDeviceID, - firstNonEmpty(remoteSysName, remoteDeviceID), - remoteManagementIP, - remoteChassisID, - ) - if remoteObservation == nil { - continue - } - - localPort := b.cache.lldpLocPorts[remote.localPortNum] - localPortID := "" - localPortIDSubtype := "" - localPortDesc := "" - if localPort != nil { - localPortID = strings.TrimSpace(localPort.portID) - localPortIDSubtype = strings.TrimSpace(localPort.portIDSubtype) - localPortDesc = strings.TrimSpace(localPort.portDesc) - } - - if strings.TrimSpace(remote.portID) == "" && - strings.TrimSpace(remote.portDesc) == "" && - localPortID == "" && - localPortDesc == "" { - continue - } - - remoteObservation.LLDPRemotes = append(remoteObservation.LLDPRemotes, topologyengine.LLDPRemoteObservation{ - LocalPortNum: strings.TrimSpace(remote.remIndex), - RemoteIndex: strings.TrimSpace(remote.localPortNum), - LocalPortID: strings.TrimSpace(remote.portID), - LocalPortIDSubtype: strings.TrimSpace(remote.portIDSubtype), - LocalPortDesc: strings.TrimSpace(remote.portDesc), - ChassisID: strings.TrimSpace(b.local.ChassisID), - SysName: b.localSysName, - PortID: localPortID, - PortIDSubtype: localPortIDSubtype, - PortDesc: localPortDesc, - ManagementIP: b.localManagementIP, - }) - } -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_registry.go b/src/go/plugin/go.d/collector/snmp_topology/topology_registry.go index 6a1161320addf6..3599283d23e640 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_registry.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_registry.go @@ -32,8 +32,6 @@ func newTopologyRegistry() *topologyRegistry { } } -var snmpTopologyRegistry = newTopologyRegistry() - func (r *topologyRegistry) register(cache *topologyCache) { if r == nil || cache == nil { return diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_snapshot_builder.go b/src/go/plugin/go.d/collector/snmp_topology/topology_snapshot_builder.go index eae52866e52168..018936f5e234ff 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_snapshot_builder.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_snapshot_builder.go @@ -2,12 +2,7 @@ package snmptopology -import ( - "time" - - topologyengine "github.com/netdata/netdata/go/plugins/pkg/l2topology" - "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" -) +import "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" func buildLocalTopologyDevice(dev ddsnmp.DeviceConnectionInfo) topologyDevice { device := topologyDevice{ @@ -72,41 +67,3 @@ func buildLocalTopologyDevice(dev ddsnmp.DeviceConnectionInfo) topologyDevice { return device } - -func (c *topologyCache) snapshot() (topologyData, bool) { - if !c.hasFreshSnapshotAt(time.Now()) { - return topologyData{}, false - } - - local := c.localDevice - local = normalizeTopologyDevice(local) - - observations, localDeviceID := c.buildEngineObservations(local) - if len(observations) == 0 { - return topologyData{}, false - } - - result, err := topologyengine.BuildL2ResultFromObservations(observations, topologyengine.DiscoverOptions{ - EnableLLDP: true, - EnableCDP: true, - EnableBridge: true, - EnableARP: true, - }) - if err != nil { - return topologyData{}, false - } - - data := topologyengine.ToGraph(result, topologyengine.GraphOptions{ - SchemaVersion: topologySchemaVersion, - Source: "snmp", - Layer: "2", - View: "summary", - AgentID: c.agentID, - LocalDeviceID: localDeviceID, - CollectedAt: c.lastUpdate, - ResolveDNSName: resolveTopologyReverseDNSName, - }) - - augmentLocalActorFromCache(&data, local) - return data, true -} diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_snmprec_test.go b/src/go/plugin/go.d/collector/snmp_topology/topology_snmprec_test.go index e2d116f4a203cb..89130d4717bbe9 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_snmprec_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_snmprec_test.go @@ -67,9 +67,10 @@ func TestTopologyCache_RealSnmprecFixtures(t *testing.T) { } coll.finalizeTopologyCache() - coll.topologyCache.mu.RLock() - snapshot, ok := coll.topologyCache.snapshot() - coll.topologyCache.mu.RUnlock() + options := defaultTopologyQueryOptionsForTest() + options.CollapseActorsByIP = false + options.EliminateNonIPInferred = false + snapshot, ok := snapshotTopologyCacheForTestWithOptions(coll.topologyCache, options) require.True(t, ok) require.GreaterOrEqual(t, len(snapshot.Actors), 1) diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_test_helpers_test.go b/src/go/plugin/go.d/collector/snmp_topology/topology_test_helpers_test.go index f1b09429a5be68..38b244de67f668 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_test_helpers_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_test_helpers_test.go @@ -2,16 +2,58 @@ package snmptopology +import ( + "fmt" + + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" +) + +func newTestSNMPTopologyCollector() *Collector { + coll, _ := newTestSNMPTopologyCollectorWithStore() + return coll +} + +func newTestSNMPTopologyCollectorWithStore() (*Collector, *ddsnmp.DeviceStore) { + store := ddsnmp.NewDeviceStore() + return New(store, NewTrapEnrichmentHandle()), store +} + +func registerTestDeviceState(store *ddsnmp.DeviceStore, devices ...ddsnmp.DeviceConnectionInfo) { + for i, dev := range devices { + store.Register(fmt.Sprintf("test:%s:%d:%d", dev.Hostname, dev.Port, i), dev) + } +} + func snapshotTopologyRegistryForTest(registry *topologyRegistry) (topologyData, bool) { - return registry.snapshotWithOptions(topologyQueryOptions{ + return snapshotTopologyRegistryForTestWithOptions(registry, defaultTopologyQueryOptionsForTest()) +} + +func snapshotTopologyRegistryForTestWithOptions(registry *topologyRegistry, options topologyQueryOptions) (topologyData, bool) { + if options.ResolveDNSName == nil { + options.ResolveDNSName = resolveTopologyReverseDNSNameNoop + } + return registry.snapshotWithOptions(options) +} + +func snapshotTopologyCacheForTest(cache *topologyCache) (topologyData, bool) { + return snapshotTopologyCacheForTestWithOptions(cache, defaultTopologyQueryOptionsForTest()) +} + +func snapshotTopologyCacheForTestWithOptions(cache *topologyCache, options topologyQueryOptions) (topologyData, bool) { + registry := newTopologyRegistry() + registry.register(cache) + return snapshotTopologyRegistryForTestWithOptions(registry, options) +} + +func defaultTopologyQueryOptionsForTest() topologyQueryOptions { + return topologyQueryOptions{ CollapseActorsByIP: true, EliminateNonIPInferred: true, MapType: topologyMapTypeLLDPCDPManaged, InferenceStrategy: topologyInferenceStrategyFDBMinimumKnowledge, ManagedDeviceFocus: topologyManagedFocusAllDevices, Depth: topologyDepthAllInternal, - ResolveDNSName: resolveTopologyReverseDNSName, - }) + } } func containsMgmtAddr(snapshot topologyData, addrs map[string]struct{}) bool { diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich.go b/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich.go index 50871141454c8c..591b97990c49b4 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich.go @@ -5,6 +5,7 @@ package snmptopology import ( "sort" "strings" + "sync/atomic" ) type TrapTopologyEnrichment struct { @@ -22,27 +23,56 @@ type TrapTopologyEnrichment struct { Neighbors []string } -// TrapEnrichmentForIP returns source-device topology enrichment for a trap -// received from the given source IP. It intentionally does not infer a trap -// interface from the source IP. -func TrapEnrichmentForIP(ip string) *TrapTopologyEnrichment { - return TrapEnrichmentForSource(ip, "") +// TrapEnrichmentHandle exposes the currently running topology registry to trap enrichment consumers. +type TrapEnrichmentHandle struct { + registry atomic.Pointer[topologyRegistry] } -// TrapEnrichmentForSource returns topology enrichment data for a trap received +// NewTrapEnrichmentHandle returns an empty process-local trap enrichment handle. +func NewTrapEnrichmentHandle() *TrapEnrichmentHandle { + return &TrapEnrichmentHandle{} +} + +func (c *Collector) publishTrapTopologyEnrichment() { + if c.trapEnrichment != nil && c.topologyRegistry != nil { + c.trapEnrichment.registry.Store(c.topologyRegistry) + } +} + +func (c *Collector) unpublishTrapTopologyEnrichment() { + if c.trapEnrichment != nil && c.topologyRegistry != nil { + c.trapEnrichment.registry.CompareAndSwap(c.topologyRegistry, nil) + } +} + +// EnrichmentForSource returns topology enrichment data for a trap received // from the given source IP and, when available, the trap subject ifIndex. // Interface and neighbor enrichment only use the trap ifIndex after the source // IP matches exactly one local topology cache. -// -// It copies active cache pointers under the registry lock, reads each cache -// under its own lock, and never blocks on I/O. -func TrapEnrichmentForSource(ip, trapIfIndex string) *TrapTopologyEnrichment { +func (h *TrapEnrichmentHandle) EnrichmentForSource(ip, trapIfIndex string) *TrapTopologyEnrichment { + if h == nil { + return nil + } + registry := h.registry.Load() + if registry == nil { + return nil + } + return registry.trapEnrichmentForSource(ip, trapIfIndex) +} + +// trapEnrichmentForSource copies active cache pointers under the registry lock, +// reads each cache under its own lock, and never blocks on I/O. +func (r *topologyRegistry) trapEnrichmentForSource(ip, trapIfIndex string) *TrapTopologyEnrichment { + if r == nil { + return nil + } + ip = normalizeIPAddress(ip) if ip == "" { return nil } - caches := snmpTopologyRegistry.activeCaches() + caches := r.activeCaches() if len(caches) == 0 { return nil } diff --git a/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich_test.go b/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich_test.go index 3ffc3b80971808..c8b937da4d6daa 100644 --- a/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich_test.go +++ b/src/go/plugin/go.d/collector/snmp_topology/topology_trap_enrich_test.go @@ -3,12 +3,15 @@ package snmptopology import ( + "context" + "errors" "testing" + "time" "github.com/stretchr/testify/require" ) -func TestTopologyCacheTrapEnrichmentForSourceUsesTrapIfIndex(t *testing.T) { +func TestTopologyCacheTrapEnrichmentUsesTrapIfIndex(t *testing.T) { cache := newTopologyCache() cache.localDevice.ManagementIP = "192.0.2.30" cache.ifIndexByIP["192.0.2.10"] = "99" @@ -29,7 +32,7 @@ func TestTopologyCacheTrapEnrichmentForSourceUsesTrapIfIndex(t *testing.T) { require.Equal(t, []string{"dist-a", "dist-b"}, enrich.Neighbors) } -func TestTopologyCacheTrapEnrichmentForSourceFallsBackToRemoteMapKeys(t *testing.T) { +func TestTopologyCacheTrapEnrichmentFallsBackToRemoteMapKeys(t *testing.T) { cache := newTopologyCache() cache.localDevice.ManagementIP = "192.0.2.30" cache.lldpRemotes["7:2"] = &lldpRemote{sysName: "dist-b"} @@ -42,7 +45,7 @@ func TestTopologyCacheTrapEnrichmentForSourceFallsBackToRemoteMapKeys(t *testing require.Equal(t, []string{"dist-a", "dist-b"}, enrich.Neighbors) } -func TestTopologyCacheTrapEnrichmentForSourceDoesNotInferInterfaceFromSourceIP(t *testing.T) { +func TestTopologyCacheTrapEnrichmentDoesNotInferInterfaceFromSourceIP(t *testing.T) { cache := newTopologyCache() cache.ifIndexByIP["192.0.2.10"] = "7" cache.ifNamesByIndex["7"] = "Gi0/7" @@ -58,7 +61,7 @@ func TestTopologyCacheTrapEnrichmentForSourceDoesNotInferInterfaceFromSourceIP(t require.Equal(t, "skipped", enrich.NeighborStatus) } -func TestTopologyCacheTrapEnrichmentForSourceNoInterfaceMatch(t *testing.T) { +func TestTopologyCacheTrapEnrichmentNoInterfaceMatch(t *testing.T) { cache := newTopologyCache() cache.localDevice.ManagementIP = "192.0.2.30" cache.lldpRemotes["7:1"] = &lldpRemote{sysName: "dist-a"} @@ -71,7 +74,7 @@ func TestTopologyCacheTrapEnrichmentForSourceNoInterfaceMatch(t *testing.T) { require.Empty(t, enrich.Neighbors) } -func TestTopologyCacheTrapEnrichmentForSourceIncludesLocalDeviceIdentity(t *testing.T) { +func TestTopologyCacheTrapEnrichmentIncludesLocalDeviceIdentity(t *testing.T) { cache := newTopologyCache() cache.localDevice.ManagementIP = "192.0.2.30" cache.localDevice.SysName = "core-sw-01" @@ -86,27 +89,32 @@ func TestTopologyCacheTrapEnrichmentForSourceIncludesLocalDeviceIdentity(t *test require.Equal(t, "vnode-node-id", enrich.SourceVnodeID) } -func TestTrapEnrichmentForSourceUsesGlobalRegistry(t *testing.T) { +func TestTrapEnrichmentHandleForSourceUsesPublishedRegistry(t *testing.T) { + registry := newTopologyRegistry() + handle := publishTrapTopologyRegistryForTest(registry) + cache := newTopologyCache() cache.localDevice.ManagementIP = "192.0.2.20" cache.ifNamesByIndex["11"] = "Gi0/11" cache.lldpRemotes["11:1"] = &lldpRemote{sysName: "dist-c"} - snmpTopologyRegistry.register(cache) - defer snmpTopologyRegistry.unregister(cache) + registry.register(cache) - enrich := TrapEnrichmentForSource("192.0.2.20", "11") + enrich := handle.EnrichmentForSource("192.0.2.20", "11") require.NotNil(t, enrich) require.Equal(t, "matched", enrich.DeviceStatus) require.Equal(t, "Gi0/11", enrich.Interface) require.Equal(t, []string{"dist-c"}, enrich.Neighbors) - mapped := TrapEnrichmentForSource("::ffff:192.0.2.20", "11") + mapped := handle.EnrichmentForSource("::ffff:192.0.2.20", "11") require.NotNil(t, mapped) require.Equal(t, "Gi0/11", mapped.Interface) } -func TestTrapEnrichmentForSourceAmbiguousGlobalRegistryMatchDoesNotEnrich(t *testing.T) { +func TestTrapEnrichmentHandleForSourceAmbiguousRegistryMatchDoesNotEnrich(t *testing.T) { + registry := newTopologyRegistry() + handle := publishTrapTopologyRegistryForTest(registry) + cacheA := newTopologyCache() cacheA.localDevice.ManagementIP = "192.0.2.20" cacheA.ifNamesByIndex["11"] = "Gi0/11" @@ -114,15 +122,68 @@ func TestTrapEnrichmentForSourceAmbiguousGlobalRegistryMatchDoesNotEnrich(t *tes cacheB.localDevice.ManagementIP = "192.0.2.20" cacheB.ifNamesByIndex["11"] = "Gi0/11" - snmpTopologyRegistry.register(cacheA) - snmpTopologyRegistry.register(cacheB) - defer snmpTopologyRegistry.unregister(cacheA) - defer snmpTopologyRegistry.unregister(cacheB) + registry.register(cacheA) + registry.register(cacheB) - enrich := TrapEnrichmentForSource("192.0.2.20", "11") + enrich := handle.EnrichmentForSource("192.0.2.20", "11") require.NotNil(t, enrich) require.Equal(t, "ambiguous", enrich.DeviceStatus) require.Equal(t, 2, enrich.DeviceMatches) require.Empty(t, enrich.Interface) require.Empty(t, enrich.Neighbors) } + +func TestCollectorRunPublishesAndClearsTrapTopologyRegistry(t *testing.T) { + coll := newTestSNMPTopologyCollector() + coll.UpdateEvery = 3600 + + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { + errCh <- coll.Run(ctx) + }() + + stopped := false + stopRunner := func() error { + if stopped { + return nil + } + stopped = true + cancel() + select { + case err := <-errCh: + return err + case <-time.After(time.Second): + return errors.New("runner did not stop") + } + } + defer func() { + require.NoError(t, stopRunner()) + }() + + require.Eventually(t, func() bool { + return coll.trapEnrichment.registry.Load() == coll.topologyRegistry + }, time.Second, 10*time.Millisecond) + + require.NoError(t, stopRunner()) + require.Nil(t, coll.trapEnrichment.registry.Load()) +} + +func TestCollectorCleanupDoesNotClearNewerTrapTopologyRegistry(t *testing.T) { + trapEnrichment := NewTrapEnrichmentHandle() + oldColl := newTestSNMPTopologyCollector() + newColl := newTestSNMPTopologyCollector() + oldColl.trapEnrichment = trapEnrichment + newColl.trapEnrichment = trapEnrichment + trapEnrichment.registry.Store(newColl.topologyRegistry) + + oldColl.Cleanup(context.Background()) + + require.Same(t, newColl.topologyRegistry, trapEnrichment.registry.Load()) +} + +func publishTrapTopologyRegistryForTest(registry *topologyRegistry) *TrapEnrichmentHandle { + handle := NewTrapEnrichmentHandle() + handle.registry.Store(registry) + return handle +} diff --git a/src/go/plugin/go.d/collector/snmp_traps/collector.go b/src/go/plugin/go.d/collector/snmp_traps/collector.go index 899b1e88a38f6c..d6cc1df1ea4309 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/collector.go +++ b/src/go/plugin/go.d/collector/snmp_traps/collector.go @@ -18,6 +18,8 @@ import ( "github.com/gosnmp/gosnmp" "github.com/netdata/netdata/go/plugins/pkg/metrix" "github.com/netdata/netdata/go/plugins/plugin/framework/collectorapi" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" ) //go:embed "config_schema.json" @@ -40,20 +42,38 @@ func directJournalLogsAvailable() bool { return activeDirectJournalJobs.Load() > 0 } -func init() { - collectorapi.Register("snmp_traps", collectorapi.Creator{ +// Register registers the SNMP traps collector with shared SNMP-family enrichment state. +func Register(deviceStore *ddsnmp.DeviceStore, topologyEnricher *snmptopology.TrapEnrichmentHandle) { + collectorapi.Register("snmp_traps", newCreator(deviceStore, topologyEnricher)) +} + +func newCreator(deviceStore *ddsnmp.DeviceStore, topologyEnricher *snmptopology.TrapEnrichmentHandle) collectorapi.Creator { + if deviceStore == nil { + panic("snmp_traps Register requires a non-nil device store") + } + if topologyEnricher == nil { + panic("snmp_traps Register requires a non-nil trap enrichment handle") + } + return collectorapi.Creator{ JobConfigSchema: configSchema, Defaults: collectorapi.Defaults{ UpdateEvery: 1, }, - CreateV2: func() collectorapi.CollectorV2 { return New() }, + CreateV2: func() collectorapi.CollectorV2 { return New(deviceStore, topologyEnricher) }, Config: func() any { return &Config{} }, Methods: snmpTrapsMethods, MethodHandler: snmpTrapsMethodHandler, - }) + } } -func New() *Collector { +// New returns an SNMP traps collector using the provided SNMP-family enrichment state. +func New(deviceStore *ddsnmp.DeviceStore, topologyEnricher *snmptopology.TrapEnrichmentHandle) *Collector { + if deviceStore == nil { + panic("snmp_traps New requires a non-nil device store") + } + if topologyEnricher == nil { + panic("snmp_traps New requires a non-nil trap enrichment handle") + } store := metrix.NewCollectorStore() return &Collector{ @@ -63,7 +83,9 @@ func New() *Collector { ReceiveBuffer: defaultListenerReceiveBuffer, }, }, - store: store, + store: store, + deviceLookup: deviceStore, + topologyEnricher: topologyEnricher, } } @@ -75,6 +97,8 @@ type Collector struct { trapWriter TrapWriter journalDir string store metrix.CollectorStore + deviceLookup deviceLookup + topologyEnricher trapTopologyEnricher jobName string vnode string versions map[SnmpVersion]struct{} @@ -636,7 +660,7 @@ func (c *Collector) handlePacket(data []byte, peerIP net.IP, conn *net.UDPConn, entry := trapEntryFromPDU(c.jobName, pdu, td, time.Now().UnixMicro(), monotonicUsec()) entry.PacketSequence = packetSequence - enrichTrapEntry(entry, c.reverseDNSEnabled, c.reverseDNS) + c.enrichTrapEntry(entry, c.reverseDNSEnabled, c.reverseDNS) renderTrapEntryTemplates(entry, td) if unknownOID { c.incTrapError("unknown_oid") diff --git a/src/go/plugin/go.d/collector/snmp_traps/collector_e2e_test.go b/src/go/plugin/go.d/collector/snmp_traps/collector_e2e_test.go index ee738679215996..a6a63aa32a42c9 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/collector_e2e_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/collector_e2e_test.go @@ -17,7 +17,7 @@ func TestCollectorReplayPcapThroughListenerToJournal(t *testing.T) { withTestCacheDir(t) port := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("e2e") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} c.Versions = []string{"v2c"} diff --git a/src/go/plugin/go.d/collector/snmp_traps/enrich.go b/src/go/plugin/go.d/collector/snmp_traps/enrich.go index 990b2963476caa..e738b1bd3992d4 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/enrich.go +++ b/src/go/plugin/go.d/collector/snmp_traps/enrich.go @@ -278,10 +278,19 @@ type deviceEnrichment struct { matches int } -var trapTopologyEnrichmentForSource = snmptopology.TrapEnrichmentForSource +type deviceLookup interface { + DevicesByHostname(hostname string) []ddsnmp.DeviceConnectionInfo +} + +type trapTopologyEnricher interface { + EnrichmentForSource(ip, trapIfIndex string) *snmptopology.TrapTopologyEnrichment +} -func resolveDeviceEnrichment(sourceIP string) deviceEnrichment { - devices := ddsnmp.DeviceRegistry.DevicesByHostname(sourceIP) +func (c *Collector) resolveDeviceEnrichment(sourceIP string) deviceEnrichment { + if c == nil || c.deviceLookup == nil { + return deviceEnrichment{} + } + devices := c.deviceLookup.DevicesByHostname(sourceIP) enrich := deviceEnrichment{matches: len(devices)} if len(devices) != 1 { return enrich @@ -305,7 +314,7 @@ func resolveDeviceEnrichment(sourceIP string) deviceEnrichment { return enrich } -func enrichTrapEntry(entry *TrapEntry, useReverseDNS bool, dns *reverseDNSResolver) { +func (c *Collector) enrichTrapEntry(entry *TrapEntry, useReverseDNS bool, dns *reverseDNSResolver) { if entry == nil { return } @@ -324,7 +333,7 @@ func enrichTrapEntry(entry *TrapEntry, useReverseDNS bool, dns *reverseDNSResolv audit.Source = &TrapSourceAudit{Selected: sourceIP, Method: "entry_source"} } - enrich := resolveDeviceEnrichment(sourceIP) + enrich := c.resolveDeviceEnrichment(sourceIP) audit.Registry = &TrapEnrichmentLookup{ Key: sourceIP, Status: lookupStatus(enrich.matches), @@ -364,7 +373,10 @@ func enrichTrapEntry(entry *TrapEntry, useReverseDNS bool, dns *reverseDNSResolv addTrapEnrichmentApplied(audit, "TRAP_INTERFACE", iface) } - topo := trapTopologyEnrichmentForSource(sourceIP, trapIfIndex) + var topo *snmptopology.TrapTopologyEnrichment + if c != nil && c.topologyEnricher != nil { + topo = c.topologyEnricher.EnrichmentForSource(sourceIP, trapIfIndex) + } topologyTrusted := topo != nil && topo.DeviceStatus == "matched" if topo != nil { audit.Topology = &TrapEnrichmentLookup{ diff --git a/src/go/plugin/go.d/collector/snmp_traps/enrich_test.go b/src/go/plugin/go.d/collector/snmp_traps/enrich_test.go index 90922ace5a1dbe..e0cb2b0c9a2594 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/enrich_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/enrich_test.go @@ -12,15 +12,15 @@ import ( ) func TestEnrichTrapEntryHostnamePriority(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.1.2.3:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.1.2.3", SysName: "core-sw-01", VnodeHostname: "core-sw.mydc.example.com", Vendor: "cisco", VnodeGUID: "8f72c1e2-3a4b-5c6d-7e8f-9a0b1c2d3e4f", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) dns := newReverseDNSResolver() @@ -49,7 +49,7 @@ func TestEnrichTrapEntryHostnamePriority(t *testing.T) { entry := &TrapEntry{ SourceIP: tc.sourceIP, } - enrichTrapEntry(entry, tc.useReverseDNS, dns) + c.enrichTrapEntry(entry, tc.useReverseDNS, dns) if entry.DeviceHostname != tc.wantHostname { t.Errorf("DeviceHostname = %q, want %q", entry.DeviceHostname, tc.wantHostname) @@ -101,8 +101,7 @@ func TestEnrichTrapEntryRegistryHostnameWinsOverTopologyAndReverseDNS(t *testing }, } - prev := trapTopologyEnrichmentForSource - trapTopologyEnrichmentForSource = func(ip, ifIndex string) *snmptopology.TrapTopologyEnrichment { + topologyEnricher := testTrapTopologyEnricher(func(ip, ifIndex string) *snmptopology.TrapTopologyEnrichment { vnodeID := "topology-vnode-id" if ip == "10.1.2.6" { vnodeID = "registry-vnode-id" @@ -120,14 +119,13 @@ func TestEnrichTrapEntryRegistryHostnameWinsOverTopologyAndReverseDNS(t *testing NeighborStatus: "matched", Neighbors: []string{"topo-neighbor"}, } - } - t.Cleanup(func() { trapTopologyEnrichmentForSource = prev }) + }) for tcName, tc := range tests { t.Run(tcName, func(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(topologyEnricher) regKey := "key:" + tc.info.Hostname + ":162" - ddsnmp.DeviceRegistry.Register(regKey, tc.info) - defer ddsnmp.DeviceRegistry.Unregister(regKey) + store.Register(regKey, tc.info) dns := newReverseDNSResolver() dns.cache[tc.info.Hostname] = reverseDNSCacheEntry{ @@ -142,7 +140,7 @@ func TestEnrichTrapEntryRegistryHostnameWinsOverTopologyAndReverseDNS(t *testing {Name: "ifIndex", OID: ifIndexOIDPrefix + ".1", Type: "InterfaceIndex", Value: int64(1)}, }, } - enrichTrapEntry(entry, true, dns) + c.enrichTrapEntry(entry, true, dns) if entry.DeviceHostname != tc.wantHost { t.Errorf("DeviceHostname = %q, want %q", entry.DeviceHostname, tc.wantHost) @@ -164,16 +162,16 @@ func TestEnrichTrapEntryRegistryHostnameWinsOverTopologyAndReverseDNS(t *testing } func TestEnrichTrapEntrySysNameOverVnodeUnknown(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.1.2.4:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.1.2.4", SysName: "real-switch", VnodeHostname: "unknown", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) entry := &TrapEntry{SourceIP: "10.1.2.4"} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "real-switch" { t.Errorf("DeviceHostname = %q, want real-switch (unknown vnode hostname treated as unresolved)", entry.DeviceHostname) @@ -181,24 +179,25 @@ func TestEnrichTrapEntrySysNameOverVnodeUnknown(t *testing.T) { } func TestEnrichTrapEntryEmptySysNameSkipped(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.1.2.5:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.1.2.5", SysName: "", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) entry := &TrapEntry{SourceIP: "10.1.2.5"} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty (empty sysName treated as unresolved)", entry.DeviceHostname) } } -func TestEnrichTrapEntryNoDeviceRegistryMatch(t *testing.T) { +func TestEnrichTrapEntryNoDeviceStoreMatch(t *testing.T) { + c, _ := newTestTrapEnrichmentCollector(nil) entry := &TrapEntry{SourceIP: "172.16.0.99"} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty for unknown device", entry.DeviceHostname) @@ -211,22 +210,21 @@ func TestEnrichTrapEntryNoDeviceRegistryMatch(t *testing.T) { } } -func TestEnrichTrapEntryAmbiguousDeviceRegistryMatchDoesNotEnrich(t *testing.T) { - ddsnmp.DeviceRegistry.Register("job-a:10.9.9.1:162", ddsnmp.DeviceConnectionInfo{ +func TestEnrichTrapEntryAmbiguousDeviceStoreMatchDoesNotEnrich(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) + store.Register("job-a:10.9.9.1:162", ddsnmp.DeviceConnectionInfo{ Hostname: "10.9.9.1", SysName: "switch-a", Vendor: "vendor-a", }) - defer ddsnmp.DeviceRegistry.Unregister("job-a:10.9.9.1:162") - ddsnmp.DeviceRegistry.Register("job-b:10.9.9.1:162", ddsnmp.DeviceConnectionInfo{ + store.Register("job-b:10.9.9.1:162", ddsnmp.DeviceConnectionInfo{ Hostname: "10.9.9.1", SysName: "switch-b", Vendor: "vendor-b", }) - defer ddsnmp.DeviceRegistry.Unregister("job-b:10.9.9.1:162") entry := &TrapEntry{SourceIP: "10.9.9.1"} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty for ambiguous registry source", entry.DeviceHostname) @@ -243,8 +241,7 @@ func TestEnrichTrapEntryAmbiguousDeviceRegistryMatchDoesNotEnrich(t *testing.T) } func TestEnrichTrapEntryDoesNotUseTopologyOnVnodeConflict(t *testing.T) { - prev := trapTopologyEnrichmentForSource - trapTopologyEnrichmentForSource = func(_, ifIndex string) *snmptopology.TrapTopologyEnrichment { + topologyEnricher := testTrapTopologyEnricher(func(_, ifIndex string) *snmptopology.TrapTopologyEnrichment { return &snmptopology.TrapTopologyEnrichment{ DeviceStatus: "matched", DeviceMethod: "management_ip", @@ -258,15 +255,14 @@ func TestEnrichTrapEntryDoesNotUseTopologyOnVnodeConflict(t *testing.T) { NeighborStatus: "matched", Neighbors: []string{"dist-a"}, } - } - t.Cleanup(func() { trapTopologyEnrichmentForSource = prev }) + }) - ddsnmp.DeviceRegistry.Register("job-a:10.9.9.2:162", ddsnmp.DeviceConnectionInfo{ + c, store := newTestTrapEnrichmentCollector(topologyEnricher) + store.Register("job-a:10.9.9.2:162", ddsnmp.DeviceConnectionInfo{ Hostname: "10.9.9.2", SysName: "registry-switch", VnodeGUID: "registry-vnode-id", }) - defer ddsnmp.DeviceRegistry.Unregister("job-a:10.9.9.2:162") entry := &TrapEntry{ SourceIP: "10.9.9.2", @@ -274,7 +270,7 @@ func TestEnrichTrapEntryDoesNotUseTopologyOnVnodeConflict(t *testing.T) { {Name: "ifIndex", OID: ifIndexOIDPrefix + ".1", Type: "InterfaceIndex", Value: int64(1)}, }, } - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "registry-switch" { t.Errorf("DeviceHostname = %q, want registry-switch", entry.DeviceHostname) @@ -294,11 +290,10 @@ func TestEnrichTrapEntryDoesNotUseTopologyOnVnodeConflict(t *testing.T) { } func TestEnrichTrapEntryUsesTrapVarbindInterfaceWithoutTopology(t *testing.T) { - prev := trapTopologyEnrichmentForSource - trapTopologyEnrichmentForSource = func(_, _ string) *snmptopology.TrapTopologyEnrichment { + topologyEnricher := testTrapTopologyEnricher(func(_, _ string) *snmptopology.TrapTopologyEnrichment { return nil - } - t.Cleanup(func() { trapTopologyEnrichmentForSource = prev }) + }) + c, _ := newTestTrapEnrichmentCollector(topologyEnricher) entry := &TrapEntry{ SourceIP: "10.9.9.3", @@ -307,7 +302,7 @@ func TestEnrichTrapEntryUsesTrapVarbindInterfaceWithoutTopology(t *testing.T) { {Name: "ifName", OID: ifNameOIDPrefix + ".29", Type: "OctetString", Value: "uplink-29"}, }, } - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.TopologyInterface != "uplink-29" { t.Errorf("TopologyInterface = %q, want uplink-29", entry.TopologyInterface) @@ -327,8 +322,9 @@ func TestEnrichTrapEntryUsesTrapVarbindInterfaceWithoutTopology(t *testing.T) { } func TestEnrichTrapEntrySourceUDPPeerFallback(t *testing.T) { + c, _ := newTestTrapEnrichmentCollector(nil) entry := &TrapEntry{SourceUDPPeer: "192.168.1.1"} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty (no device match)", entry.DeviceHostname) @@ -336,12 +332,14 @@ func TestEnrichTrapEntrySourceUDPPeerFallback(t *testing.T) { } func TestEnrichTrapEntryNilEntry(t *testing.T) { - enrichTrapEntry(nil, false, nil) + c, _ := newTestTrapEnrichmentCollector(nil) + c.enrichTrapEntry(nil, false, nil) } func TestEnrichTrapEntryNoSource(t *testing.T) { + c, _ := newTestTrapEnrichmentCollector(nil) entry := &TrapEntry{} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty", entry.DeviceHostname) @@ -349,11 +347,11 @@ func TestEnrichTrapEntryNoSource(t *testing.T) { } func TestEnrichTrapEntryReverseDNSDefaultOff(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.5.5.1:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.5.5.1", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) dns := newReverseDNSResolver() dns.cache["10.5.5.1"] = reverseDNSCacheEntry{ @@ -362,7 +360,7 @@ func TestEnrichTrapEntryReverseDNSDefaultOff(t *testing.T) { } entry := &TrapEntry{SourceIP: "10.5.5.1"} - enrichTrapEntry(entry, false, dns) + c.enrichTrapEntry(entry, false, dns) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty (reverse DNS disabled, no vnode/sysName)", entry.DeviceHostname) @@ -370,6 +368,7 @@ func TestEnrichTrapEntryReverseDNSDefaultOff(t *testing.T) { } func TestEnrichTrapEntryReverseDNSEnabledNoSNMPState(t *testing.T) { + c, _ := newTestTrapEnrichmentCollector(nil) dns := newReverseDNSResolver() dns.cache["10.6.6.1"] = reverseDNSCacheEntry{ name: "peer.mydc.example.com", @@ -377,7 +376,7 @@ func TestEnrichTrapEntryReverseDNSEnabledNoSNMPState(t *testing.T) { } entry := &TrapEntry{SourceIP: "10.6.6.1"} - enrichTrapEntry(entry, true, dns) + c.enrichTrapEntry(entry, true, dns) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty because reverse DNS is not authoritative identity", entry.DeviceHostname) @@ -394,11 +393,11 @@ func TestEnrichTrapEntryReverseDNSEnabledNoSNMPState(t *testing.T) { } func TestEnrichTrapEntryReverseDNSDisabledNoCacheUse(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.7.7.1:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.7.7.1", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) dns := newReverseDNSResolver() dns.cache["10.7.7.1"] = reverseDNSCacheEntry{ @@ -407,7 +406,7 @@ func TestEnrichTrapEntryReverseDNSDisabledNoCacheUse(t *testing.T) { } entry := &TrapEntry{SourceIP: "10.7.7.1"} - enrichTrapEntry(entry, false, dns) + c.enrichTrapEntry(entry, false, dns) if entry.DeviceHostname != "" { t.Errorf("DeviceHostname = %q, want empty (reverse DNS disabled, no SNMP state)", entry.DeviceHostname) @@ -415,12 +414,12 @@ func TestEnrichTrapEntryReverseDNSDisabledNoCacheUse(t *testing.T) { } func TestEnrichTrapEntryReverseDNSDoesNotReplaceKnownHostname(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:10.7.7.2:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "10.7.7.2", SysName: "known-switch", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) dns := newReverseDNSResolver() dns.cache["10.7.7.2"] = reverseDNSCacheEntry{ @@ -428,7 +427,7 @@ func TestEnrichTrapEntryReverseDNSDoesNotReplaceKnownHostname(t *testing.T) { expiresAt: farFuture(), } entry := &TrapEntry{SourceIP: "10.7.7.2"} - enrichTrapEntry(entry, true, dns) + c.enrichTrapEntry(entry, true, dns) if entry.DeviceHostname != "known-switch" { t.Errorf("DeviceHostname = %q, want known-switch", entry.DeviceHostname) @@ -439,6 +438,7 @@ func TestEnrichTrapEntryReverseDNSDoesNotReplaceKnownHostname(t *testing.T) { } func TestEnrichTrapEntryReverseDNSEnabledSchedulesAsyncLookup(t *testing.T) { + c, _ := newTestTrapEnrichmentCollector(nil) dns := newReverseDNSResolver() defer dns.Close() @@ -455,7 +455,7 @@ func TestEnrichTrapEntryReverseDNSEnabledSchedulesAsyncLookup(t *testing.T) { } entry := &TrapEntry{SourceIP: "203.0.113.10"} - enrichTrapEntry(entry, true, dns) + c.enrichTrapEntry(entry, true, dns) select { case <-started: @@ -515,17 +515,17 @@ func TestEnrichTrapEntryVendorAndVnodeEnrichment(t *testing.T) { for tcName, tc := range tests { t.Run(tcName, func(t *testing.T) { + c, store := newTestTrapEnrichmentCollector(nil) regKey := "key:" + tc.hostname + ":162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + store.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: tc.hostname, SysName: tc.sysName, Vendor: tc.vendor, VnodeGUID: tc.vnodeGUID, }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) entry := &TrapEntry{SourceIP: tc.hostname} - enrichTrapEntry(entry, false, nil) + c.enrichTrapEntry(entry, false, nil) if entry.DeviceVendor != tc.wantVendor { t.Errorf("DeviceVendor = %q, want %q", entry.DeviceVendor, tc.wantVendor) diff --git a/src/go/plugin/go.d/collector/snmp_traps/func_logs_test.go b/src/go/plugin/go.d/collector/snmp_traps/func_logs_test.go index a219b1564dd656..4f242ed945967c 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/func_logs_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/func_logs_test.go @@ -14,6 +14,8 @@ import ( "github.com/netdata/netdata/go/plugins/plugin/agent/jobmgr/funcctl" "github.com/netdata/netdata/go/plugins/plugin/framework/collectorapi" "github.com/netdata/netdata/go/plugins/plugin/framework/functions" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_traps/snmptrapsfunc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -147,8 +149,7 @@ func TestSNMPTrapsLogsDispatchDoesNotRequireRunningJob(t *testing.T) { t.Cleanup(func() { activeDirectJournalJobs.Store(startJournalJobs) }) t.Setenv(netdataLogDirEnv, filepath.Join(t.TempDir(), "logs")) - creator, ok := collectorapi.DefaultRegistry.Lookup("snmp_traps") - require.True(t, ok) + creator := newCreator(ddsnmp.NewDeviceStore(), snmptopology.NewTrapEnrichmentHandle()) reg := newSNMPTrapsTestFunctionRegistry() var gotCode int diff --git a/src/go/plugin/go.d/collector/snmp_traps/init_test.go b/src/go/plugin/go.d/collector/snmp_traps/init_test.go index 731b05240cd4a8..426e5574e0db16 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/init_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/init_test.go @@ -13,18 +13,19 @@ import ( "github.com/netdata/netdata/go/plugins/plugin/framework/chartengine" "github.com/netdata/netdata/go/plugins/plugin/framework/charttpl" - "github.com/netdata/netdata/go/plugins/plugin/framework/collectorapi" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" "github.com/netdata/netdata/go/plugins/plugin/go.d/pkg/collecttest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestCollectorChartTemplateYAML(t *testing.T) { - collecttest.AssertChartTemplateSchema(t, New().ChartTemplateYAML()) + collecttest.AssertChartTemplateSchema(t, newTestSNMPTrapsCollector().ChartTemplateYAML()) } func TestCollectorChartTemplateYAMLChartsDeclareAlgorithms(t *testing.T) { - charts := chartTemplatesByIDFromYAML(t, New().ChartTemplateYAML()) + charts := chartTemplatesByIDFromYAML(t, newTestSNMPTrapsCollector().ChartTemplateYAML()) assertAllChartTemplatesDeclareAlgorithm(t, charts) for _, id := range []string{ @@ -68,7 +69,7 @@ func TestCollectorChartTemplateYAMLIncludesProfileMetricCharts(t *testing.T) { require.NotNil(t, rt) require.NotEmpty(t, tmpl) - c := New() + c := newTestSNMPTrapsCollector() c.profileMetrics = rt c.dynamicChartYAML = tmpl @@ -90,12 +91,29 @@ func TestCollectorChartTemplateYAMLIncludesProfileMetricCharts(t *testing.T) { assert.Contains(t, contexts, "snmp.trap.cisco.config.changes") } -func TestCollectorRegistrationAvailableByDefault(t *testing.T) { - creator, ok := collectorapi.DefaultRegistry.Lookup("snmp_traps") - require.True(t, ok) +func TestCollectorCreatorDefaults(t *testing.T) { + creator := newCreator(ddsnmp.NewDeviceStore(), snmptopology.NewTrapEnrichmentHandle()) assert.False(t, creator.Defaults.Disabled) } +func TestCollectorCreatorRequiresSharedDependencies(t *testing.T) { + require.PanicsWithValue(t, "snmp_traps Register requires a non-nil device store", func() { + _ = newCreator(nil, snmptopology.NewTrapEnrichmentHandle()) + }) + require.PanicsWithValue(t, "snmp_traps Register requires a non-nil trap enrichment handle", func() { + _ = newCreator(ddsnmp.NewDeviceStore(), nil) + }) +} + +func TestCollectorNewRequiresSharedDependencies(t *testing.T) { + require.PanicsWithValue(t, "snmp_traps New requires a non-nil device store", func() { + _ = New(nil, snmptopology.NewTrapEnrichmentHandle()) + }) + require.PanicsWithValue(t, "snmp_traps New requires a non-nil trap enrichment handle", func() { + _ = New(ddsnmp.NewDeviceStore(), nil) + }) +} + func chartTemplatesByIDFromYAML(t *testing.T, raw string) map[string]charttpl.Chart { t.Helper() @@ -215,7 +233,7 @@ func TestConfigSchemaDynCfgRetentionDefaultDisablesTimeRotation(t *testing.T) { } func TestCollectorDefaultListenReceiveBuffer(t *testing.T) { - assert.Equal(t, defaultListenerReceiveBuffer, New().Listen.ReceiveBuffer) + assert.Equal(t, defaultListenerReceiveBuffer, newTestSNMPTrapsCollector().Listen.ReceiveBuffer) } func TestConfigSchemaDynCfgTabsRenderAllTopLevelFieldsOnce(t *testing.T) { @@ -469,7 +487,7 @@ func TestCollectorInit_BindsEndpointsAndCheckIsNoop(t *testing.T) { withTestCacheDir(t) port := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} @@ -493,7 +511,7 @@ func TestCollectorInit_IdempotentDoubleInit(t *testing.T) { withTestCacheDir(t) port := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} @@ -510,7 +528,7 @@ func TestCollectorInit_IdempotentDoubleInit(t *testing.T) { func TestCollectorInit_InvalidJobNameIsCodedError(t *testing.T) { withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("../bad") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: 162}} @@ -528,7 +546,7 @@ func TestCollectorInit_InvalidJobNameIsCodedError(t *testing.T) { func TestCollectorInit_InvalidEndpointsIsCodedError(t *testing.T) { withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "tcp", Address: "127.0.0.1", Port: 162}} @@ -543,7 +561,7 @@ func TestCollectorInit_InvalidEndpointsIsCodedError(t *testing.T) { func TestCollectorInit_InvalidReceiveBufferIsCodedError(t *testing.T) { withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Listen.ReceiveBuffer = -1 @@ -560,7 +578,7 @@ func TestCollectorInit_InvalidReceiveBufferIsCodedError(t *testing.T) { func TestCollectorInit_TooLargeReceiveBufferIsCodedError(t *testing.T) { withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Listen.ReceiveBuffer = maxListenerReceiveBuffer + 1 @@ -576,7 +594,7 @@ func TestCollectorInit_TooLargeReceiveBufferIsCodedError(t *testing.T) { func TestCollectorInit_NoOutputBackendIsCodedError(t *testing.T) { disabled := false - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Journal.Enabled = &disabled @@ -595,7 +613,7 @@ func TestCollectorInit_MissingNetdataLogRootIsRetryableCodedError(t *testing.T) root := filepath.Join(t.TempDir(), "missing") withNetdataLogDir(t, root) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} @@ -623,7 +641,7 @@ func TestCollectorInit_OTELOnlySkipsJournalCreation(t *testing.T) { srv := startOTLPFixture(t, nil) const jobName = "otel-only" - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName(jobName) c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Journal.Enabled = &disabled @@ -657,7 +675,7 @@ func TestCollectorInit_OTLPPreflightFailureIsRetryableCodedError(t *testing.T) { endpoint := "http://" + ln.Addr().String() require.NoError(t, ln.Close()) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("otlp-preflight") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Journal.Enabled = &disabled @@ -685,7 +703,7 @@ func TestCollectorInit_BindsMultipleEndpoints(t *testing.T) { firstPort := freeUDPPort(t) secondPort := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{ {Protocol: "udp", Address: "127.0.0.1", Port: firstPort}, @@ -717,7 +735,7 @@ func TestCollectorInit_BindFailureIsRetryableCodedError(t *testing.T) { port := conn.LocalAddr().(*net.UDPAddr).Port - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} @@ -742,7 +760,7 @@ func TestCollectorInit_ReceiveBufferFailureIsRetryableCodedError(t *testing.T) { return errors.New("set buffer failed") } - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} @@ -761,7 +779,7 @@ func TestCollectorInit_ReceiveBufferFailureIsRetryableCodedError(t *testing.T) { func TestCollectorInit_InvalidVersionIsCodedError(t *testing.T) { withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: 162}} c.Versions = []string{"v5"} @@ -779,7 +797,7 @@ func TestCollectorInit_ProfileLoadFailureIsCodedError(t *testing.T) { resetProfileCacheForTest() withTestCacheDir(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Versions = []string{" V1 ", "V2C"} @@ -802,7 +820,7 @@ func TestCollectorInit_PartialBindFailureClosesPriorSockets(t *testing.T) { defer secondConn.Close() secondPort := secondConn.LocalAddr().(*net.UDPAddr).Port - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{ {Protocol: "udp", Address: "127.0.0.1", Port: firstPort}, @@ -836,7 +854,7 @@ func TestCollectorInit_EngineStateStatErrorIsRetryableCodedError(t *testing.T) { const jobName = "engine-state-stat-error" require.NoError(t, os.WriteFile(engineBootsDir(jobName), []byte("not a directory"), 0644)) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName(jobName) c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Versions = []string{"v3"} @@ -870,7 +888,7 @@ func TestCollectorInit_CleansCreatedV3StateOnEngineBootsFailure(t *testing.T) { const jobName = "cleanup-v3-state" require.NoError(t, os.MkdirAll(engineBootsPath(jobName), 0750)) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName(jobName) c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: freeUDPPort(t)}} c.Versions = []string{"v3"} @@ -896,7 +914,7 @@ func TestCollectorCleanupIsIdempotent(t *testing.T) { withTestCacheDir(t) port := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} @@ -909,7 +927,7 @@ func TestCollectorCleanupIsIdempotent(t *testing.T) { } func TestCollectorCollectRequiresStartedListener(t *testing.T) { - c := New() + c := newTestSNMPTrapsCollector() err := c.Collect(context.Background()) require.Error(t, err) assert.Contains(t, err.Error(), "listener not started") diff --git a/src/go/plugin/go.d/collector/snmp_traps/listener_test.go b/src/go/plugin/go.d/collector/snmp_traps/listener_test.go index 8fad389483bd7d..fb93c506bd4667 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/listener_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/listener_test.go @@ -81,7 +81,7 @@ func TestListenerReadLoopDoesNotReportReadErrorDuringClose(t *testing.T) { func TestCollectorLogListenerReadErrorIsRateLimited(t *testing.T) { var buf bytes.Buffer - c := New() + c := newTestSNMPTrapsCollector() c.Logger = logger.NewWithWriter(&buf) ep := EndpointConfig{ Protocol: "udp4", diff --git a/src/go/plugin/go.d/collector/snmp_traps/pipeline_test.go b/src/go/plugin/go.d/collector/snmp_traps/pipeline_test.go index 82fea57ff8332f..7ba7d01a9d523c 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/pipeline_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/pipeline_test.go @@ -119,18 +119,19 @@ func TestCollectorHandlePacketRecoversFromPanic(t *testing.T) { func TestCollectorHandlePacketRendersTemplatesAfterEnrichment(t *testing.T) { packet := readColdStartUDPPacket(t) + deviceStore := ddsnmp.NewDeviceStore() regKey := "test:198.51.100.10:162" - ddsnmp.DeviceRegistry.Register(regKey, ddsnmp.DeviceConnectionInfo{ + deviceStore.Register(regKey, ddsnmp.DeviceConnectionInfo{ Hostname: "198.51.100.10", SysName: "core-sw-01", Vendor: "cisco", }) - defer ddsnmp.DeviceRegistry.Unregister(regKey) trap := testColdStartTrap("security", "warning", "security coldStart on {_HOSTNAME} from {TRAP_DEVICE_VENDOR}") setSingleTestTrap(t, trap) writer := &mockTrapWriter{} c := newDefaultTestV2Collector(writer) + c.deviceLookup = deviceStore c.handlePacket(packet.payload, packet.peer, nil, nil) @@ -162,8 +163,7 @@ func TestCollectorHandlePacketDoesNotUseListenerVnodeAsSourceNode(t *testing.T) func TestCollectorHandlePacketRendersTopologyEnrichmentBeforeReverseDNS(t *testing.T) { packet := readColdStartUDPPacket(t) - prev := trapTopologyEnrichmentForSource - trapTopologyEnrichmentForSource = func(ip, ifIndex string) *snmptopology.TrapTopologyEnrichment { + topologyEnricher := testTrapTopologyEnricher(func(ip, ifIndex string) *snmptopology.TrapTopologyEnrichment { if ip != "198.51.100.10" { t.Fatalf("topology enrichment looked up IP %q, want 198.51.100.10", ip) } @@ -180,8 +180,7 @@ func TestCollectorHandlePacketRendersTopologyEnrichmentBeforeReverseDNS(t *testi InterfaceStatus: "skipped", NeighborStatus: "skipped", } - } - t.Cleanup(func() { trapTopologyEnrichmentForSource = prev }) + }) dns := newReverseDNSResolver() dns.cache["198.51.100.10"] = reverseDNSCacheEntry{ @@ -198,6 +197,7 @@ func TestCollectorHandlePacketRendersTopologyEnrichmentBeforeReverseDNS(t *testi setSingleTestTrap(t, trap) writer := &mockTrapWriter{} c := newDefaultTestV2Collector(writer) + c.topologyEnricher = topologyEnricher c.reverseDNSEnabled = true c.reverseDNS = dns diff --git a/src/go/plugin/go.d/collector/snmp_traps/profile_test.go b/src/go/plugin/go.d/collector/snmp_traps/profile_test.go index 2978eb74fc234c..ac5f6e2bc80d10 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/profile_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/profile_test.go @@ -90,7 +90,7 @@ func TestCollectorInitAcquiresProfileCache(t *testing.T) { port := freeUDPPort(t) - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port}} @@ -109,11 +109,11 @@ func TestMultipleCollectorsShareSameCache(t *testing.T) { port1 := freeUDPPort(t) port2 := freeUDPPort(t) - c1 := New() + c1 := newTestSNMPTrapsCollector() c1.SetJobName("job1") c1.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port1}} - c2 := New() + c2 := newTestSNMPTrapsCollector() c2.SetJobName("job2") c2.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: port2}} @@ -142,7 +142,7 @@ func TestInitBindFailureReleasesProfileRef(t *testing.T) { require.NoError(t, err) defer conn.Close() - c := New() + c := newTestSNMPTrapsCollector() c.SetJobName("local") c.Listen.Endpoints = []EndpointConfig{{Protocol: "udp", Address: "127.0.0.1", Port: conn.LocalAddr().(*net.UDPAddr).Port}} @@ -444,7 +444,7 @@ traps: require.Equal(t, "diagnostic", td.Category) require.Equal(t, "warning", td.Severity) - c := New() + c := newTestSNMPTrapsCollector() c.overrides = buildOverrideMap([]OverrideConfig{ { OID: oid, diff --git a/src/go/plugin/go.d/collector/snmp_traps/test_helpers_test.go b/src/go/plugin/go.d/collector/snmp_traps/test_helpers_test.go index a41b0ba0a6d556..6bc7baa67e1d8e 100644 --- a/src/go/plugin/go.d/collector/snmp_traps/test_helpers_test.go +++ b/src/go/plugin/go.d/collector/snmp_traps/test_helpers_test.go @@ -10,6 +10,8 @@ import ( "github.com/gosnmp/gosnmp" "github.com/netdata/netdata/go/plugins/pkg/metrix" + "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp/ddsnmp" + snmptopology "github.com/netdata/netdata/go/plugins/plugin/go.d/collector/snmp_topology" ) func readSinglePcapUDPPacket(t *testing.T, fixture string) pcapUDPPacket { @@ -55,6 +57,24 @@ func newDefaultTestV2Collector(writer TrapWriter) *Collector { return newTestV2Collector("test", writer, nil, []string{"public"}) } +func newTestSNMPTrapsCollector() *Collector { + return New(ddsnmp.NewDeviceStore(), snmptopology.NewTrapEnrichmentHandle()) +} + +type testTrapTopologyEnricher func(ip, trapIfIndex string) *snmptopology.TrapTopologyEnrichment + +func (f testTrapTopologyEnricher) EnrichmentForSource(ip, trapIfIndex string) *snmptopology.TrapTopologyEnrichment { + return f(ip, trapIfIndex) +} + +func newTestTrapEnrichmentCollector(topologyEnricher trapTopologyEnricher) (*Collector, *ddsnmp.DeviceStore) { + store := ddsnmp.NewDeviceStore() + return &Collector{ + deviceLookup: store, + topologyEnricher: topologyEnricher, + }, store +} + func withCleanJobMetrics(t *testing.T, jobName string) *perJobMetrics { t.Helper() removeJobMetrics(jobName) diff --git a/src/libnetdata/c_rhash/tests.c b/src/libnetdata/c_rhash/tests.c index 3caa7d003662d6..061f4e5ee51a75 100644 --- a/src/libnetdata/c_rhash/tests.c +++ b/src/libnetdata/c_rhash/tests.c @@ -105,7 +105,6 @@ int test_uint64_ptr() { #define UINT64_PTR_INC_ITERATION_COUNT 5000 int test_uint64_ptr_incremental() { c_rhash hash = c_rhash_new(100); - void *val; TEST_START(); diff --git a/src/libnetdata/inlined.h b/src/libnetdata/inlined.h index ee5d90051ab2d4..aa8ee11b9530d7 100644 --- a/src/libnetdata/inlined.h +++ b/src/libnetdata/inlined.h @@ -108,9 +108,9 @@ static inline uint32_t murmur32(uint32_t k) { static uint64_t murmur64(uint64_t k) __attribute__((const)); static inline uint64_t murmur64(uint64_t k) { k ^= k >> 33; - k *= 0xff51afd7ed558ccdUL; + k *= 0xff51afd7ed558ccdULL; k ^= k >> 33; - k *= 0xc4ceb9fe1a85ec53UL; + k *= 0xc4ceb9fe1a85ec53ULL; k ^= k >> 33; return k; diff --git a/src/libnetdata/log/nd_log.h b/src/libnetdata/log/nd_log.h index 211540f3c80471..b2fe65b8540e51 100644 --- a/src/libnetdata/log/nd_log.h +++ b/src/libnetdata/log/nd_log.h @@ -117,7 +117,7 @@ extern int aclklog_enabled; #define LOG_DATE_LENGTH 26 void log_date(char *buffer, size_t len, time_t now); -static inline void debug_dummy(void) {} +static inline void debug_dummy(void) { /* no-op: target for debug/timing macros when those are compiled out */ } void nd_log_limits_reset(void); void nd_log_limits_unlimited(void); diff --git a/src/libnetdata/log/systemd-cat-native.c b/src/libnetdata/log/systemd-cat-native.c index 7d2a9b05ef9c19..381cf6b6b7a291 100644 --- a/src/libnetdata/log/systemd-cat-native.c +++ b/src/libnetdata/log/systemd-cat-native.c @@ -84,7 +84,6 @@ static inline size_t copy_replacing_newlines(char *dst, size_t dst_len, const ch memcpy(current_dst, current_src, copy_len); current_dst += copy_len; - remaining_dst_len -= copy_len; bytes_copied += copy_len; break; } diff --git a/src/libnetdata/netipc/src/transport/posix/netipc_uds_receive.c b/src/libnetdata/netipc/src/transport/posix/netipc_uds_receive.c index 50da34e5879888..b528bfc57b7a3a 100644 --- a/src/libnetdata/netipc/src/transport/posix/netipc_uds_receive.c +++ b/src/libnetdata/netipc/src/transport/posix/netipc_uds_receive.c @@ -17,7 +17,7 @@ static uint64_t monotonic_ms(void) { struct timespec ts; clock_gettime(CLOCK_MONOTONIC, &ts); - return (uint64_t)ts.tv_sec * 1000ull + (uint64_t)ts.tv_nsec / 1000000ull; + return (uint64_t)ts.tv_sec * 1000ULL + (uint64_t)ts.tv_nsec / 1000000ULL; } static receive_wait_t receive_wait_init(uint32_t timeout_ms, int abort_fd) diff --git a/src/ml/ml.cc b/src/ml/ml.cc index 24258b48f735ea..cc4181c23e2cf0 100644 --- a/src/ml/ml.cc +++ b/src/ml/ml.cc @@ -346,7 +346,7 @@ const char *db_models_delete = const char *db_models_prune = "DELETE FROM models " - "WHERE after < @after LIMIT @n;"; + "WHERE rowid IN (SELECT rowid FROM models WHERE after < @after LIMIT @n);"; static int ml_dimension_add_model(const nd_uuid_t *metric_uuid, const ml_kmeans_inlined_t *inlined_km) diff --git a/src/streaming/stream-connector.c b/src/streaming/stream-connector.c index cfe02a1568f7d0..a94c6314857b1d 100644 --- a/src/streaming/stream-connector.c +++ b/src/streaming/stream-connector.c @@ -693,7 +693,7 @@ bool stream_connector_init(struct sender_state *s) { completion_init(&sc->completion); char tag[NETDATA_THREAD_TAG_MAX + 1]; - snprintfz(tag, NETDATA_THREAD_TAG_MAX, THREAD_TAG_STREAM_SENDER "-CN" "[%d]", + snprintfz(tag, NETDATA_THREAD_TAG_MAX, THREAD_TAG_STREAM_SENDER "-CN[%d]", sc->id); sc->thread = nd_thread_create(tag, NETDATA_THREAD_OPTION_DEFAULT, stream_connector_thread, sc); diff --git a/src/streaming/stream-receiver-internals.h b/src/streaming/stream-receiver-internals.h index 856601b687418a..c68a98d7b0f87e 100644 --- a/src/streaming/stream-receiver-internals.h +++ b/src/streaming/stream-receiver-internals.h @@ -74,6 +74,7 @@ struct receiver_state { nd_poll_event_t wanted; usec_t last_traffic_ut; + size_t bytes_received; // raw socket bytes received on this connection (diagnostics) struct pollfd_meta meta; } thread; diff --git a/src/streaming/stream-receiver.c b/src/streaming/stream-receiver.c index 624c3e0360e87b..ca9396eb399ad3 100644 --- a/src/streaming/stream-receiver.c +++ b/src/streaming/stream-receiver.c @@ -153,6 +153,7 @@ static ssize_t receiver_read_uncompressed(struct receiver_state *r) { r->thread.uncompressed.read_len += bytes; r->thread.uncompressed.read_buffer[r->thread.uncompressed.read_len] = '\0'; + r->thread.bytes_received += bytes; pulse_stream_received_bytes(bytes); } @@ -271,6 +272,7 @@ static ssize_t receiver_read_compressed(struct receiver_state *r) { if(bytes > 0) { r->thread.compressed.used += bytes; + r->thread.bytes_received += bytes; worker_set_metric(WORKER_RECEIVER_JOB_BYTES_READ, (NETDATA_DOUBLE)bytes); pulse_stream_received_bytes(bytes); } @@ -423,6 +425,11 @@ void stream_receiver_move_to_running_unsafe(struct stream_thread *sth, struct re rpt->thread.compressed.enabled = stream_decompression_initialize(rpt); buffered_reader_init(&rpt->thread.uncompressed); + // start fresh at admission: the no-traffic timeout must be measured from when we start + // reading (now), not from when the connection was accepted and queued. + rpt->thread.bytes_received = 0; + rpt->thread.last_traffic_ut = now_monotonic_usec(); + rpt->thread.line_buffer = buffer_create(sizeof(rpt->thread.uncompressed.read_buffer), NULL); // help preferred_sender_buffer() select the right buffer @@ -516,16 +523,39 @@ static void stream_receiver_remove_internal(struct stream_thread *sth, struct re if(parser) count = parser->user.data_collections_count; + // gather diagnostics for a single, uniform disconnect line (same fields for every reason): + // bytes_in distinguishes "the child sent nothing" (network/silent) from "sent but unparsed"; + // iface exposes the child's connection type (e.g. ppp0 cellular vs eth0) for log-side pivots. + size_t bytes_out = 0; + spinlock_lock(&rpt->thread.send_to_child.spinlock); + if(rpt->thread.send_to_child.scb) + bytes_out = stream_circular_buffer_stats_unsafe(rpt->thread.send_to_child.scb)->bytes_sent; + spinlock_unlock(&rpt->thread.send_to_child.spinlock); + + char iface[64] = ""; + if(rpt->host && rpt->host->rrdlabels) + rrdlabels_get_value_strcpyz(rpt->host->rrdlabels, iface, sizeof(iface), "_net_default_iface"); + + time_t connected_s = rpt->connected_since_s ? (now_realtime_sec() - rpt->connected_since_s) : 0; + long long idle_s = (long long)((now_monotonic_usec() - rpt->thread.last_traffic_ut) / USEC_PER_SEC); + double repl_pct = rpt->host ? rpt->host->stream.rcv.status.replication.percent : 0.0; + errno_clear(); nd_log(NDLS_DAEMON, NDLP_ERR, - "STREAM RCV[%zu] '%s' [from [%s]:%s]: " - "receiver disconnected (after %zu received messages): %s" + "STREAM RCV[%zu] '%s' [from [%s]:%s]: receiver disconnected: " + "reason=\"%s\" msgs=%zu bytes_in=%zu bytes_out=%zu connected=%llds idle=%llds repl=%.0f%% iface=%s" , sth->id , rpt->hostname ? rpt->hostname : "-" , rpt->remote_ip ? rpt->remote_ip : "-" , rpt->remote_port ? rpt->remote_port : "-" + , stream_handshake_error_to_string(reason) , count - , stream_handshake_error_to_string(reason)); + , rpt->thread.bytes_received + , bytes_out + , (long long)connected_s + , idle_s + , repl_pct + , iface[0] ? iface : "-"); internal_fatal(META_GET(&sth->run.meta, (Word_t)&rpt->thread.meta) == NULL, "Receiver to be removed is not found in the list of receivers"); diff --git a/src/streaming/stream-replication-sender.c b/src/streaming/stream-replication-sender.c index e15378df96730c..0391bc213bb028 100644 --- a/src/streaming/stream-replication-sender.c +++ b/src/streaming/stream-replication-sender.c @@ -98,6 +98,7 @@ struct replication_query { STORAGE_ENGINE_BACKEND backend; struct replication_request *rq; + size_t alloc_size; // bytes accounted in replication_buffers_allocated for this query size_t dimensions; struct replication_dimension data[]; }; @@ -118,9 +119,11 @@ static struct replication_query *replication_query_prepare( bool synchronous ) { size_t dimensions = rrdset_number_of_dimensions(st); - struct replication_query *q = callocz(1, sizeof(struct replication_query) + dimensions * sizeof(struct replication_dimension)); - __atomic_add_fetch(&replication_buffers_allocated, sizeof(struct replication_query) + dimensions * sizeof(struct replication_dimension), __ATOMIC_RELAXED); + size_t alloc_size = sizeof(struct replication_query) + dimensions * sizeof(struct replication_dimension); + struct replication_query *q = callocz(1, alloc_size); + __atomic_add_fetch(&replication_buffers_allocated, alloc_size, __ATOMIC_RELAXED); + q->alloc_size = alloc_size; q->dimensions = dimensions; q->st = st; @@ -301,7 +304,7 @@ static void replication_query_finalize(BUFFER *wb, struct replication_query *q, spinlock_unlock(&replication_queries.spinlock); } - __atomic_sub_fetch(&replication_buffers_allocated, sizeof(struct replication_query) + dimensions * sizeof(struct replication_dimension), __ATOMIC_RELAXED); + __atomic_sub_fetch(&replication_buffers_allocated, q->alloc_size, __ATOMIC_RELAXED); freez(q); } @@ -1574,6 +1577,7 @@ static void replication_pipeline_cancel_and_cleanup(void) { internal_error(true, "REPLICATION: cancelled %zu inflight queries", cancelled); + __atomic_sub_fetch(&replication_buffers_allocated, rtp.max_requests_ahead * sizeof(struct replication_request), __ATOMIC_RELAXED); freez(rtp.rqs); rtp.rqs = NULL; rtp.max_requests_ahead = 0; diff --git a/src/web/api/functions/function-netdata-streaming.c b/src/web/api/functions/function-netdata-streaming.c index f138de69a5204b..913d17c00d0504 100644 --- a/src/web/api/functions/function-netdata-streaming.c +++ b/src/web/api/functions/function-netdata-streaming.c @@ -309,6 +309,16 @@ int function_netdata_streaming(BUFFER *wb, const char *function __maybe_unused, buffer_json_add_array_item_string(wb, NULL); // MlSilenced } + // MachineGUID + NodeID — hidden columns, to join streaming rows to the node inventory + buffer_json_add_array_item_string(wb, host->machine_guid); // MachineGUID + if(!UUIDiszero(host->node_id)) { + char node_id_str[UUID_STR_LEN]; + uuid_unparse_lower(host->node_id.uuid, node_id_str); + buffer_json_add_array_item_string(wb, node_id_str); // NodeID + } + else + buffer_json_add_array_item_string(wb, NULL); // NodeID + // close buffer_json_array_close(wb); } @@ -879,6 +889,19 @@ int function_netdata_streaming(BUFFER *wb, const char *function __maybe_unused, RRDF_FIELD_SORT_DESCENDING, NULL, RRDF_FIELD_SUMMARY_SUM, RRDF_FIELD_FILTER_RANGE, RRDF_FIELD_OPTS_NONE, NULL); + + // MachineGUID + NodeID — hidden, used to join streaming rows to the node inventory + buffer_rrdf_table_add_field(wb, field_id++, "MachineGUID", "Machine GUID", + RRDF_FIELD_TYPE_STRING, RRDF_FIELD_VISUAL_VALUE, RRDF_FIELD_TRANSFORM_NONE, + 0, NULL, NAN, RRDF_FIELD_SORT_ASCENDING, NULL, + RRDF_FIELD_SUMMARY_COUNT, RRDF_FIELD_FILTER_MULTISELECT, + RRDF_FIELD_OPTS_NONE, NULL); + + buffer_rrdf_table_add_field(wb, field_id++, "NodeID", "Cloud Node ID", + RRDF_FIELD_TYPE_STRING, RRDF_FIELD_VISUAL_VALUE, RRDF_FIELD_TRANSFORM_NONE, + 0, NULL, NAN, RRDF_FIELD_SORT_ASCENDING, NULL, + RRDF_FIELD_SUMMARY_COUNT, RRDF_FIELD_FILTER_MULTISELECT, + RRDF_FIELD_OPTS_NONE, NULL); } buffer_json_object_close(wb); // columns buffer_json_member_add_string(wb, "default_sort_column", "Node"); diff --git a/src/web/api/v2/api_v2_claim.c b/src/web/api/v2/api_v2_claim.c index 423a1d4098b4dc..a3ea2141f43a4e 100644 --- a/src/web/api/v2/api_v2_claim.c +++ b/src/web/api/v2/api_v2_claim.c @@ -219,7 +219,6 @@ static int api_claim(uint8_t version, struct web_client *w, char *url) { if(claim_agent(base_url, token, rooms, cloud_config_proxy_get(), cloud_config_insecure_get())) { msg = "ok"; - can_be_claimed = false; claim_reload_and_wait_online(); response = CLAIM_RESP_ACTION_OK; } diff --git a/src/web/websocket/websocket-send.c b/src/web/websocket/websocket-send.c index a836d2c061a39a..0d9000d539c610 100644 --- a/src/web/websocket/websocket-send.c +++ b/src/web/websocket/websocket-send.c @@ -78,7 +78,7 @@ static int websocket_protocol_send_frame( if(!wsc) return -1; - const char *disconnect_msg = ""; + const char *disconnect_msg; if (wsc->sock.fd < 0) { disconnect_msg = "Client not connected";