Skip to content

salt-ssh: Single.run_wfunc() never dispatches to the existing _run_wfunc_relenv() method (dead code) #70225

Description

@twangboy

Description

salt.client.ssh.Single.run_wfunc() always calls self._run_wfunc_thin(), even when the deployment type is relenv:

def run_wfunc(self):
    """
    ...
    """
    return self._run_wfunc_thin()

Single._run_wfunc_relenv() already exists as a complete implementation specifically intended for this case -- its own docstring says "Execute a function using salt-call from relenv deployment. Bypasses the wrapper system entirely since relenv includes a full salt-call binary." It is never called from anywhere in the codebase.

Why this isn't a trivial one-line fix

Wiring up _run_wfunc_relenv() naively (dispatch on self.opts.get("relenv")) was attempted while investigating #70186 and found to be unsafe as-is:

  1. No deploy-bootstrap fallback. self.deploy() (which sends the relenv tarball and, via cmd_block()'s SCP logic, the minion config) is only ever triggered from inside cmd_block()'s "undefined SHIM state" error-detection/retry logic. _run_wfunc_relenv() has no equivalent -- it just runs {thin_dir}/salt-call and assumes the target is already deployed. Routing state.apply/etc. straight to it would break the very first command against a fresh target.
  2. Config-dir mismatch. _run_wfunc_relenv() passes --config-dir={thin_dir}/conf to salt-call, but the actual shim (SSH_SH_SHIM_RELENV) writes the minion config to {thin_dir}/minion and invokes salt-call -c "{THIN_DIR}" (i.e. config lives at {thin_dir}, not {thin_dir}/conf). _run_wfunc_relenv()'s own config path appears to have never been exercised/tested.
  3. Some wrapper functions may be master-side-only. salt/client/ssh/wrapper/state.py (and others) are heavily __context__["fileclient"]-dependent -- pillar/state compilation happens on the master using the master's fileserver access, which a bare salt-call --local on the target cannot replicate. It's not obvious which wrapper functions are safe to bypass this way without wrapper-by-wrapper review.

Suggested next step

Either: (a) fix _run_wfunc_relenv()'s config-dir path and add deploy-bootstrap/retry logic so it can safely replace _run_wfunc_thin() for the functions where that's valid (likely just state.*), or (b) remove _run_wfunc_relenv() entirely as unused/superseded dead code if no one intends to finish wiring it up.

Related

Activity

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions