Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support rr_dns lookup host #113

Merged
merged 12 commits into from
Jul 16, 2024
Merged

feat: support rr_dns lookup host #113

merged 12 commits into from
Jul 16, 2024

Conversation

jiuker
Copy link
Contributor

@jiuker jiuker commented Jul 7, 2024

feat: support rr_dns lookup host

@jiuker jiuker marked this pull request as draft July 7, 2024 15:45
@jiuker jiuker requested a review from harshavardhana July 7, 2024 16:14
@jiuker jiuker changed the title feat: support rr_dns reload after a time feat: support rr_dns reload when health is not ok Jul 7, 2024
@jiuker jiuker marked this pull request as ready for review July 7, 2024 16:18
@jiuker jiuker requested review from klauspost and ramondeklein and removed request for ramondeklein July 7, 2024 16:19
ramondeklein

This comment was marked as off-topic.

@jiuker jiuker requested a review from ramondeklein July 9, 2024 01:03
@ramondeklein

This comment was marked as off-topic.

klauspost

This comment was marked as off-topic.

main.go Outdated Show resolved Hide resolved
@jiuker jiuker requested a review from klauspost July 9, 2024 09:43
@ramondeklein

This comment was marked as off-topic.

@jiuker

This comment was marked as off-topic.

@jiuker jiuker changed the title feat: support rr_dns reload when health is not ok fix: support rr_dns reload when health is not ok Jul 9, 2024
@ramondeklein

This comment was marked as off-topic.

@jiuker

This comment was marked as off-topic.

@ramondeklein

This comment was marked as off-topic.

@harshavardhana
Copy link
Member

@jiuker what is needed here is not a custom resolver but a way to get inputs for list of backends that you need to monitor. Today customers specify

sidekick [flags] http://minio-cluster{1...4}.example.com/

However the plan is to support a DNS that provides a CNAME list of hosts or A record of IPs like this.

dig cloud.min.io

; <<>> DiG 9.18.24-0ubuntu0.22.04.1-Ubuntu <<>> cloud.min.io
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 37124
;; flags: qr rd ra; QUERY: 1, ANSWER: 8, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 65494
;; QUESTION SECTION:
;cloud.min.io.			IN	A

;; ANSWER SECTION:
cloud.min.io.		287	IN	A	15.15.15.4
cloud.min.io.		287	IN	A	15.15.15.5
cloud.min.io.		287	IN	A	15.15.15.3
cloud.min.io.		287	IN	A	15.15.15.8
cloud.min.io.		287	IN	A	15.15.15.7
cloud.min.io.		287	IN	A	15.15.15.2
cloud.min.io.		287	IN	A	15.15.15.6
cloud.min.io.		287	IN	A	15.15.15.1

;; Query time: 0 msec
;; SERVER: 127.0.0.53#53(127.0.0.53) (UDP)
;; WHEN: Tue Jul 09 10:43:20 PDT 2024
;; MSG SIZE  rcvd: 169

Now before starting sidekick you must do https://pkg.go.dev/net#LookupHost to get the list of addrs and then use those addrs to register backends.

So ideally a customer would specify just this

./sidekick [flags] http://cloud.min.io

Now we should support SIGHUP to ensure we can do a fresh lookup of cloud.min.io in-case anything changes to support adding new "backends".

This is for situations like customer added a new pool but they are simply updating their DNS entries but not sidekick command line configuration, sidekick simply automatically starts regarding new pool backends as soon as SIGHUP is called.

This PR right now is not doing what we want.

@jiuker jiuker marked this pull request as draft July 10, 2024 03:44
@jiuker jiuker marked this pull request as draft July 10, 2024 03:44
refactor
@jiuker jiuker marked this pull request as ready for review July 10, 2024 07:46
lint
@jiuker
Copy link
Contributor Author

jiuker commented Jul 10, 2024

@ramondeklein @klauspost review this again please.

@jiuker
Copy link
Contributor Author

jiuker commented Jul 10, 2024

@jiuker what is needed here is not a custom resolver but a way to get inputs for list of backends that you need to monitor. Today customers specify

sidekick [flags] http://minio-cluster{1...4}.example.com/

However the plan is to support a DNS that provides a CNAME list of hosts or A record of IPs like this.

dig cloud.min.io

; <<>> DiG 9.18.24-0ubuntu0.22.04.1-Ubuntu <<>> cloud.min.io
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 37124
;; flags: qr rd ra; QUERY: 1, ANSWER: 8, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 65494
;; QUESTION SECTION:
;cloud.min.io.			IN	A

