Dns package #595

maurafortino · 2025-01-09T15:47:30Z

What's Included:

moved SRVRecordDialer to its own packaged
removed any instances of nonstandard go libraries from package
added "load balancing" for weighted SRV records
created tests for NewSRVRecordDialer

johnabass · 2025-01-15T17:03:34Z

internal/dns/srvRecordDialer_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewSRVRecordDialer(t *testing.T) {


Consider using github.com/stretchr/testify/suite for tests. That package lets you organize your tests better, moving setup/teardown code into separate methods, etc. It provides a lot of the same structure as frameworks like TestNG for Java.

johnabass · 2025-01-15T17:04:59Z

internal/dns/srvRecordDialer.go

+	LookupSRV(ctx context.Context, service, proto, name string) (string, []*net.SRV, error)
+}
+
+func NewSRVRecordDialer(fqdns []string, sortBy string, resolver Resolver) (http.RoundTripper, error) {


You probably want to use the functional options pattern here. This function is a primary entry point for this package and it's signature will likely need to change as this package evolves over time. Using functional options allows clients to be backwards compatible and resilient in the face of new features.

johnabass · 2025-01-15T17:06:36Z

internal/dns/srvRecordDialer.go

+
+	//TODO: add retry logic if we receive conn error? or just move to next one?
+	switch d.sortBy {
+	case "weight":


Instead of using constants whose value you switch on, a more extensible way would be to create a Sorter strategy that did the sorting. Then, instead of passing sortBy, you'd have someone pass this code a Sorter. This package can provide (2) implementations of Sorter: one for weight and one for priority.

I think this sorting logic is going to be replace soon with actual sampling/load-balancing code, right @maurafortino ?

@denopink there is sampling/load-balancing code in the getAddrByWeight function nested inside the switch statement: https://github.com/xmidt-org/caduceus/blob/4a72c1e544ab1c9c609872b1f6624bd92269f4e5/internal/dns/srvRecordDialer.go#L111C36-L112C21

Spoke with John and we're going to be changing from sorter strategy to chooser strategy

I may have missed the conversation behind the algo being used in getAddrByWeight.

I recommend adding a short description of the expected behavior of getAddrByWeight.

I may have missed the conversation behind the algo being used in getAddrByWeight.

I recommend adding a short description of the expected behavior of getAddrByWeight.

@denopink I don't think we had a discussion on which algo to use. i'm just using weighted random for now. i can set up a time though to discuss if needed.

got it, just letting you know I don't think getAddrByWeight is actually implementing “weighted random” load balancing. weighted random load balancing usually means routing requests evenly across healthy targets in a random order.

No need for a meeting (imo) if you aren't focusing on the load balancing specifics just yet, I just wanted to point it out in case I was missing something. Sounds like it's a place holder while you're working on this. thx for clarifying 🙂

denopink · 2025-01-15T17:43:25Z

internal/dns/srvRecordDialer.go

+		d.srvs = append(d.srvs, addrs...)
+	}
+
+	// TODO: ask wes/john whether 1 or more net.LookupSRV error should trigger an error from NewSRVRecordDailer


When a customer registers a set of svr records (via a single registration call to webpa) and one of the src record fails, I would think we wouldn't want the srv record to silently fail... otherwise our customer may not realize there's an issue with one of their srv records.

What are your thoughts @schmidtw @johnabass?

denopink · 2025-01-15T17:47:52Z

internal/dns/srvRecordDialer.go

+func getAddrByWeight(srvs []*net.SRV) (*net.SRV, int, error) {
+	if len(srvs) == 0 {
+		return nil, -1, errors.New("no SRV records available")
+	}
+
+	totalWeight := 0
+	for _, srv := range srvs {
+		totalWeight += int(srv.Weight)
+	}
+
+	if totalWeight == 0 {
+		totalWeight = len(srvs)
+	}
+
+	randWeight := rand.Intn(totalWeight)
+	currentWeight := 0
+
+	for i, srv := range srvs {
+		currentWeight += int(srv.Weight)
+		if randWeight < currentWeight {
+			return srv, i, nil
+		}
+	}
+
+	return nil, -1, errors.New("failed to choose an SRV record by weight")
+}


best to convert these errors into package errors for testing and general error handling

denopink · 2025-01-15T17:48:25Z

internal/dns/srvRecordDialer.go

+			return conn, errs
+		}
+	default:
+		return nil, fmt.Errorf("unknown loadBalancingScheme type: %s", d.sortBy)


best to convert this to a package error for testing and general error handling

denopink · 2025-01-15T17:48:50Z

internal/dns/srvRecordDialer.go

+		_, addrs, err := resolver.LookupSRV(context.Background(), "", "", fqdn)
+		if err != nil {
+			errs = errors.Join(errs,
+				fmt.Errorf("srv lookup failure: `%s`", fqdn),


best to convert this to a package error for testing and general error handling

can you clarify what you mean by package error?

package level variable with an assigned error

is that common practice for us? i don't think i've ever seen us make error a package level variable. do you have an example? maybe i'm not understanding fully.

https://github.com/xmidt-org/ancla/blob/cf3eaffa4279a79ce69f41da9432e5266dbe1507/chrysom/basicClient.go#L27-L36

oooh yeah was def not understanding fully. thx!

denopink · 2025-01-15T17:48:56Z

internal/dns/srvRecordDialer.go

+
+	// TODO: ask wes/john whether 1 or more net.LookupSRV error should trigger an error from NewSRVRecordDailer
+	if len(d.srvs) == 0 {
+		return nil, errors.Join(fmt.Errorf("expected atleast 1 srv record from fqdn list `%v`", d.fqdns), errs)


best to convert this to a package error for testing and general error handling

denopink

Left a few suggestions, great progress so far! 🍻

internal/dns/srvRecordDialer.go

internal/dns/srvRecordDialer_test.go

denopink

more suggestions! 😄

maurafortino · 2025-01-30T15:06:58Z

internal/sink/srvRecordDialer.go

moved to internal/dns/srvRecordDialer.go

maurafortino · 2025-01-30T15:13:16Z

internal/dns/chooser.go

@denopink @schmidtw I would like discuss how we are going to be choosing between weight and priority. @johnabass mentioned we normally would choose one or the other (he wasn't sure exactly which one is chosen but for this example we'll go with weight) and then if there are two with the same weight we look to see which has the higher priority. And then vice versa if we use priority. Is this how we want to do that or are we thinking of distributing events differently?

maurafortino · 2025-01-30T15:14:50Z

internal/README.md

ignore this file - will be removing.

maurafortino added 2 commits January 9, 2025 08:31

created tests for srvRecordDialer

19b7c21

Merge branch 'denopink/feat/rewrite' into dns-package

60125b7

maurafortino added the caduceus-refactor label Jan 9, 2025

maurafortino requested a review from denopink January 9, 2025 15:47

maurafortino self-assigned this Jan 9, 2025

updated srvRecordDialer to include 'load balancing' and updated tests

4a72c1e

maurafortino marked this pull request as draft January 10, 2025 18:41

maurafortino marked this pull request as ready for review January 13, 2025 16:28

maurafortino requested review from schmidtw and johnabass January 13, 2025 20:47

johnabass reviewed Jan 15, 2025

View reviewed changes

denopink requested review from denopink and removed request for denopink January 15, 2025 17:37

denopink reviewed Jan 15, 2025

View reviewed changes

denopink requested changes Jan 15, 2025

View reviewed changes

denopink reviewed Jan 15, 2025

View reviewed changes

internal/dns/srvRecordDialer.go Show resolved Hide resolved

denopink reviewed Jan 15, 2025

View reviewed changes

internal/dns/srvRecordDialer_test.go Show resolved Hide resolved

denopink requested changes Jan 15, 2025

View reviewed changes

maurafortino added 3 commits January 17, 2025 12:48

PR requested changes

00ff231

updated NewSRVRecordDialer to take in opts

b55ad80

updated tests to suites

7435056

maurafortino commented Jan 30, 2025

View reviewed changes

internal/sink/srvRecordDialer.go Outdated

Copy link

Contributor Author

maurafortino Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to internal/dns/srvRecordDialer.go

removed commented out tests

57a0860

maurafortino commented Jan 30, 2025

View reviewed changes

removed unnecessary comment

b94c6c3

maurafortino commented Jan 30, 2025

View reviewed changes

internal/README.md Outdated

Copy link

Contributor Author

maurafortino Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore this file - will be removing.

maurafortino requested review from johnabass and denopink January 30, 2025 15:15

maurafortino added 2 commits January 30, 2025 10:15

Delete internal/dns/chooser_test.go

4d2e38a

Delete internal/README.md

3d17dfd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dns package #595

Dns package #595

maurafortino commented Jan 9, 2025 •

edited

Loading

johnabass Jan 15, 2025

johnabass Jan 15, 2025

johnabass Jan 15, 2025

denopink Jan 15, 2025

maurafortino Jan 15, 2025

maurafortino Jan 15, 2025

denopink Jan 15, 2025

maurafortino Jan 15, 2025

denopink Jan 15, 2025 •

edited

Loading

denopink Jan 15, 2025 •

edited

Loading

denopink Jan 15, 2025

denopink Jan 15, 2025

denopink Jan 15, 2025

maurafortino Jan 15, 2025

denopink Jan 15, 2025

maurafortino Jan 15, 2025

denopink Jan 15, 2025

maurafortino Jan 15, 2025

denopink Jan 15, 2025

denopink left a comment

denopink left a comment

maurafortino Jan 30, 2025

maurafortino Jan 30, 2025

maurafortino Jan 30, 2025

Dns package #595

Are you sure you want to change the base?

Dns package #595

Conversation

maurafortino commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denopink Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

denopink Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denopink left a comment

Choose a reason for hiding this comment

denopink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurafortino commented Jan 9, 2025 •

edited

Loading

denopink Jan 15, 2025 •

edited

Loading

denopink Jan 15, 2025 •

edited

Loading