Skip to content

Commit

Permalink
etcd: fix data race in migration tests
Browse files Browse the repository at this point in the history
  • Loading branch information
Andrew Lytvynov authored and awly committed Sep 15, 2020
1 parent ded07a1 commit 908bf4a
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 5 deletions.
14 changes: 9 additions & 5 deletions lib/backend/etcdbk/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ type EtcdBackend struct {
cancel context.CancelFunc
watchStarted context.Context
signalWatchStart context.CancelFunc
watchDone chan struct{}
}

// Config represents JSON config for etcd backend
Expand Down Expand Up @@ -213,6 +214,7 @@ func New(ctx context.Context, params backend.Params) (*EtcdBackend, error) {
ctx: closeCtx,
watchStarted: watchStarted,
signalWatchStart: signalWatchStart,
watchDone: make(chan struct{}),
buf: buf,
}
if err = b.reconnect(); err != nil {
Expand Down Expand Up @@ -331,6 +333,8 @@ func (b *EtcdBackend) asyncWatch() {
}

func (b *EtcdBackend) watchEvents() error {
defer close(b.watchDone)

start:
eventsC := b.client.Watch(b.ctx, b.cfg.Key, clientv3.WithPrefix())
b.signalWatchStart()
Expand Down Expand Up @@ -688,26 +692,24 @@ func (b *EtcdBackend) syncLegacyPrefix(ctx context.Context) error {
b.Infof("Migrating Teleport etcd data from legacy prefix %q to configured prefix %q", legacyDefaultPrefix, b.cfg.Key)
defer b.Infof("Teleport etcd data migration complete")

var errs []error

// Now we know that legacy prefix has some data newer than the configured
// prefix. Migrate it over to configured prefix.
//
// First, let's backup the data under configured prefix, in case the
// migration kicked in by mistake.
backupPrefix := b.backupPrefix(b.cfg.Key)
b.Debugf("Backup everything under %q to %q", b.cfg.Key, backupPrefix)
b.Infof("Backup everything under %q to %q", b.cfg.Key, backupPrefix)
for _, kv := range prefixData.Kvs {
// Replace the prefix.
key := backupPrefix + strings.TrimPrefix(string(kv.Key), b.cfg.Key)
b.Debugf("Copying %q -> %q", kv.Key, key)
if _, err := b.client.Put(ctx, key, string(kv.Value)); err != nil {
errs = append(errs, trace.WrapWithMessage(err, "failed copying %q to %q: %v", kv.Key, key, err))
return trace.WrapWithMessage(err, "failed backing up %q to %q: %v", kv.Key, key, err)
}
}

// Now delete existing prefix data.
b.Debugf("Deleting everything under %q", b.cfg.Key)
b.Infof("Deleting everything under %q", b.cfg.Key)
deletePrefix := b.cfg.Key
// Make sure the prefix ends with a '/', so that we don't delete the backup
// created above or any other unrelated data.
Expand All @@ -718,6 +720,8 @@ func (b *EtcdBackend) syncLegacyPrefix(ctx context.Context) error {
return trace.Wrap(err)
}

b.Infof("Copying everything under %q to %q", legacyDefaultPrefix, b.cfg.Key)
var errs []error
// Finally, copy over all the data from the legacy prefix to the new one.
for _, kv := range legacyData.Kvs {
// Replace the prefix.
Expand Down
5 changes: 5 additions & 0 deletions lib/backend/etcdbk/etcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ func (s *EtcdSuite) assertKV(ctx context.Context, c *check.C, key, val string) {
}

func (s *EtcdSuite) TestSyncLegacyPrefix(c *check.C) {
// Stop the watch goroutine to allow us to modify s.bk.cfg.Key without data
// races.
s.bk.cancel()
<-s.bk.watchDone

ctx := context.Background()
s.bk.cfg.Key = customPrefix1
c.Assert(s.bk.cfg.Validate(), check.IsNil)
Expand Down

0 comments on commit 908bf4a

Please sign in to comment.