Skip to content

fix(e2e): revert direct Ollama harness commits - #8626

Merged
cv merged 2 commits into
mainfrom
revert/emergency-ollama-harness-main-commits
Aug 8, 2026
Merged

fix(e2e): revert direct Ollama harness commits#8626
cv merged 2 commits into
mainfrom
revert/emergency-ollama-harness-main-commits

Conversation

@ericksoa

@ericksoa ericksoa commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR reverts the two remaining direct main commits made outside the intended #8058 PR workflow. It restores only the three affected E2E files for review and does not claim that the restored protected E2E harness behavior is correct.

Changes

  • Revert ebbbc91c005d07cf2a52cf8673b1e4f795490e3f (fix(e2e): require Ollama service cleanup).
  • Revert 5722e63901fa7187f761993e1679ff98dce44e5b (fix(e2e): use installed Ollama service in protected run).
  • Restore the affected paths to their exact contents before 5722e63901fa7187f761993e1679ff98dce44e5b.
  • Preserve unrelated commit 37f54757a680a26cc63fc0b2a2c816b15aae57c0 and its complete diff.
  • Do not duplicate the existing verified main revert fad771aa3296c8df47994602bb41ec69d9492e12, which already reverts 11ae5a69d2930b4092ed0e7ec6de3249806599df.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification: The semantic E2E phase check and the focused GPU E2E helper suite pass for the restored file tree.
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This PR restores internal E2E harness and test files without changing a supported user surface.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer-directed exact source reversion; the affected paths match the pre-5722e639 contents and add no behavior.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: The diff changes only three internal E2E harness and test files. It does not change a supported CLI, API, configuration, default, user workflow, or documentation surface.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npm run test:e2e-phases:check passed for 122 tests across 79 files; npx vitest run --project e2e-support test/e2e/support/gpu-e2e-helpers.test.ts passed 24 tests.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Aaron Erickson aerickson@nvidia.com

Summary by CodeRabbit

  • Tests
    • Improved GPU end-to-end test setup and teardown reliability.
    • Simplified runtime startup and cleanup behavior to reduce test-environment failures.
    • Removed obsolete coverage for deprecated service-management scenarios.

Reverts ebbbc91.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Reverts 5722e63.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@ericksoa ericksoa self-assigned this Aug 8, 2026
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 67bd7732-0a69-48f7-ab99-a4da5159b9b2

📥 Commits

Reviewing files that changed from the base of the PR and between fad771a and 37ce69c.

📒 Files selected for processing (3)
  • test/e2e/live/gpu-e2e-helpers.ts
  • test/e2e/live/managed-image-protected-runtime-helpers.ts
  • test/e2e/support/gpu-e2e-helpers.test.ts

📝 Walkthrough

Walkthrough

Ollama GPU end-to-end helpers now use direct process startup and cleanup commands. Startup writes to a temporary log and polls /api/tags. Teardown ignores cleanup command results. Tests remove coverage for the deleted systemd and privileged cleanup paths.

Changes

Ollama runtime lifecycle

Layer / File(s) Summary
Direct Ollama startup and cleanup
test/e2e/live/gpu-e2e-helpers.ts, test/e2e/live/managed-image-protected-runtime-helpers.ts
Cleanup stops Ollama services and processes directly. Protected startup launches ollama serve, sets OLLAMA_HOST, writes to a temporary log, and polls /api/tags. Teardown no longer asserts cleanup command success.
Test alignment
test/e2e/support/gpu-e2e-helpers.test.ts
Tests remove unused imports and delete mocks for privileged cleanup and systemd-based restart behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RuntimeHelper
  participant cleanupOllama
  participant Ollama
  participant TagsAPI
  RuntimeHelper->>cleanupOllama: stop existing runtime
  RuntimeHelper->>Ollama: start ollama serve
  RuntimeHelper->>TagsAPI: poll /api/tags
  TagsAPI-->>RuntimeHelper: readiness response
Loading

Suggested reviewers: jyaunches, aasthajh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the E2E Ollama harness reversion, which matches the primary changes and objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert/emergency-ollama-harness-main-commits

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

@github-code-quality

github-code-quality Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 37ce69c in the revert/emergency-oll... branch remains at 96%, unchanged from commit 7846127 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 37ce69c in the revert/emergency-oll... branch remains at 81%, unchanged from commit ebbbc91 in the main branch.

Show a code coverage summary of the most impacted files.
File main ebbbc91 revert/emergency-oll... 37ce69c +/-
src/lib/inferen...vllm-api-key.ts 90% 83% -7%
src/lib/inferen...fecycle-lock.ts 80% 76% -4%
src/lib/inferen.../vllm-models.ts 87% 84% -3%
src/lib/inferen...tion-cluster.ts 85% 83% -2%
src/lib/inferen...er-lifecycle.ts 79% 77% -2%
src/lib/inferen...er-lifecycle.ts 77% 76% -1%
src/lib/adapters/http/probe.ts 87% 86% -1%
src/lib/credentials/store.ts 56% 55% -1%
src/lib/inferen...ter-executor.ts 70% 70% 0%
src/lib/onboard...carryforward.ts 88% 96% +8%

Updated August 08, 2026 15:57 UTC

@cv
cv merged commit 7846127 into main Aug 8, 2026
62 of 63 checks passed
@cv
cv deleted the revert/emergency-ollama-harness-main-commits branch August 8, 2026 15:54
@ericksoa
ericksoa restored the revert/emergency-ollama-harness-main-commits branch August 8, 2026 15:54
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Blocking findings reported

Advisor assessment: Blockers require maintainer review
Next action: Review the blockers below.
Findings: 1 blocker · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 1 blocker · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: cloud-inference, cloud-onboard, security-posture, gpu-e2e, managed-image-protected-runtime

Blockers

PRA-1 Blocker — Fail when Ollama cleanup leaves a prior listener

  • Location: test/e2e/live/gpu-e2e-helpers.ts:210
  • Category: correctness
  • Problem: The cleanup command suppresses every stop failure and does not verify that the Ollama process and loopback listener are gone. The protected runtime then treats any listener on port 11434 as the daemon it started.
  • Impact: A stale system-owned Ollama service can satisfy readiness, model pull, and GPU-placement probes. The protected qualification can then test a daemon outside this run's lifecycle.
  • Fix: Restore authoritative service cleanup with explicit failure handling, and verify that no Ollama daemon or listener remains before starting the E2E daemon.
  • Verification: Inspect the cleanup command result and verify, with a controlled existing Ollama listener, that startup fails or removes the listener before its readiness probe succeeds.
  • Test coverage: Add a focused helper test that simulates a service-stop failure with a remaining Ollama listener and asserts cleanup fails before protected runtime startup accepts readiness.
  • Evidence: test/e2e/live/gpu-e2e-helpers.ts:203-215 suppresses all cleanup failures and contains no process or listener verification. test/e2e/live/managed-image-protected-runtime-helpers.ts:129-148 calls cleanupOllama, starts a daemon, and accepts any successful response from 127.0.0.1:11434. test/e2e/live/gpu-e2e.test.ts:130-135 treats cleanup exit code as success; test/e2e/live/gpu-e2e.test.ts:166-168 uses the same cleanup before installation.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@wscurran wscurran added chore Build, CI, dependency, or tooling maintenance area: e2e End-to-end tests, nightly failures, or validation infrastructure area: local-models Local model providers, downloads, launch, or connectivity provider: ollama Ollama local model provider behavior labels Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: e2e End-to-end tests, nightly failures, or validation infrastructure area: local-models Local model providers, downloads, launch, or connectivity chore Build, CI, dependency, or tooling maintenance provider: ollama Ollama local model provider behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants