Skip to content

[FEATURE] add swarm provider#199

Merged
burtenshaw merged 6 commits into
releasefrom
exp-swarm-provider
Dec 19, 2025
Merged

[FEATURE] add swarm provider#199
burtenshaw merged 6 commits into
releasefrom
exp-swarm-provider

Conversation

@burtenshaw

Copy link
Copy Markdown
Collaborator

In response to #194 , this PR experiments with a docker swarm provider.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 14, 2025
@burtenshaw burtenshaw marked this pull request as draft November 14, 2025 22:12
@burtenshaw burtenshaw changed the title [FEATURE/WIP] add swarm provider [FEATURE] add swarm provider Nov 17, 2025
@burtenshaw burtenshaw marked this pull request as ready for review November 17, 2025 09:39
@jspisak jspisak added the enhancement New feature or request label Nov 19, 2025
@burtenshaw burtenshaw mentioned this pull request Dec 3, 2025
@burtenshaw burtenshaw requested a review from pankit-eng December 3, 2025 19:20
@burtenshaw burtenshaw changed the base branch from main to release December 18, 2025 19:41
@Darktex

Darktex commented Dec 18, 2025

Copy link
Copy Markdown
Collaborator

Claude's review copypaste while I look at it:

Pull Request #199 Review

Summary

This PR adds a DockerSwarmProvider that creates replicated Docker Swarm services with built-in load balancing, enabling local concurrency for environment containers. The implementation is clean and well-documented, but has a critical bug that prevents it from being used.

Overall Assessment: Good implementation, needs a one-line fix to be usable.

Critical Issues

1. DockerSwarmProvider Not Exported

Severity: CRITICAL - The class cannot be imported by users

The class is defined in src/openenv/core/containers/runtime/providers.py:287, but src/openenv/core/containers/runtime/init.py doesn't export it:

# Current __init__.py
from .providers import (
    ContainerProvider,
    KubernetesProvider,
    LocalDockerProvider,
    RuntimeProvider,
)
# DockerSwarmProvider is missing!

Fix:

from .providers import (
    ContainerProvider,
    DockerSwarmProvider,  # Add this
    KubernetesProvider,
    LocalDockerProvider,
    RuntimeProvider,
)

__all__ = [
    "ContainerProvider",
    "DockerSwarmProvider",  # Add this
    "LocalDockerProvider",
    "KubernetesProvider",
    "RuntimeProvider",
    "UVProvider",
]

Important Issues

2. wait_for_ready Doesn't Actually Wait For All Replicas

Severity: IMPORTANT - Misleading behavior

The docstring at providers.py:459-462 says:

def wait_for_ready(self, base_url: str, timeout_s: float = 30.0) -> None:
    """
    Wait for *all* replicas to become healthy by polling /health.
    """

But the implementation just polls until one replica responds. With Swarm's load balancer, requests round-robin across replicas, so some might still be starting when this returns.

Options:

  1. Update the docstring to reflect actual behavior (waits for "at least one")
  2. Or actually check all replicas via docker service ps to verify all tasks are running

3. No Way to Scale After Creation

Severity: MINOR - Missing feature

There's no scale(replicas: int) method. Users can't dynamically adjust replica count after the service is created. This would be useful for:

  • Scaling up during high load
  • Scaling down to save resources
  • Testing different concurrency levels

Suggested addition:

def scale(self, replicas: int) -> None:
    """Scale the service to the specified number of replicas."""
    if not self._service_name:
        raise RuntimeError("No service running")
    subprocess.run(
        ["docker", "service", "scale", f"{self._service_name}={replicas}"],
        check=True,
        capture_output=True,
    )

Minor Issues

4. No Tests

Severity: MINOR - Hard to verify correctness

There are no tests for DockerSwarmProvider. Given that it modifies system state (Swarm init, networks, services), integration tests would help catch regressions.

Consider adding:

  • Unit test with mocked subprocess calls
  • Integration test that requires Docker (can be skipped in CI if Docker unavailable)

Positive Aspects

  • Clean implementation: Follows the same pattern as LocalDockerProvider
  • Good docstrings: Clear documentation of kwargs and behavior
  • Auto-init Swarm: Convenient for local dev (auto_init_swarm=True default)
  • Overlay network support: Enables container-to-container communication
  • Resource limits: Supports CPU and memory limits via kwargs
  • Placement constraints: Supports Swarm placement constraints
  • Error handling: Clear error messages with command output on failure
  • Graceful cleanup: stop_container() ignores already-removed services

Code Quality

The implementation is solid:

Aspect Assessment
Error handling Good - catches subprocess errors, provides context
Type hints Good - proper Optional, Dict, Sequence usage
Docstrings Good - clear kwargs documentation
Code style Good - consistent with existing providers
Security OK - no shell=True, uses subprocess list args

Action Items

  • CRITICAL: Add DockerSwarmProvider to __init__.py exports
  • Fix or clarify wait_for_ready docstring
  • Consider adding scale() method
  • Add basic tests

Review Decision

REQUEST_CHANGES - The class cannot be imported without fixing the export. This is a one-line fix, after which the PR is ready to merge.

@burtenshaw

Copy link
Copy Markdown
Collaborator Author

thanks @Darktex. I fixed claude's comments.

@burtenshaw burtenshaw merged commit 8afe95b into release Dec 19, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants