chore: add labeled client and update volume cleanup #145
Conversation
WalkthroughReplaces direct Moby Docker client usage with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Setup as Setup()
participant Wrapper as tastoraclient.Client
participant SDK as Docker SDK (moby client)
Test->>Setup: Setup(t) -> (client, cleanupLabel)
Setup->>SDK: create raw Docker client
Setup->>Wrapper: NewClient(rawClient, cleanupLabel)
Setup-->>Test: return Wrapper (TastoraDockerClient)
Test->>Wrapper: Create/Start/Inspect containers
Wrapper->>SDK: delegated CommonAPIClient calls
Note right of Wrapper: Wrapper exposes CleanupLabel()
Test->>Wrapper: request Cleanup()
Wrapper-->>Test: CleanupLabel
Test->>SDK: list & remove volumes/networks filtered by label
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-27T08:24:51.731ZApplied to files:
📚 Learning: 2025-08-07T13:30:18.395ZApplied to files:
📚 Learning: 2025-08-27T08:33:47.114ZApplied to files:
🧬 Code graph analysis (4)framework/docker/docker_test.go (4)
framework/docker/file/file_writer_test.go (1)
framework/docker/file/file_retriever_test.go (1)
framework/docker/setup.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/docker/container/lifecycle.go (1)
45-58: Go compilation will fail: unused parametertestNamemust be discarded or removedThe parameter
testNameis unused in the function body (verified: declared at line 47, referenced nowhere else). Go forbids unused parameters. The proposed fix to discard it with blank identifier_is correct and maintains signature stability while fixing the compilation error. Multiple call sites currently pass this parameter.func (c *Lifecycle) CreateContainer( ctx context.Context, - testName string, + _ string, networkID string, image Image,Alternatively, if removing it entirely is preferred, update call sites at:
- framework/docker/dataavailability/node.go:283
- framework/docker/ibc/relayer/hermes.go:88
- framework/docker/evstack/reth/node.go:236
- framework/docker/cosmos/node.go:464
- framework/docker/evstack/evmsingle/node.go:189
- framework/docker/evstack/node.go:159
- framework/docker/container/node.go:140
🧹 Nitpick comments (12)
framework/docker/evstack/reth/builder.go (1)
93-105: Guard against a nil Docker client in BuildAdd a fast-fail to avoid later NPEs when callers forget WithDockerClient.
Apply:
@@ import ( "context" + "errors" "github.com/celestiaorg/tastora/framework/types" "testing" @@ func (b *NodeBuilder) Build(ctx context.Context) (*Node, error) { + if b.dockerClient == nil { + return nil, errors.New("docker client is required; call WithDockerClient") + } cfg := Config{framework/docker/container/node.go (1)
193-207: Differentiate “not found” from real errors in ensureVolumeTreating any inspect error as “volume missing” can mask daemon/permission/network issues. Use errdefs.IsNotFound to branch.
Apply:
@@ -import ( +import ( "context" "fmt" "github.com/celestiaorg/tastora/framework/docker/internal" @@ "github.com/celestiaorg/tastora/framework/types" "github.com/docker/docker/api/types/mount" volumetypes "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" "github.com/docker/go-connections/nat" "go.uber.org/zap" ) @@ - _, err := n.DockerClient.VolumeInspect(ctx, volName) - if err != nil { - // volume doesn't exist, create it + _, err := n.DockerClient.VolumeInspect(ctx, volName) + if err != nil { + if !errdefs.IsNotFound(err) { + return fmt.Errorf("inspecting volume %q: %w", volName, err) + } + // volume doesn't exist, create it v, createErr := n.DockerClient.VolumeCreate(ctx, volumetypes.CreateOptions{ Name: volName, Labels: map[string]string{ consts.CleanupLabel: n.DockerClient.CleanupLabel(), consts.NodeOwnerLabel: nodeName, }, })Also applies to: 3-16
framework/docker/internal/docker_keyring_test.go (2)
39-40: Close the client in teardownRelease HTTP connections/resources after the suite.
Apply:
func (s *DockerKeyringTestSuite) TearDownSuite() { if s.containerID != "" { ctx := context.Background() _ = s.dockerClient.ContainerStop(ctx, s.containerID, container.StopOptions{}) _ = s.dockerClient.ContainerRemove(ctx, s.containerID, container.RemoveOptions{}) } + if s.dockerClient != nil { + _ = s.dockerClient.Close() + } }Also applies to: 87-93
70-72: Typo: “wet up” → “set up”Fix the comment wording.
Apply:
- // wet up keyring directory in container + // set up keyring directory in containerframework/docker/file/file_retriever.go (1)
42-59: Verify bind-mount visibility without starting the helper containerSome Docker setups only apply binds after start; CopyFromContainer may fail to see volume contents if the container never starts.
Please confirm CI/runtime behavior. If flaky, options:
- Start the container before copy, then remove:
- ContainerStart → CopyFromContainer → ContainerRemove.
- Or switch to HostConfig.Mounts with mount.TypeVolume.
Also applies to: 50-53
framework/docker/client/client.go (1)
13-17: Consider tiny helpers to reduce label boilerplateA helper for common label/filter usage would cut repetition and mistakes at call sites.
// In this package: func (c *Client) LabelsMap(key string) map[string]string { return map[string]string{key: c.cleanupLabel} }Usage:
Labels: dockerClient.LabelsMap(consts.CleanupLabel).Also applies to: 29-32
framework/docker/container/lifecycle.go (3)
37-43: Param naming nitRename
ctoclito avoid confusion with the method receiverc *Lifecycleelsewhere.
214-225: Stop should also ignore NotFoundFor parity with other call sites and cleanup flows, treat “not found” like “already stopped.”
-err := c.client.ContainerStop(ctx, c.id, timeout) -if err != nil && errdefs.IsNotModified(err) { +err := c.client.ContainerStop(ctx, c.id, timeout) +if err != nil && (errdefs.IsNotModified(err) || errdefs.IsNotFound(err)) { // container is already stopped, this is not an error return nil }
172-175: Prefer structured logging over fmt.PrintfUse
c.logto emit the panic details instead of printing to stdout.- fmt.Printf("\nContainer name: %s.\nerror: %s.\nlogs\n%s\n", c.containerName, err.Error(), logs.String()) + c.log.Error("Container failed to start", + zap.String("container", c.containerName), + zap.Error(err), + zap.String("logs", logs.String()), + )framework/docker/container/job.go (1)
215-221: Minor log key typo
zap.Strings("env,", opts.Env)likely meant"env". Harmless but noisy in logs.- zap.Strings("env,", opts.Env), + zap.Strings("env", opts.Env),framework/docker/setup.go (2)
158-186: Avoid double-counting reclaimed space across retries
spaceReclaimedis incremented before deletion and outside the retry closure. If a conflict retries, the same volume’s size can be counted multiple times. Increment only on successful removal.- for _, vol := range volumeList.Volumes { - // try to get volume size before removal - if vol.UsageData != nil { - spaceReclaimed += uint64(vol.UsageData.Size) - } - - err := cli.VolumeRemove(ctx, vol.Name, true) + for _, vol := range volumeList.Volumes { + err := cli.VolumeRemove(ctx, vol.Name, true) if err != nil { if errdefs.IsConflict(err) { return err } // Log but continue with other volumes t.Logf("Failed to remove volume %s: %v", vol.Name, err) } else { deletedCount++ + if vol.UsageData != nil { + spaceReclaimed += uint64(vol.UsageData.Size) + } } }Also applies to: 198-201
271-279: Optional: demux logs for readability
ContainerLogsreturns a multiplexed stream when both stdio are requested. Consider usingstdcopy.StdCopy(as in job.Wait) so logs are readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
framework/docker/chain_node_test.go(2 hunks)framework/docker/client/client.go(1 hunks)framework/docker/container/image.go(3 hunks)framework/docker/container/job.go(4 hunks)framework/docker/container/lifecycle.go(3 hunks)framework/docker/container/node.go(4 hunks)framework/docker/cosmos/chain_builder.go(2 hunks)framework/docker/cosmos/chain_config.go(1 hunks)framework/docker/cosmos/node.go(1 hunks)framework/docker/dataavailability/config.go(2 hunks)framework/docker/dataavailability/network_builder.go(3 hunks)framework/docker/docker_test.go(1 hunks)framework/docker/evstack/chain_builder.go(3 hunks)framework/docker/evstack/config.go(2 hunks)framework/docker/evstack/evmsingle/builder.go(3 hunks)framework/docker/evstack/evmsingle/config.go(1 hunks)framework/docker/evstack/reth/builder.go(3 hunks)framework/docker/evstack/reth/config.go(1 hunks)framework/docker/file/file_retriever.go(2 hunks)framework/docker/file/file_writer.go(2 hunks)framework/docker/ibc/relayer/hermes.go(1 hunks)framework/docker/ibc_test.go(2 hunks)framework/docker/ibc_transfer_test.go(0 hunks)framework/docker/internal/busybox.go(2 hunks)framework/docker/internal/docker_keyring.go(3 hunks)framework/docker/internal/docker_keyring_test.go(3 hunks)framework/docker/internal/keyring.go(2 hunks)framework/docker/internal/network.go(1 hunks)framework/docker/internal/startcontainer.go(2 hunks)framework/docker/setup.go(5 hunks)framework/docker/toml.go(1 hunks)framework/docker/volume/owner.go(3 hunks)framework/testutil/docker/toml.go(2 hunks)framework/types/docker_client.go(1 hunks)
💤 Files with no reviewable changes (1)
- framework/docker/ibc_transfer_test.go
🧰 Additional context used
📓 Path-based instructions (1)
framework/testutil/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Functions in
framework/testutilshould depend on small interfaces defined in the same package to avoid dependency cycles.
Files:
framework/testutil/docker/toml.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: chatton
PR: celestiaorg/tastora#106
File: framework/docker/dataavailability/config.go:3-7
Timestamp: 2025-08-27T08:24:51.731Z
Learning: The tastora codebase consistently uses `github.com/moby/moby/client` for Docker client imports throughout all framework/docker packages. Do not suggest changing this to `github.com/docker/docker/client` as it would break type compatibility.
📚 Learning: 2025-08-27T08:24:51.731Z
Learnt from: chatton
PR: celestiaorg/tastora#106
File: framework/docker/dataavailability/config.go:3-7
Timestamp: 2025-08-27T08:24:51.731Z
Learning: The tastora codebase consistently uses `github.com/moby/moby/client` for Docker client imports throughout all framework/docker packages. Do not suggest changing this to `github.com/docker/docker/client` as it would break type compatibility.
Applied to files:
framework/docker/cosmos/node.goframework/types/docker_client.goframework/docker/cosmos/chain_config.goframework/docker/internal/keyring.goframework/docker/evstack/evmsingle/config.goframework/docker/evstack/chain_builder.goframework/docker/internal/startcontainer.goframework/docker/docker_test.goframework/docker/container/node.goframework/docker/chain_node_test.goframework/docker/evstack/config.goframework/docker/dataavailability/network_builder.goframework/docker/evstack/evmsingle/builder.goframework/docker/toml.goframework/docker/setup.goframework/docker/client/client.goframework/docker/container/image.goframework/docker/evstack/reth/builder.goframework/testutil/docker/toml.goframework/docker/internal/docker_keyring_test.goframework/docker/cosmos/chain_builder.goframework/docker/internal/busybox.goframework/docker/file/file_writer.goframework/docker/ibc_test.goframework/docker/container/lifecycle.goframework/docker/volume/owner.goframework/docker/file/file_retriever.goframework/docker/internal/docker_keyring.goframework/docker/evstack/reth/config.goframework/docker/dataavailability/config.goframework/docker/container/job.goframework/docker/internal/network.go
📚 Learning: 2025-08-07T13:30:18.395Z
Learnt from: CR
PR: celestiaorg/tastora#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T13:30:18.395Z
Learning: Applies to framework/testutil/**/*.go : Functions in `framework/testutil` should depend on small interfaces defined in the same package to avoid dependency cycles.
Applied to files:
framework/docker/docker_test.go
🧬 Code graph analysis (33)
framework/docker/cosmos/node.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/types/docker_client.go (1)
framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/cosmos/chain_config.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/internal/keyring.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/evstack/evmsingle/config.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/evstack/chain_builder.go (3)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/cosmos/chain_builder.go (1)
ChainBuilder(109-157)framework/docker/evstack/evmsingle/builder.go (1)
ChainBuilder(14-24)
framework/docker/internal/startcontainer.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/docker_test.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/container/node.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/chain_node_test.go (2)
framework/docker/cosmos/node.go (2)
NewChainNode(115-150)ChainNodeParams(93-112)framework/docker/client/client.go (1)
Client(14-17)
framework/docker/evstack/config.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/dataavailability/network_builder.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/evstack/evmsingle/builder.go (3)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/cosmos/chain_builder.go (1)
ChainBuilder(109-157)framework/docker/evstack/chain_builder.go (1)
ChainBuilder(15-40)
framework/docker/toml.go (2)
framework/testutil/docker/toml.go (1)
ModifyConfigFile(16-51)framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/setup.go (4)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/testutil/random/random.go (1)
LowerCaseLetterString(10-16)framework/docker/client/client.go (1)
NewClient(22-27)framework/docker/consts/consts.go (2)
CelestiaDockerPrefix(4-4)CleanupLabel(6-6)
framework/docker/client/client.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/container/image.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/evstack/reth/builder.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/ibc/relayer/hermes.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/testutil/docker/toml.go (2)
framework/docker/toml.go (1)
ModifyConfigFile(15-50)framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/internal/docker_keyring_test.go (1)
framework/docker/client/client.go (2)
Client(14-17)NewClient(22-27)
framework/docker/cosmos/chain_builder.go (3)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/evstack/chain_builder.go (1)
ChainBuilder(15-40)framework/docker/evstack/evmsingle/builder.go (1)
ChainBuilder(14-24)
framework/docker/internal/busybox.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/file/file_writer.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/ibc_test.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/container/lifecycle.go (3)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/port/port.go (1)
Listeners(15-15)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/volume/owner.go (3)
framework/docker/client/client.go (1)
Client(14-17)framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/file/file_retriever.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/internal/docker_keyring.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/evstack/reth/config.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/dataavailability/config.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
framework/docker/container/job.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/internal/network.go (1)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)
🔇 Additional comments (38)
framework/docker/evstack/reth/builder.go (1)
18-18: Interface-based client injection looks goodThis aligns with the new cleanup-label abstraction and improves testability.
Also applies to: 54-57
framework/docker/container/node.go (1)
89-100: Volume removal implementation verified as correctThe RemoveVolumes signature has been successfully updated to remove the testName parameter, and the sole call site in node.go:99 correctly invokes it with only the context parameter, aligning with label-based cleanup via CleanupLabel.
framework/docker/cosmos/node.go (1)
114-149: Constructor interface change verified—all call sites compatibleConfirmed:
tastoraclient.Clientimplementstypes.TastoraDockerClientinterface (with embeddedclient.ClientandCleanupLabel()method). All call sites pass compatible types: test code uses&tastoraclient.Client{}, production code passesb.dockerClient(typed astypes.TastoraDockerClient).framework/docker/cosmos/chain_config.go (1)
6-6: LGTM: Clean interface adoption.The replacement of the concrete Docker client type with
types.TastoraDockerClientinterface is correct and aligns with the PR's objective to support labeled cleanup.Also applies to: 14-14
framework/docker/internal/busybox.go (1)
6-6: LGTM: Interface adoption in internal utility.The function signature update correctly adopts the
TastoraDockerClientinterface without affecting the internal logic.Also applies to: 28-28
framework/docker/internal/startcontainer.go (1)
5-5: LGTM: Consistent interface adoption.The
StartContainerfunction signature correctly adopts the new interface without any logic changes.Also applies to: 18-18
framework/docker/docker_test.go (1)
42-42: LGTM: Test configuration updated for labeled client.The type change correctly adopts the new
TastoraDockerClientinterface. The removal of explicit per-test cleanup registration (mentioned in the AI summary) aligns with the PR's objective—cleanup is now handled automatically via the labeled client mechanism.framework/docker/internal/keyring.go (1)
7-7: LGTM: Keyring utility updated correctly.The function signature correctly adopts the
TastoraDockerClientinterface with no changes to the keyring extraction logic.Also applies to: 21-21
framework/docker/container/image.go (1)
6-6: LGTM: Image pulling interface updated.The
PullImagemethod signature correctly adopts theTastoraDockerClientinterface without affecting the image inspection and pull logic.Also applies to: 34-34
framework/docker/evstack/reth/config.go (1)
5-5: LGTM: Reth configuration updated.The
DockerClientfield type correctly adopts theTastoraDockerClientinterface, consistent with other configuration structures.Also applies to: 12-12
framework/docker/ibc_test.go (1)
18-18: LGTM: IBC test helpers updated consistently.Both
createCelestiaChainandcreateSimappChainfunction signatures correctly adopt theTastoraDockerClientinterface, maintaining consistency across the test suite.Also applies to: 50-50
framework/docker/chain_node_test.go (1)
33-33: Empty client instances are acceptable for hostname-only tests.The test creates empty
&tastoraclient.Client{}instances. While these would fail if Docker operations were attempted, this test only validates hostname generation logic and doesn't invoke any Docker client methods.Also applies to: 46-46, 59-59
framework/docker/volume/owner.go (1)
19-19: Cleanup label correctly sourced from client interface.The change from using
opts.TestNametoopts.Client.CleanupLabel()properly implements the labeled client pattern for volume cleanup, ensuring resources are tagged with the appropriate label for deletion.Also applies to: 56-56
framework/docker/ibc/relayer/hermes.go (1)
39-39: LGTM: Interface migration applied correctly.The signature change to accept
types.TastoraDockerClientaligns with the broader refactoring effort and maintains backward compatibility through interface abstraction.framework/docker/evstack/config.go (1)
14-14: LGTM: Config struct updated to use interface.The field type change aligns with the project-wide migration to the
TastoraDockerClientinterface abstraction.framework/types/docker_client.go (1)
7-12: Well-designed interface for Docker client abstraction.The interface cleanly extends
client.CommonAPIClientwith theCleanupLabel()method, enabling resource tagging for proper cleanup. The documentation clearly states the purpose.framework/docker/dataavailability/network_builder.go (1)
23-23: LGTM: Builder pattern updated for interface.The
dockerClientfield andWithDockerClientmethod correctly adopt theTastoraDockerClientinterface, maintaining the builder pattern's fluent API.Also applies to: 100-102
framework/docker/file/file_writer.go (1)
22-22: LGTM: Consistent cleanup labeling implemented.The migration to
TastoraDockerClientand the use ofw.cli.CleanupLabel()for container labels ensures proper resource tagging for cleanup operations, consistent with the PR's objectives.Also applies to: 27-27, 56-56
framework/docker/internal/docker_keyring.go (1)
26-26: LGTM: Docker keyring updated to use interface.The migration to
TastoraDockerClientis correctly implemented. The import aliastastoratypeskeeps the code readable while avoiding naming conflicts.Also applies to: 37-43
framework/testutil/docker/toml.go (2)
19-51: LGTM! Clean migration to the interface abstraction.The function signature update correctly adopts the
TastoraDockerClientinterface, maintaining the same behavior while enabling better testability and cleanup label support.
7-7: No dependency cycle introduced.Verification confirms that
framework/typescontains no imports from other internal packages (only external dependencies). Therefore, there is no direct or transitive path fromframework/typesback toframework/testutil, and no dependency cycle is created by the new import.framework/docker/toml.go (1)
10-10: LGTM! Consistent interface adoption.The migration to
types.TastoraDockerClientis implemented correctly, aligning with the broader refactoring across the codebase.Also applies to: 18-50
framework/docker/evstack/evmsingle/builder.go (1)
5-5: LGTM! Builder correctly adopts the interface.The
ChainBuilderstruct and its fluent API methods are updated consistently to usetypes.TastoraDockerClient.Also applies to: 18-18, 54-57
framework/docker/dataavailability/config.go (1)
5-5: LGTM! Config struct updated correctly.The
DockerClientfield type migration totypes.TastoraDockerClientis consistent with the codebase-wide refactoring.Also applies to: 14-14
framework/docker/evstack/chain_builder.go (1)
6-6: LGTM! Evstack ChainBuilder migrated correctly.The builder pattern correctly adopts the
types.TastoraDockerClientinterface, maintaining consistency with other chain builders in the codebase.Also applies to: 23-23, 93-96
framework/docker/cosmos/chain_builder.go (1)
117-117: LGTM! Cosmos ChainBuilder updated consistently.The
dockerClientfield andWithDockerClientmethod are correctly migrated to usetypes.TastoraDockerClient, aligning with the interface abstraction pattern.Also applies to: 249-251
framework/docker/evstack/evmsingle/config.go (1)
5-5: LGTM! Config struct migrated correctly.The
DockerClientfield type update totypes.TastoraDockerClientis consistent with other config structures in the codebase.Also applies to: 12-12
framework/docker/internal/network.go (1)
6-6: LGTM! Internal network utility updated correctly.The
GetContainerInternalIPfunction signature correctly adoptstypes.TastoraDockerClient, which embedsclient.CommonAPIClientand provides the necessaryContainerInspectmethod.Also applies to: 12-12
framework/docker/client/client.go (1)
29-32: LGTM; wrapper cleanly satisfies TastoraDockerClientEmbedding moby client and exposing
CleanupLabel()matches the interface; import usesgithub.com/moby/moby/clientas in the rest of the codebase. Based on learningsframework/docker/container/lifecycle.go (1)
99-105: Correct label usageTagging the container with
{consts.CleanupLabel: c.client.CleanupLabel()}is exactly what enables precise cleanup.framework/docker/container/job.go (3)
29-35: Interface migration looks goodSwapping to
types.TastoraDockerClientand keepingtestNamefor naming is consistent with the new cleanup model.
180-181: Good: containers labeled with client cleanup labelThis aligns container lifecycle with the new per-run cleanup.
45-78: All NewJob call sites are compliant with the updated signature.Verification confirms all three invocations of
NewJobcorrectly passtypes.TastoraDockerClient:
file_writer_test.go:30andfile_retriever_test.go:30passclifromdocker.DockerSetup(t), which returnstypes.TastoraDockerClientnode.go:67passesn.DockerClient, which is of typetypes.TastoraDockerClientframework/docker/setup.go (5)
58-66: Nice: unique per-run cleanup label and labeled networkNetwork creation carries the same label for targeted cleanup.
71-74: t.Cleanup wiring is correctUsing
DockerCleanup(t, dockerClient)ensures the label travels with the client.
149-151: Helper mirrors new modelForwarding to the test-name variant via
cli.CleanupLabel()keeps the public API simple.
77-147: Cleanup filters follow label correctlyContainer selection, stop/wait/remove flow, and deferring volume/network pruning to labeled routines are sound.
50-74: No action required — API change properly verified across all callersAll four call sites of
DockerSetup()have been correctly updated:
ibc_transfer_test.go:51anddocker_test.go:66properly destructure the return tuple and passdockerClientto downstream APIsfile_retriever_test.go:22andfile_writer_test.go:22usecliwith compatibleVolumeCreate()callstypes.TastoraDockerClientinterface extendsclient.CommonAPIClient, maintaining method compatibility- All downstream usage patterns confirmed compatible with the new return type
| // NewClient creates a new Client with the given Docker client and cleanup label. | ||
| func NewClient(c *client.Client, cleanupLabel string) *Client { | ||
| return &Client{ | ||
| Client: c, | ||
| cleanupLabel: cleanupLabel, | ||
| } | ||
| } |
There was a problem hiding this comment.
Panic early on nil underlying client
Defend against accidental nil by validating the passed *client.Client.
func NewClient(c *client.Client, cleanupLabel string) *Client {
+ if c == nil {
+ panic("nil docker client")
+ }
return &Client{
Client: c,
cleanupLabel: cleanupLabel,
}
}🤖 Prompt for AI Agents
In framework/docker/client/client.go around lines 21 to 27, NewClient currently
accepts a *client.Client that may be nil; add a nil check and panic (or return
an error if API design prefers) early to fail fast. Update NewClient to validate
the incoming c parameter and panic with a clear message if c == nil (or change
signature to return (*Client, error) and return an error) so the package defends
against accidental nil underlying client.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/docker/volume/owner.go (1)
18-25: Remove unusedTestNamefield fromOwnerOptionsstruct.The
TestNamefield is no longer used in theSetOwnerfunction—cleanup labels are now sourced fromopts.Client.CleanupLabel(). Remove the field from the struct definition and update the instantiation atframework/docker/container/node.go:182where it's being passed.
🧹 Nitpick comments (1)
framework/docker/file/file_writer.go (1)
21-25: Interface migration is correct. Remove unusedtestNamefield.The change from
*client.Clienttotypes.TastoraDockerClientinterface is sound and properly enables the labeled cleanup functionality. However, thetestNamefield is never used in anyWritermethods—it's set in the constructor but never read. This appears to be residual code that should be removed.Remove the
testNamefield from the Writer struct (line 24) and from theNewWriterfunction signature and initialization (lines 28-29).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
framework/docker/cosmos/node.go(1 hunks)framework/docker/file/file_retriever.go(3 hunks)framework/docker/file/file_writer.go(2 hunks)framework/docker/volume/owner.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/docker/file/file_retriever.go
- framework/docker/cosmos/node.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: chatton
PR: celestiaorg/tastora#106
File: framework/docker/dataavailability/config.go:3-7
Timestamp: 2025-08-27T08:24:51.731Z
Learning: The tastora codebase consistently uses `github.com/moby/moby/client` for Docker client imports throughout all framework/docker packages. Do not suggest changing this to `github.com/docker/docker/client` as it would break type compatibility.
📚 Learning: 2025-08-27T08:24:51.731Z
Learnt from: chatton
PR: celestiaorg/tastora#106
File: framework/docker/dataavailability/config.go:3-7
Timestamp: 2025-08-27T08:24:51.731Z
Learning: The tastora codebase consistently uses `github.com/moby/moby/client` for Docker client imports throughout all framework/docker packages. Do not suggest changing this to `github.com/docker/docker/client` as it would break type compatibility.
Applied to files:
framework/docker/volume/owner.goframework/docker/file/file_writer.go
🧬 Code graph analysis (2)
framework/docker/volume/owner.go (3)
framework/docker/client/client.go (1)
Client(14-17)framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
framework/docker/file/file_writer.go (2)
framework/types/docker_client.go (1)
TastoraDockerClient(9-12)framework/docker/consts/consts.go (1)
CleanupLabel(6-6)
🔇 Additional comments (6)
framework/docker/volume/owner.go (3)
11-11: LGTM!The import is necessary for the new
types.TastoraDockerClientinterface.
42-43: LGTM!Minor readability improvement.
57-57: LGTM! Cleanup label now sourced from the client.This change centralizes the cleanup label management by sourcing it from the client interface rather than the test name, which aligns with the PR's objective to fix volume cleanup issues in subtests.
framework/docker/file/file_writer.go (3)
13-13: LGTM: Import addition supports the interface migration.The new import is necessary for the
types.TastoraDockerClientinterface.
28-30: LGTM: Constructor signature updated to accept interface.The signature change from
*client.Clienttotypes.TastoraDockerClientis a breaking API change but aligns with the PR objectives to enable proper cleanup via labeled clients. The interface-based design is more flexible and testable.
57-57: LGTM: Core cleanup fix implemented correctly.The label now uses
w.cli.CleanupLabel()to get a per-run cleanup label value, enabling proper resource cleanup. This directly addresses the PR objective of ensuring volumes and containers created in tests are properly removed.
mojtaba-esk
left a comment
mojtaba-esk
left a comment
There was a problem hiding this comment.
LGTM! thanks for the great work!
Left a few nits
| s.Require().NoError(err) | ||
|
|
||
| s.dockerClient = dockerClient | ||
| s.dockerClient = tastoraclient.NewClient(dockerClient, "docker-keyring-test-suite") |
There was a problem hiding this comment.
Q: is the "docker-keyring-test-suite" fixed for all tests? is it safe to do so if two tests are running concurrently on the same machine? will they share their keyring?
There was a problem hiding this comment.
oh good call, I think we need to move this to SetupTest and add a random element to the name
There was a problem hiding this comment.
actually I think in this case it doesn't make a difference since we aren't using the label for anything
framework/docker/setup.go
Outdated
| func DockerCleanup(t DockerSetupTestingT, cli *client.Client) func() { | ||
| return DockerCleanupWithTestName(t, cli, t.Name()) | ||
| func DockerCleanup(t DockerSetupTestingT, cli types.TastoraDockerClient) func() { | ||
| return DockerCleanupWithTestName(t, cli, cli.CleanupLabel()) |
There was a problem hiding this comment.
Nit: we could probably rename this func: DockerCleanupWithTestName
There was a problem hiding this comment.
since we are changing names maybe we can actually use this opportunity to remove the Docker prefix from the names since the package is already docker. so we get docker.Setup instead of docker.DockerSetup etc.
framework/docker/setup.go
Outdated
| } | ||
|
|
||
| func PruneVolumesWithRetryAndTestName(ctx context.Context, t DockerSetupTestingT, cli *client.Client, testName string) { | ||
| func PruneVolumesWithRetryAndTestName(ctx context.Context, t DockerSetupTestingT, cli types.TastoraDockerClient, testName string) { |
There was a problem hiding this comment.
Nit: Also here instead of testname here PruneVolumesWithRetryAndTestName we refer to a cleanup label
framework/docker/setup.go
Outdated
| for _, vol := range volumeList.Volumes { | ||
| // try to get volume size before removal | ||
| if vol.UsageData != nil { | ||
| spaceReclaimed += uint64(vol.UsageData.Size) |
There was a problem hiding this comment.
Nit: to have it more accurate, one could add it to the total spaceReclaimed after being successfully deleted.
There was a problem hiding this comment.
this way it adds it before, and if retry to remove it, it will be added again and again.
|
merged as the failure is still the jwt-secret one which is being fixed in a separate branch |
Overview
Volumes were still not being cleaned up after tests, I found that this was due to
pruneexplicitly not working on volumes that were created with the docker client, even if there was no associated container (this is by design)This PR should also address the issue of resources not being correctly deleted when created in subtests (due to issues with t.Name() ), this is achieved by adding in a label upon client creation, and then using that label when that client instance creates or removes resources.
Method signatures needed to be updated to take an interface rather than the concrete type.
Summary by CodeRabbit