Skip to content

fix(ansible): ensure_venv_version skips a venv whose interpreter is broken — the exact state an interpreter upgrade creates #17990

Open

Description

@mrveiss

Part of #13743. Found while gathering host evidence for #13747.

Summary

roles/python_interpreter/tasks/ensure_venv_version.yml removes a wrong-version venv only when the existing venv's interpreter still runs. When the venv exists but its interpreter is broken, the guard no-ops and the stale venv survives — and that is precisely the state an interpreter upgrade produces.

The npu-worker role's own comment states the opposite guarantee:

ensure_venv_version below is what makes this safe on an already-deployed box: it detects the existing 3.11 venv as wrong-version and removes it, so the 3.11-built OpenVINO binaries in site-packages are never carried into a 3.14 venv (which would fail at import, not at deploy).

That guarantee does not hold in the case it is written for.

The chain, each step measured

The guard registers {{ venv_path }}/bin/python --version with failed_when: false, then gates removal on rc == 0:

when:
  - _ensure_venv_python_version.rc == 0
  - _ensure_venv_expected | trim not in _ensure_venv_python_version.stdout

The header explains the rc != 0 branch as "Safe/idempotent when the venv is missing (rc != 0 → no-op)". But rc != 0 has two causes, and only one is safe:

rc != 0 because Correct action What happens
venv is missing no-op correct
venv exists, its interpreter is gone remove it no-op — stale venv survives

The second is the normal state after the host's old interpreter is removed: venv/bin/python is a symlink to a binary that no longer exists.

Step 1 — the guard no-ops. rc != 0, so both the debug and the file: state=absent task are skipped.

Step 2 — creates: does not save it. The create task is guarded by creates: "{{ npu_install_dir }}/venv/bin/python". Ansible's creates tests path existence, which follows symlinks, so a dangling symlink reads as absent and the task runs:

islink   True
exists   False   <- what creates: tests
lexists  True

Step 3 — and re-creating does not clean it. The task is python3.14 -m venv {{ npu_install_dir }}/venv, with no --clear. Measured on a scratch venv by planting a marker in site-packages:

before:              1
after plain re-venv: 1     <- survives
after --clear:       0

Result. A 3.14 venv whose site-packages still holds binaries built for the old interpreter. OpenVINO fails at import, not at deploy — which is the exact failure mode the comment says is prevented, and the reason #13747 asks for host verification rather than a green pip install.

Why it reads as clean

This is the MEASUREMENT_DISCIPLINE shape: rc != 0 means the measurement could not be taken, and the guard treats it as there is nothing to do. Nothing found and did not look are different results, and the comment documents only the first.

Proposed fix

Distinguish the two causes before acting:

  • If the venv directory exists (lexists on bin/python, or a stat on the directory) but the interpreter does not run, treat it as wrong-version and remove it. Unreadable is not clean.
  • Add --clear to the create task so a surviving directory cannot carry old site-packages forward even if the guard is bypassed. Defence in depth: the guard decides, --clear makes the decision unrecoverable-proof.
  • Keep the genuine no-op only for "the directory does not exist at all".

Acceptance criteria

  • A venv whose interpreter symlink dangles is removed by ensure_venv_version, not skipped.
  • A venv directory that exists with no bin/python at all is removed rather than skipped.
  • The create task uses --clear, so stale site-packages cannot survive a rebuild.
  • The genuine missing-venv case is still a silent no-op (no failure, no change).
  • Verified by deliberate failure: a planted marker file in site-packages is absent after a rebuild, and present before. A test that only asserts the venv exists afterwards would pass without the fix.

Stated limits

Not reproduced against a real deployed host — the deployed worker here is already correctly on 3.14.6, so the broken-interpreter state does not currently exist on it. The chain above is demonstrated step by step (symlink semantics, creates: behaviour, venv without --clear) rather than observed end to end in production. All callers of ensure_venv_version share this guard, so the scope is every role that calls it, not only npu-worker.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions