Skip to content

Conversation

@mdelapenya
Copy link
Member

What does this PR do?

It sets the ulimits described in the Solace docs:

Why is it important?

The Solace tests are failing on CI, because of the specs of the GH runner (the tests pass on my local machine).

@mdelapenya mdelapenya requested a review from a team as a code owner November 25, 2025 09:37
@mdelapenya mdelapenya added the bug An issue with the library label Nov 25, 2025
@mdelapenya mdelapenya self-assigned this Nov 25, 2025
@netlify
Copy link

netlify bot commented Nov 25, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit aa01e9c
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-go/deploys/692578dbd9dac00008d62b3a
😎 Deploy Preview https://deploy-preview-3497--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Summary by CodeRabbit

  • Chores

    • Dependency update: Docker units configuration adjusted to direct inclusion.
  • New Features

    • Solace container now configured with resource limits for memory and file descriptor management.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

The changes upgrade the github.com/docker/go-units dependency from indirect to direct and implement Docker ulimits configuration for Solace containers, setting specific resource limits (open file descriptors, core dumps, memory locking) via HostConfig during container initialization.

Changes

Cohort / File(s) Summary
Dependency Management
modules/solace/go.mod
Promoted github.com/docker/go-units from indirect to direct require.
Container Configuration
modules/solace/solace.go
Added Docker units import, configured ulimits in HostConfig (nofile: 1048576, core: -1, memlock: -1), implemented functional options pattern in Run method, and added documentation for required ulimits.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant Run
    participant HostConfig
    participant Container

    Caller->>Run: Run(ctx, opts...)
    activate Run
    Note over Run: Create container settings
    
    loop opts iteration
        Run->>Run: Apply Option to settings
    end
    
    Run->>HostConfig: Set Ulimits
    activate HostConfig
    HostConfig->>HostConfig: nofile = 1048576
    HostConfig->>HostConfig: core = -1
    HostConfig->>HostConfig: memlock = -1
    deactivate HostConfig
    
    Run->>Container: Create with HostConfig
    activate Container
    Container-->>Run: Success
    deactivate Container
    
    Run-->>Caller: Container ready
    deactivate Run
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify that ulimit values (1048576, -1, -1) are correct and justified for Solace requirements
  • Confirm functional options pattern is correctly applied and all provided options are properly processed
  • Check that the Docker units import is properly utilized and the dependency is necessary

Poem

🐰 With Docker's units so fine and true,
Ulimits flow through containers too,
File descriptors dance at peaks so high,
While memory locks and core dumps sigh,
Solace finds peace in limits refined! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix(solace): set ulimits for container' directly reflects the main change: configuring Solace container ulimits to resolve CI test failures.
Description check ✅ Passed The description is related to the changeset, explaining the purpose of setting ulimits and providing links to relevant Solace documentation and the reason for the fix.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
modules/solace/solace.go (1)

12-18: Ulimits configuration via units.Ulimit matches Docker API usage

Importing github.com/docker/go-units and assigning hc.Ulimits = []*units.Ulimit{...} is idiomatic for Docker’s HostConfig.Ulimits, and the chosen values (nofile=1048576, core=-1, memlock=-1) align with what Solace brokers typically require. One optional consideration: this assignment replaces any existing hc.Ulimits; if you ever expect callers or other customizers to preconfigure ulimits, you might instead append or consciously document that Solace settings are authoritative.

Also applies to: 66-88

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00f2020 and aa01e9c.

📒 Files selected for processing (2)
  • modules/solace/go.mod (1 hunks)
  • modules/solace/solace.go (3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.

Applied to files:

  • modules/solace/go.mod
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.

Applied to files:

  • modules/solace/go.mod
  • modules/solace/solace.go
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.

Applied to files:

  • modules/solace/solace.go
⏰ 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). (3)
  • GitHub Check: test (1.24.x, modules/solace) / test: modules/solace/1.24.x
  • GitHub Check: test (1.25.x, modules/solace) / test: modules/solace/1.25.x
  • GitHub Check: Analyze (go)
🔇 Additional comments (2)
modules/solace/go.mod (1)

7-14: Direct go-units dependency correctly reflects new import

Promoting github.com/docker/go-units v0.5.0 to a direct requirement matches the new usage in solace.go and avoids depending on it transitively; this looks correct as-is.

modules/solace/solace.go (1)

29-38: Run documentation clearly surfaces Solace ulimit requirements

The expanded comment on Run is clear and helpful, explicitly calling out the required ulimits and linking to the Solace docs. It accurately reflects what the module now enforces via HostConfig.Ulimits.

@mdelapenya mdelapenya merged commit 615ad22 into testcontainers:main Nov 25, 2025
17 checks passed
@mdelapenya mdelapenya deleted the solace-fix branch November 25, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug An issue with the library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant