Skip to content

Commit

Permalink
acceptance: ensure the vol directory is only deleted after shutdown
Browse files Browse the repository at this point in the history
Prior to this patch, the docker volume directory was deleted before
the cluster nodes were shut down, which could cause them to crash
abnormally and have the Assert method return an unexpected failure,
even when the test would otherwise succeed.

This patch fixes it.

Release justification: non-production code changes

Release note: None
  • Loading branch information
knz committed Aug 25, 2022
1 parent 64cda3d commit a0beb1e
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 3 deletions.
1 change: 1 addition & 0 deletions pkg/acceptance/cluster/dockercluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,7 @@ func (l *DockerCluster) Cleanup(ctx context.Context, preserveLogs bool) {
}
for _, v := range volumes {
if preserveLogs && v.Name() == "logs" {
log.Infof(ctx, "preserving log directory: %s", l.volumesDir)
continue
}
if err := os.RemoveAll(filepath.Join(l.volumesDir, v.Name())); err != nil {
Expand Down
22 changes: 19 additions & 3 deletions pkg/acceptance/util_docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/build/bazel"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/containerd/containerd/platforms"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
Expand Down Expand Up @@ -121,18 +122,33 @@ func testDocker(
cfg.Nodes = append(cfg.Nodes, cluster.NodeConfig{Stores: []cluster.StoreConfig{{}}})
}
l := StartCluster(ctx, t, cfg).(*cluster.DockerCluster)
defer l.AssertAndStop(ctx, t)

var preserveLogs bool
defer func() {
// Check the final health of the cluster nodes and
// stop the cluster after that.
l.AssertAndStop(ctx, t)

// Note: we must be careful to clean up the volumes *after*
// the cluster has been shut down (in the `AssertAndStop` call).
// Otherwise, the directory removal will cause the cluster nodes
// to crash and report abnormal termination, even when the test
// succeeds otherwise.
log.Infof(ctx, "cleaning up docker volume")
l.Cleanup(ctx, preserveLogs)
}()

if len(l.Nodes) > 0 {
containerConfig.Env = append(containerConfig.Env, "PGHOST="+l.Hostname(0))
}

log.Infof(ctx, "starting one-shot container")
err = l.OneShot(
ctx, acceptanceImage, types.ImagePullOptions{}, containerConfig, hostConfig,
platforms.DefaultSpec(), "docker-"+name,
)
preserveLogs := err != nil
l.Cleanup(ctx, preserveLogs)
log.Infof(ctx, "one-shot container terminated: %v", err)
preserveLogs = err != nil
})
return err
}
Expand Down

0 comments on commit a0beb1e

Please sign in to comment.