fix: ensure volumes can be rebound to new containers (#136)

* chore: wip adding test to verify volume re-use

* chore: small refactor in volume creation

* chore: sanitize docker resource name

* chore: add version negotiation

Author: chatton

framework/docker/container/job.go

@@ -210,7 +210,7 @@ func (job *Job) Start(ctx context.Context, cmd []string, opts Options) (*Contain
 	}
 
 	var (
-		containerName = internal.SanitizeContainerName(job.testName + "-" + random.LowerCaseLetterString(6))
+		containerName = internal.SanitizeDockerResourceName(job.testName + "-" + random.LowerCaseLetterString(6))
 		hostName      = internal.CondenseHostName(containerName)
 		logger        = job.log.With(
 			zap.String("command", strings.Join(cmd, " ")), //nolint:gosec // testing only so safe to expose credentials in cli

framework/docker/container/node.go

@@ -3,6 +3,7 @@ package container
 import (
 	"context"
 	"fmt"
+	"github.com/celestiaorg/tastora/framework/docker/internal"
 
 	"github.com/celestiaorg/tastora/framework/docker/consts"
 	"github.com/celestiaorg/tastora/framework/docker/file"
@@ -158,28 +159,26 @@ func (n *Node) WriteFile(ctx context.Context, relPath string, content []byte) er
 	return fw.WriteFile(ctx, n.VolumeName, relPath, content)
 }
 
+func (n *Node) GetVolumeName(nodeName string) string {
+	return internal.SanitizeDockerResourceName(n.TestName + "-" + nodeName + "-vol")
+}
+
 // CreateAndSetupVolume creates a Docker volume for the node and sets up proper ownership.
 // This consolidates the volume creation pattern used across all node types.
 // The nodeName parameter should be the specific name for this node instance.
+// Volume names are deterministic based on testName and nodeName to support re-binding.
 func (n *Node) CreateAndSetupVolume(ctx context.Context, nodeName string) error {
-	// create volume with appropriate labels
-	v, err := n.DockerClient.VolumeCreate(ctx, volumetypes.CreateOptions{
-		Labels: map[string]string{
-			consts.CleanupLabel:   n.TestName,
-			consts.NodeOwnerLabel: nodeName,
-		},
-	})
-	if err != nil {
-		return fmt.Errorf("creating volume for %s: %w", n.nodeType.String(), err)
+	// volume name should be deterministic based on test name and node name.
+	volName := n.GetVolumeName(nodeName)
+	if err := n.ensureVolume(ctx, nodeName, volName); err != nil {
+		return fmt.Errorf("ensure volume %w", err)
 	}
 
-	n.VolumeName = v.Name
-
 	// configure volume ownership
 	if err := volume.SetOwner(ctx, volume.OwnerOptions{
 		Log:        n.Logger,
 		Client:     n.DockerClient,
-		VolumeName: v.Name,
+		VolumeName: volName,
 		ImageRef:   n.Image.Ref(),
 		TestName:   n.TestName,
 		UidGid:     n.Image.UIDGID,
@@ -189,3 +188,26 @@ func (n *Node) CreateAndSetupVolume(ctx context.Context, nodeName string) error
 
 	return nil
 }
+
+// ensureVolume checks if a volume with the given name exists, and creates it if it doesn't.
+func (n *Node) ensureVolume(ctx context.Context, nodeName, volName string) error {
+	// check if volume already exists
+	_, err := n.DockerClient.VolumeInspect(ctx, volName)
+	if err != nil {
+		// volume doesn't exist, create it
+		v, createErr := n.DockerClient.VolumeCreate(ctx, volumetypes.CreateOptions{
+			Name: volName,
+			Labels: map[string]string{
+				consts.CleanupLabel:   n.TestName,
+				consts.NodeOwnerLabel: nodeName,
+			},
+		})
+		if createErr != nil {
+			return fmt.Errorf("creating volume for %s: %w", n.nodeType.String(), createErr)
+		}
+		volName = v.Name
+	}
+
+	n.VolumeName = volName
+	return nil
+}

framework/docker/cosmos/node.go

@@ -242,7 +242,7 @@ func (cn *ChainNode) GetKeyring() (keyring.Keyring, error) {
 
 // Name of the test node container.
 func (cn *ChainNode) Name() string {
-	return fmt.Sprintf("%s-%s-%d-%s", cn.ChainID, cn.NodeType(), cn.Index, internal.SanitizeContainerName(cn.TestName))
+	return fmt.Sprintf("%s-%s-%d-%s", cn.ChainID, cn.NodeType(), cn.Index, internal.SanitizeDockerResourceName(cn.TestName))
 }
 
 // NodeType returns the type of the ChainNode as a string.

framework/docker/dataavailability/node.go

@@ -95,7 +95,7 @@ func NewNode(cfg Config, testName string, image container.Image, index int, node
 
 // Name returns the container name for the Node.
 func (n *Node) Name() string {
-	return fmt.Sprintf("da-%s-%d-%s", n.nodeType.String(), n.Index, internal.SanitizeContainerName(n.TestName))
+	return fmt.Sprintf("da-%s-%d-%s", n.nodeType.String(), n.Index, internal.SanitizeDockerResourceName(n.TestName))
 }
 
 // HostName returns the condensed hostname for the Node.

framework/docker/evstack/node.go

@@ -73,7 +73,7 @@ func NewNode(cfg Config, testName string, image container.Image, index int, isAg
 
 // Name of the test node container.
 func (n *Node) Name() string {
-	return fmt.Sprintf("%s-evstack-%d-%s", n.cfg.ChainID, n.Index, internal.SanitizeContainerName(n.TestName))
+	return fmt.Sprintf("%s-evstack-%d-%s", n.cfg.ChainID, n.Index, internal.SanitizeDockerResourceName(n.TestName))
 }
 
 // HostName returns the condensed hostname for the Node.
@@ -294,4 +294,3 @@ func (n *Node) isNodeHealthy(client *http.Client, healthURL string) bool {
 	n.logger().Debug("evstack node not ready yet", zap.String("url", healthURL), zap.Int("status", resp.StatusCode))
 	return false
 }
-

framework/docker/evstack/reth/node.go

@@ -54,7 +54,7 @@ func newNode(ctx context.Context, cfg Config, testName string, index int) (*Node
 
 // Name returns a stable container name
 func (n *Node) Name() string {
-	return fmt.Sprintf("reth-%d-%s", n.Index, internal.SanitizeContainerName(n.TestName))
+	return fmt.Sprintf("reth-%d-%s", n.Index, internal.SanitizeDockerResourceName(n.TestName))
 }
 
 // HostName returns a condensed hostname

framework/docker/ibc/relayer/hermes.go

@@ -72,7 +72,7 @@ func NewHermes(ctx context.Context, dockerClient *dockerclient.Client, testName,
 
 // Name returns the hostname of the docker container.
 func (h *Hermes) Name() string {
-	return fmt.Sprintf("%s-%d-hermes", internal.SanitizeContainerName(h.TestName), h.Index)
+	return fmt.Sprintf("%s-%d-hermes", internal.SanitizeDockerResourceName(h.TestName), h.Index)
 }
 
 // Start starts the Hermes relayer.

framework/docker/internal/strings.go

@@ -24,11 +24,11 @@ func CondenseHostName(name string) string {
 
 var validContainerCharsRE = regexp.MustCompile(`[^a-zA-Z0-9_.-]`)
 
-// SanitizeContainerName returns name with any
+// SanitizeDockerResourceName returns name with any
 // invalid characters replaced with underscores.
 // Subtests will include slashes, and there may be other
 // invalid characters too.
-func SanitizeContainerName(name string) string {
+func SanitizeDockerResourceName(name string) string {
 	return validContainerCharsRE.ReplaceAllLiteralString(name, "_")
 }
 

framework/docker/node_volume_test.go

@@ -0,0 +1,111 @@
+package docker
+
+import (
+	"github.com/celestiaorg/tastora/framework/docker/container"
+	"github.com/celestiaorg/tastora/framework/docker/internal"
+	"github.com/celestiaorg/tastora/framework/types"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap/zaptest"
+	"testing"
+)
+
+func TestNode_VolumeRebinding(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping due to short mode")
+	}
+	t.Parallel()
+
+	testCfg := setupDockerTest(t)
+
+	logger := zaptest.NewLogger(t)
+	image := container.Image{
+		Repository: "alpine",
+		Version:    "latest",
+		UIDGID:     "0:0",
+	}
+
+	createNode := func() *container.Node {
+		node := container.NewNode(
+			testCfg.NetworkID,
+			testCfg.DockerClient,
+			testCfg.TestName,
+			image,
+			"/test",
+			0,
+			types.NodeTypeValidator,
+			logger,
+		)
+		node.SetContainerLifecycle(container.NewLifecycle(logger, testCfg.DockerClient, testCfg.TestName))
+
+		nodeName := testCfg.TestName + "-test-node-0"
+
+		err := node.CreateAndSetupVolume(testCfg.Ctx, nodeName)
+		require.NoError(t, err)
+		require.NotEmpty(t, node.VolumeName)
+
+		// create a container that will just sleep forever so the test can exec into it.
+		err = node.CreateContainer(
+			testCfg.Ctx,
+			t.Name(),
+			testCfg.NetworkID,
+			image,
+			nil,
+			"",
+			[]string{node.GetVolumeName(nodeName) + ":/test"},
+			nil,
+			internal.CondenseHostName(nodeName),
+			[]string{"sleep", "infinity"},
+			nil,
+			nil,
+		)
+		require.NoError(t, err)
+		return node
+	}
+
+	node := createNode()
+
+	err := node.StartContainer(testCfg.Ctx)
+	require.NoError(t, err)
+
+	volumeName1 := node.VolumeName
+
+	// write test files to volume
+	testContent1 := []byte("test file 1 content")
+	testContent2 := []byte("test file 2 content")
+
+	err = node.WriteFile(testCfg.Ctx, "test1.txt", testContent1)
+	require.NoError(t, err)
+
+	err = node.WriteFile(testCfg.Ctx, "test2.txt", testContent2)
+	require.NoError(t, err)
+
+	// verify files were written
+	readContent1, err := node.ReadFile(testCfg.Ctx, "test1.txt")
+	require.NoError(t, err)
+	require.Equal(t, testContent1, readContent1)
+
+	readContent2, err := node.ReadFile(testCfg.Ctx, "test2.txt")
+	require.NoError(t, err)
+	require.Equal(t, testContent2, readContent2)
+
+	// remove the code but preserver the volumes, this allows us to create a new node on the same volume.
+	err = node.Remove(testCfg.Ctx, types.WithPreserveVolumes())
+	require.NoError(t, err)
+
+	// re-creating exact same node, which should bind to the same volume.
+	node = createNode()
+
+	volumeName2 := node.VolumeName
+
+	// verify that the same volume was used
+	require.Equal(t, volumeName1, volumeName2, "should reuse the same volume")
+
+	// verify that files are still present
+	readContent1, err = node.ReadFile(testCfg.Ctx, "test1.txt")
+	require.NoError(t, err)
+	require.Equal(t, testContent1, readContent1)
+
+	readContent2, err = node.ReadFile(testCfg.Ctx, "test2.txt")
+	require.NoError(t, err)
+	require.Equal(t, testContent2, readContent2)
+}

framework/docker/setup.go

@@ -47,7 +47,7 @@ var KeepVolumesOnFailure = os.Getenv("ICTEST_SKIP_FAILURE_CLEANUP") != ""
 func DockerSetup(t DockerSetupTestingT) (*client.Client, string) {
 	t.Helper()
 
-	cli, err := client.NewClientWithOpts(client.FromEnv)
+	cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
 	if err != nil {
 		panic(fmt.Errorf("failed to create docker client: %v", err))
 	}