GATEWAYS-4306: exporting metrics for conntrack per zone by shrouti1995 · Pull Request #21 · digitalocean/openvswitch_exporter

shrouti1995 · 2025-08-13T10:32:35Z

Adding support to export conntrack metrics per zone.
Details:

Implemented proper metric collection through Prometheus collector interface

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jcooperdo · 2025-09-29T23:57:06Z

internal/ovsexporter/conntrack.go

+// ConntrackCollectorWithAggAccessor wraps the existing collector with access to the aggregator snapshot
+type ConntrackCollectorWithAggAccessor struct {
+	*conntrackCollector
+	SnapshotFunc func() map[uint16]map[uint32]int


Is this used anywhere?

* trying to fix the static check issue * trying to fix the static check issue

* adding test cases * adding mock tests * Context-Based Cancellation Refactoring * add centralised error propagation * solve lint error

* adding test cases * adding mock tests * Context-Based Cancellation Refactoring * add centralised error propagation * solve lint error * modelling test cases in table format * graceful shut down * centralised config * solve lint error

shrouti1995

@anitgandhi @jcooperdo when you get time

anitgandhi · 2025-10-28T17:59:09Z

internal/conntrack/mock.go

+}
+
+// NewZoneMarkAggregator creates a mock aggregator for testing
+func NewZoneMarkAggregator() (*MockZoneMarkAggregator, error) {


this should be named NewMockZoneMarkAggregator()

anitgandhi · 2025-10-28T18:01:22Z

internal/conntrack/config.go

+}
+
+// LoadConfig loads conntrack configuration from environment variables
+func LoadConfig() *Config {


it's confusing to use env vars directly for config here, when the top-level exporter config is driven by CLI flags
we should be consistent and use CLI flags for everything.
this package (internal/conntrack) shouldn't really care about flags vs env vars actually, it should only care about its own Config type/struct and associated defaults

it should be left up to the caller to define how to change the config (env vars, flags, whatever)

anitgandhi · 2025-10-28T19:03:04Z

internal/conntrack/aggregator_linux_test.go

+				// Test concurrent snapshot access
+				done := make(chan bool, 10)
+				for i := 0; i < 10; i++ {
+					go func() {
+						snapshot := agg.Snapshot()
+						if snapshot == nil {
+							t.Error("Concurrent snapshot returned nil")
+						}
+						done <- true
+					}()
+				}
+
+				// Wait for all goroutines
+				for i := 0; i < 10; i++ {
+					<-done
+				}


could simplify this with a waitgroup

internal/ovsexporter/ovsexporter.go

anitgandhi · 2025-10-28T19:05:54Z

internal/ovsexporter/ovsexporter.go

-	cs []prometheus.Collector
+	mu               sync.Mutex
+	cs               []prometheus.Collector
+	conntrackEnabled bool


is there ever a situation where this is false but aggregator is non-nil?
i don't believe there would be , in which case this extra bool doesn't provide any value

Co-authored-by: Anit Gandhi <anitgandhi@gmail.com>

anitgandhi · 2025-10-29T14:45:52Z

internal/conntrack/config.go

+	}
+}
+
+// LoadConfig loads conntrack configuration from environment variables


remove this

anitgandhi · 2025-10-29T14:46:42Z

internal/conntrack/aggregator.go

aggregator.go would be a better name

anitgandhi · 2025-10-29T14:50:46Z

CONNTRACK_CONFIG.md

this feels out-dated at this point seeing as we don't have env vars anymore and also in this PR we didn't have the hardcoded consts either
i'd recommend getting rid of this doc and instead making the godoc in internal/conntrack/config.go more clear by adding these details

anitgandhi · 2025-10-29T14:52:39Z

internal/ovsexporter/ovsexporter.go

+		newDatapathCollector(c.Datapath.List),
 	}
+
+	// Create the aggregator


i don't see why we should put this in internal/ovsexporter at all
we should instead put the conntrack prometheus logic in internal/conntrack/exporter.go , and then just import it from main.go (if the additional exporter is enabled, which should be behind a new config flag bool in main.go)

my bad. added it in this way.

anitgandhi · 2025-10-29T16:04:17Z

internal/conntrack/aggregator_linux.go

+}
+
+// StopWithTimeout cancels listening and closes the connection with a configurable timeout.
+func (a *ZoneMarkAggregator) StopWithTimeout(timeout time.Duration) error {


does this need to be exported?

anitgandhi · 2025-10-29T16:04:36Z

internal/conntrack/aggregator_linux.go

+func (a *ZoneMarkAggregator) GetError() error {
+	// This is a non-blocking way to check if there are any errors
+	// The actual error handling happens in Stop()
+	return nil
+}


looks like a no-op, remove if it's not going to be useful

anitgandhi · 2025-10-30T16:56:57Z

internal/ovsexporter/ovsexporter.go

-			// Additional generic netlink family collectors can be added here.
-			newDatapathCollector(c.Datapath.List),
-		},
+	collectors := []prometheus.Collector{


can revert this

anitgandhi · 2025-10-30T16:57:29Z

internal/ovsexporter/test_helpers.go

this file should be in internal/conntrack right?

cleaned it up.

anitgandhi · 2025-10-30T17:53:54Z

cmd/openvswitch_exporter/main.go

+	case syscall.SIGHUP:
+		log.Printf("Received SIGHUP, reloading config...")
+		// TODO: Add config reload logic here
+		log.Printf("Config reloaded")
+		return


if we're not going to support this now (which i don't think we really need to), let's just not handle SIGHUP

cmd/openvswitch_exporter/main.go

anitgandhi · 2025-10-30T17:59:48Z

cmd/openvswitch_exporter/main.go

+		// TODO: Add config reload logic here
+		log.Printf("Config reloaded")
+		return
+	case syscall.SIGQUIT:


doesn't seem like we're actually treating SIGQUIT any differently than the other signals in practice

also, generally handling SIQQUIT gracefully is a bit weird. The Go runtime already does the "correct" thing , which is to dump a stack trace and exit.

let's remove handling for this, and just leave the standard SIGINT and SIGTERM for graceful termination

Co-authored-by: Anit Gandhi <anitgandhi@gmail.com>

anitgandhi · 2025-11-03T15:03:51Z

cmd/openvswitch_exporter/main.go

+	// Handle shutdown signals
+	sigChan := make(chan os.Signal, 1)
+	signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)

-	if err := http.ListenAndServe(*metricsAddr, mux); err != nil {
-		log.Fatalf("cannot start Open vSwitch exporter: %v", err)
+	// Start server in goroutine
+	go func() {
+		log.Printf("starting Open vSwitch exporter on %q", *metricsAddr)
+		if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+			log.Fatalf("cannot start Open vSwitch exporter: %v", err)
+		}
+	}()
+
+	// Wait for shutdown signal
+	sig := <-sigChan
+	log.Printf("Received signal %v, stopping gracefully...", sig)


Suggested change

// Handle shutdown signals

sigChan := make(chan os.Signal, 1)

signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)

if err := http.ListenAndServe(*metricsAddr, mux); err != nil {

log.Fatalf("cannot start Open vSwitch exporter: %v", err)

// Start server in goroutine

go func() {

log.Printf("starting Open vSwitch exporter on %q", *metricsAddr)

if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {

log.Fatalf("cannot start Open vSwitch exporter: %v", err)

}

}()

// Wait for shutdown signal

sig := <-sigChan

log.Printf("Received signal %v, stopping gracefully...", sig)

// Start server in goroutine

go func() {

log.Printf("starting Open vSwitch exporter on %q", *metricsAddr)

if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {

log.Fatalf("cannot start Open vSwitch exporter: %v", err)

}

}()

// Wait for shutdown signal

sigChan := make(chan os.Signal, 1)

signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)

sig := <-sigChan

GATEWAYS-4306: exporting metrics for conntrack per zone

16b12d1

shrouti1995 requested a review from Copilot August 13, 2025 10:39

Copilot AI reviewed Aug 13, 2025

View reviewed changes

shrouti1995 marked this pull request as draft August 13, 2025 10:51

shrouti1995 added 3 commits August 28, 2025 21:40

GATEWAYS-4306: scaling with even driven approach

f33d105

code cleanup

30b6d58

conntrack destroy rate limiting part 1

79cd030

jcooperdo reviewed Sep 29, 2025

View reviewed changes

refactor

f4b2a6b

shrouti1995 marked this pull request as ready for review October 10, 2025 06:34

shrouti1995 mentioned this pull request Oct 22, 2025

GATEWAYS-4306: exporting metrics for conntrack per zone digitalocean/go-openvswitch#137

Closed

shrouti1995 added 6 commits October 27, 2025 19:49

aggregating metrics for conntrack count per zone

b98892f

trying to fix the static check issue

901ac81

solve gha issue

a4ae2cd

* trying to fix the static check issue * trying to fix the static check issue

uncomment code

836243b

Adding test cases

ed8d851

* adding test cases * adding mock tests * Context-Based Cancellation Refactoring * add centralised error propagation * solve lint error

refining code

78c7134

* adding test cases * adding mock tests * Context-Based Cancellation Refactoring * add centralised error propagation * solve lint error * modelling test cases in table format * graceful shut down * centralised config * solve lint error

shrouti1995 requested a review from jcooperdo October 28, 2025 15:20

shrouti1995 self-assigned this Oct 28, 2025

shrouti1995 commented Oct 28, 2025

View reviewed changes

anitgandhi reviewed Oct 28, 2025

View reviewed changes

shrouti1995 and others added 2 commits October 29, 2025 15:57

Update internal/ovsexporter/ovsexporter.go

71644e4

Co-authored-by: Anit Gandhi <anitgandhi@gmail.com>

code review suggestions

b21f8d5

anitgandhi reviewed Oct 29, 2025

View reviewed changes

shrouti1995 added 4 commits October 29, 2025 22:34

code review suggestions

a33ec5d

fixing test cases

8d4ce2d

fixing test cases

f6ea39e

removing unnecessary files

f67d1a8

shrouti1995 requested a review from anitgandhi October 30, 2025 07:34

anitgandhi reviewed Oct 30, 2025

View reviewed changes

shrouti1995 and others added 2 commits November 3, 2025 18:40

Apply suggestions from code review

6f7a802

Co-authored-by: Anit Gandhi <anitgandhi@gmail.com>

review comments addressed

4883f1e

shrouti1995 requested a review from anitgandhi November 3, 2025 13:41

anitgandhi reviewed Nov 25, 2025

View reviewed changes

Conversation

shrouti1995 commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrouti1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shrouti1995 commented Aug 13, 2025 •

edited

Loading