Skip to content

Move CloudLab-specific sync logic onto the provider class when a second remote provider is added #435

Description

@milindsrivastava1997

Context

#424 added LocalProvider and gated every rsync-over-SSH call site with provider.is_remote() (experiment_utils/sync.py, services/query_engine.py, services/clickhouse_service.py, services/docker_victoriametrics.py). That was the right scope for "CloudLab or single local machine," but the gate is a stopgap, not a real abstraction.

The problem

Every is_remote()-gated branch's body still reaches directly into provider.username / provider.hostname_suffix and hand-builds node{idx}.{hostname_suffix} SSH addressing — attributes that aren't part of InfrastructureProvider's interface, only CloudLabProvider's. is_remote() just answers "is there a network hop," not "is this CloudLab's specific addressing scheme."

That's fine while CloudLab is the only remote provider. It stops being fine the moment a second remote provider (AWS, Kubernetes, etc.) is added: it would correctly report is_remote() == True, fall into these branches, and then crash on provider.username/provider.hostname_suffix (or worse, silently do the wrong thing if it happens to define similarly-named attributes for an unrelated purpose).

We hit a related version of this same conflation in PrometheusService.reset() during #424: the old code branched on hasattr(provider, "username") to decide whether to shell out to a CloudLab-only reset_prometheus.py script. It turned out that script just duplicated provider.execute_command(...) with hardcoded SSH instead of going through the abstraction — so the fix was to delete the branch entirely, not to make the check smarter. File-transfer (rsync) doesn't have that same luck: InfrastructureProvider has no generic "move a file onto that node" primitive the way it does for command execution via execute_command, so there's no parallel one-line fix there.

Proposed fix

When a second remote provider is actually being added, give InfrastructureProvider a real file-transfer primitive (e.g. provider.sync_to_remote(local_path, remote_path) / provider.sync_from_remote(...)) that each provider implements using its own addressing scheme, and move the rsync/scp construction in sync.py, query_engine.py, clickhouse_service.py, and docker_victoriametrics.py behind that method instead of hand-building CloudLab's node{idx}.{hostname_suffix} SSH command inline.

Until then, is_remote() stays as the gate (intentionally simpler to fail loud via AttributeError on a missing .username than to silently skip a needed transfer), and LocalProvider is is the only thing exercising the "false" branch.

Scope note

Not urgent — only relevant once a second remote provider is actually planned. Filed as a flag for that point, not a task to pick up now.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions