Skip to content

chore(healthcheck): fix DERP test flakes #7211

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 20, 2023
Merged

chore(healthcheck): fix DERP test flakes #7211

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
699348d
chore(healthcheck): fix DERP test flakes
coadler Apr 19, 2023
0cab353
fixup! chore(healthcheck): fix DERP test flakes
coadler Apr 19, 2023
6f54a91
fixup! chore(healthcheck): fix DERP test flakes
coadler Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions coderd/apidoc/docs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions coderd/apidoc/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

97 changes: 66 additions & 31 deletions coderd/healthcheck/derp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"sync/atomic"
"time"

"golang.org/x/sync/errgroup"
"golang.org/x/xerrors"
"tailscale.com/derp"
"tailscale.com/derp/derphttp"
Expand All @@ -21,6 +20,8 @@ import (
"tailscale.com/tailcfg"
"tailscale.com/types/key"
tslogger "tailscale.com/types/logger"

"github.com/coder/coder/coderd/util/ptr"
)

type DERPReport struct {
Expand Down Expand Up @@ -48,11 +49,12 @@ type DERPNodeReport struct {
Healthy bool `json:"healthy"`
Node *tailcfg.DERPNode `json:"node"`

CanExchangeMessages bool `json:"can_exchange_messages"`
RoundTripPing time.Duration `json:"round_trip_ping"`
UsesWebsocket bool `json:"uses_websocket"`
ClientLogs [][]string `json:"client_logs"`
ClientErrs [][]error `json:"client_errs"`
ServerInfo derp.ServerInfoMessage `json:"node_info"`
CanExchangeMessages bool `json:"can_exchange_messages"`
RoundTripPing time.Duration `json:"round_trip_ping"`
UsesWebsocket bool `json:"uses_websocket"`
ClientLogs [][]string `json:"client_logs"`
ClientErrs [][]error `json:"client_errs"`

STUN DERPStunReport `json:"stun"`
}
Expand Down Expand Up @@ -161,8 +163,19 @@ func (r *DERPNodeReport) Run(ctx context.Context) {
r.ClientLogs = [][]string{}
r.ClientErrs = [][]error{}

r.doExchangeMessage(ctx)
r.doSTUNTest(ctx)
wg := &sync.WaitGroup{}

wg.Add(2)
go func() {
defer wg.Done()
r.doExchangeMessage(ctx)
}()
go func() {
defer wg.Done()
r.doSTUNTest(ctx)
}()

wg.Wait()

// We can't exchange messages with the node,
if (!r.CanExchangeMessages && !r.Node.STUNOnly) ||
Expand All @@ -181,60 +194,78 @@ func (r *DERPNodeReport) doExchangeMessage(ctx context.Context) {
return
}

var peerKey atomic.Pointer[key.NodePublic]
eg, ctx := errgroup.WithContext(ctx)
var (
peerKey atomic.Pointer[key.NodePublic]
lastSent atomic.Pointer[time.Time]
)
ctx, cancel := context.WithCancel(ctx)
defer cancel()
wg := &sync.WaitGroup{}

receive, receiveID, err := r.derpClient(ctx, r.derpURL())
if err != nil {
return
}
defer receive.Close()

eg.Go(func() error {
wg.Add(2)
go func() {
defer wg.Done()
defer receive.Close()

pkt, err := r.recvData(receive)
if err != nil {
r.writeClientErr(receiveID, xerrors.Errorf("recv derp message: %w", err))
return err
return
}

if *peerKey.Load() != pkt.Source {
r.writeClientErr(receiveID, xerrors.Errorf("received pkt from unknown peer: %s", pkt.Source.ShortString()))
return err
return
}

t, err := time.Parse(time.RFC3339Nano, string(pkt.Data))
if err != nil {
r.writeClientErr(receiveID, xerrors.Errorf("parse time from peer: %w", err))
return err
}
t := lastSent.Load()

r.mu.Lock()
r.CanExchangeMessages = true
r.RoundTripPing = time.Since(t)
r.RoundTripPing = time.Since(*t)
Copy link
Member

Choose a reason for hiding this comment

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

Since the send loop updates last sent every second, do we know that we're actually benchmarking the correct ping/ping?

r.mu.Unlock()
return nil
})
eg.Go(func() error {

cancel()
}()
go func() {
defer wg.Done()
send, sendID, err := r.derpClient(ctx, r.derpURL())
if err != nil {
return err
return
}
defer send.Close()

key := send.SelfPublicKey()
peerKey.Store(&key)

err = send.Send(receive.SelfPublicKey(), []byte(time.Now().Format(time.RFC3339Nano)))
if err != nil {
r.writeClientErr(sendID, xerrors.Errorf("send derp message: %w", err))
return err
ticker := time.NewTicker(time.Second)
defer ticker.Stop()

iter := 0
for {
lastSent.Store(ptr.Ref(time.Now()))
err = send.Send(receive.SelfPublicKey(), []byte(fmt.Sprintf("%d", iter)))
if err != nil {
r.writeClientErr(sendID, xerrors.Errorf("send derp message: %w", err))
return
}
iter++

select {
case <-ctx.Done():
return
case <-ticker.C:
}
}
return nil
})
}()

_ = eg.Wait()
wg.Wait()
}

func (r *DERPNodeReport) doSTUNTest(ctx context.Context) {
Expand Down Expand Up @@ -378,7 +409,7 @@ func (r *DERPNodeReport) derpClient(ctx context.Context, derpURL *url.URL) (*der
return client, id, nil
}

func (*DERPNodeReport) recvData(client *derphttp.Client) (derp.ReceivedPacket, error) {
func (r *DERPNodeReport) recvData(client *derphttp.Client) (derp.ReceivedPacket, error) {
for {
msg, err := client.Recv()
if err != nil {
Expand All @@ -388,6 +419,10 @@ func (*DERPNodeReport) recvData(client *derphttp.Client) (derp.ReceivedPacket, e
switch msg := msg.(type) {
case derp.ReceivedPacket:
return msg, nil
case derp.ServerInfoMessage:
r.mu.Lock()
r.ServerInfo = msg
r.mu.Unlock()
default:
// Drop all others!
}
Expand Down
20 changes: 11 additions & 9 deletions coderd/healthcheck/derp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"github.com/coder/coder/coderd/healthcheck"
"github.com/coder/coder/tailnet"
"github.com/coder/coder/testutil"
)

//nolint:tparallel
Expand Down Expand Up @@ -66,8 +67,7 @@ func TestDERP(t *testing.T) {
for _, node := range region.NodeReports {
assert.True(t, node.Healthy)
assert.True(t, node.CanExchangeMessages)
// TODO: test this without serializing time.Time over the wire.
// assert.Positive(t, node.RoundTripPing)
assert.Positive(t, node.RoundTripPing)
assert.Len(t, node.ClientLogs, 2)
assert.Len(t, node.ClientLogs[0], 1)
assert.Len(t, node.ClientErrs[0], 0)
Expand All @@ -81,9 +81,13 @@ func TestDERP(t *testing.T) {
}
})

t.Run("OK/Tailscale/Dallas", func(t *testing.T) {
t.Run("Tailscale/Dallas/OK", func(t *testing.T) {
t.Parallel()

if testutil.InCI() {
t.Skip("This test depends on reaching out over the network to Tailscale servers, which is inherently flaky.")
}

derpSrv := derp.NewServer(key.NewNode(), func(format string, args ...any) { t.Logf(format, args...) })
defer derpSrv.Close()
srv := httptest.NewServer(derphttp.Handler(derpSrv))
Expand All @@ -107,8 +111,7 @@ func TestDERP(t *testing.T) {
for _, node := range region.NodeReports {
assert.True(t, node.Healthy)
assert.True(t, node.CanExchangeMessages)
// TODO: test this without serializing time.Time over the wire.
// assert.Positive(t, node.RoundTripPing)
assert.Positive(t, node.RoundTripPing)
assert.Len(t, node.ClientLogs, 2)
assert.Len(t, node.ClientLogs[0], 1)
assert.Len(t, node.ClientErrs[0], 0)
Expand Down Expand Up @@ -171,13 +174,12 @@ func TestDERP(t *testing.T) {
for _, node := range region.NodeReports {
assert.False(t, node.Healthy)
assert.True(t, node.CanExchangeMessages)
// TODO: test this without serializing time.Time over the wire.
// assert.Positive(t, node.RoundTripPing)
assert.Positive(t, node.RoundTripPing)
assert.Len(t, node.ClientLogs, 2)
assert.Len(t, node.ClientLogs[0], 3)
assert.Len(t, node.ClientLogs[1], 3)
assert.Len(t, node.ClientErrs, 2)
assert.Len(t, node.ClientErrs[0], 1)
assert.Len(t, node.ClientErrs[0], 1) // this
assert.Len(t, node.ClientErrs[1], 1)
assert.True(t, node.UsesWebsocket)

Expand All @@ -188,7 +190,7 @@ func TestDERP(t *testing.T) {
}
})

t.Run("OK/STUNOnly", func(t *testing.T) {
t.Run("STUNOnly/OK", func(t *testing.T) {
t.Parallel()

var (
Expand Down
8 changes: 8 additions & 0 deletions docs/api/debug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ curl -X GET http://coder-server:8080/api/v2/debug/health \
"stunport": 0,
"stuntestIP": "string"
},
"node_info": {
"tokenBucketBytesBurst": 0,
"tokenBucketBytesPerSecond": 0
},
"round_trip_ping": 0,
"stun": {
"canSTUN": true,
Expand Down Expand Up @@ -158,6 +162,10 @@ curl -X GET http://coder-server:8080/api/v2/debug/health \
"stunport": 0,
"stuntestIP": "string"
},
"node_info": {
"tokenBucketBytesBurst": 0,
"tokenBucketBytesPerSecond": 0
},
"round_trip_ping": 0,
"stun": {
"canSTUN": true,
Expand Down
Loading