;; ANSWER SECTION:
cloud.min.io.		287	IN	A	15.15.15.4
cloud.min.io.		287	IN	A	15.15.15.5
cloud.min.io.		287	IN	A	15.15.15.3
cloud.min.io.		287	IN	A	15.15.15.8
cloud.min.io.		287	IN	A	15.15.15.7
cloud.min.io.		287	IN	A	15.15.15.2
cloud.min.io.		287	IN	A	15.15.15.6
cloud.min.io.		287	IN	A	15.15.15.1

;; Query time: 0 msec
;; SERVER: 127.0.0.53#53(127.0.0.53) (UDP)
;; WHEN: Tue Jul 09 10:43:20 PDT 2024
;; MSG SIZE  rcvd: 169

Now before starting sidekick you must do https://pkg.go.dev/net#LookupHost to get the list of addrs and then use those addrs to register backends.

So ideally a customer would specify just this

./sidekick [flags] http://cloud.min.io

Now we should support SIGHUP to ensure we can do a fresh lookup of cloud.min.io in-case anything changes to support adding new "backends".

This is for situations like customer added a new pool but they are simply updating their DNS entries but not sidekick command line configuration, sidekick simply automatically starts regarding new pool backends as soon as SIGHUP is called.

This PR right now is not doing what we want.

┌──────┬───────────────────┬────────┬───────┬──────────┬─────┬─────┬────────────────┬───────────────┬─────────────┬─────────────┐
│ SITE │ HOST              │ STATUS │ CALLS │ FAILURES │ Rx  │ Tx  │ TOTAL DOWNTIME │ LAST DOWNTIME │ MIN LATENCY │ MAX LATENCY │
│ 1st  │ http://15.15.15.8 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.2 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.6 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.7 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.1 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.4 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.3 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │
│ 1st  │ http://15.15.15.5 │ DOWN   │ 0     │ 0        │ 0 B │ 0 B │ 0s             │ 0s            │ 0s          │ 0s          │

@harshavardhana

metrics.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@ramondeklein
Copy link
Contributor

Before I review the code, I want to understand the requested functionality. As I understand there are two ways to configure sidekick:

  1. The existing way using the ellipsis notation, such as sidekick [flags] http://minio-cluster{1...4}.example.com/.
  2. The new method where a single DNS name resolves to multiple IP addresses, such as sidekick [flags] http://minio-cluster.example.com/ (hostname minio-cluster.example.com can resolve to multiple IP addresses). This method could be used to support a round robin DNS (I guess that's meant with "rr_dns") that will serve a list of IP addresses in "random" order to ensure that all servers are handling requests.

I think we need a little different implementation to support this feature. There should be one go-routine (that runs every 5 minutes or when forced using SIGHUP) that determines all the IP addresses of the specified endpoints, so:

  1. minio-cluster{1...4}.example.com will probably result in 4 IP addresses (all having a unique hostname). Sidekick will create 4 back-ends.
  2. minio-cluster.example.com will result in X IP addresses (let's assume 8 while all using the same hostname). Sidekick will create 8 backends.

I'm not sure if we should also support round-robin DNS look-up for the ellipsis notation or always use the first IP that is presented (@harshavardhana what do you think?)

Sidekick will create a custom http.Client per back-end that always dials the IP address that is associated with that backend and uses the hostname for that backend (in case of TLS) to verify the server certificate. Each time it modifies this list, then it will swap out the map atomically, so no locking is needed. Health checks and normal behavior won't be altered.

apply suggestion
@jiuker jiuker requested a review from klauspost July 10, 2024 14:32
@harshavardhana
Copy link
Member

I'm not sure if we should also support round-robin DNS look-up for the ellipsis notation or always use the first IP that is presented (@harshavardhana what do you think?)

If you have a single arg RR-DNS based, multiple host input is supported; if you specify more than one arg either via ellipses or multiple separate hosts, we do not invoke this code.

@harshavardhana
Copy link
Member

@jiuker there are some UI changes we need will keep you posted

add flags
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
jiuker added 3 commits July 11, 2024 16:54
apply suggestion
apply suggestion
apply suggestion
@jiuker jiuker requested a review from ramondeklein July 11, 2024 09:13
jiuker added 2 commits July 11, 2024 23:18
@jiuker
Copy link
Contributor Author

jiuker commented Jul 11, 2024

@ramondeklein review again plz

@jiuker jiuker changed the title fix: support rr_dns lookup host feat: support rr_dns lookup host Jul 11, 2024
Copy link
Contributor

@ramondeklein ramondeklein left a comment

Choose a reason for hiding this comment

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

One open comment, but I'm fine keeping the CLI parameter.

main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
jiuker added 3 commits July 16, 2024 09:43
log
log
add opts
@jiuker jiuker requested a review from harshavardhana July 16, 2024 01:55
@harshavardhana harshavardhana requested a review from poornas July 16, 2024 03:25
@harshavardhana harshavardhana merged commit dc67c00 into minio:master Jul 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants