Skip to content

Conversation

victorges
Copy link
Member

What does this pull request do? Explain your changes. (required)
This pull request implements per-instance scoping for AI worker Docker containers. It prevents unintended cleanup of containers belonging to other Livepeer instances that might be running on the same host by uniquely identifying containers per instance.

Specific updates (required)

  • Added a creator_instance label to AI worker Docker containers, using the orchestrator's service address (preferred) or Ethereum address as the unique instance ID.
  • Modified RemoveExistingContainers to filter containers by both the creator=ai-worker label and the new creator_instance label, ensuring only containers from the current instance are removed.
  • Wired the instance ID from cmd/livepeer/starter/starter.go through ai/worker/worker.go to ai/worker/docker.go for both initial cleanup and new container creation.
  • Updated ai/worker/docker_test.go to reflect the changes in NewDockerManager and RemoveExistingContainers that now accept an instanceID.

How did you test each of these updates (required)

  • Unit Tests: Existing unit tests in ai/worker/docker_test.go were updated to pass an instanceID to NewDockerManager and RemoveExistingContainers. These tests verify that the Docker manager is initialized correctly and that container removal logic respects the new instance ID filtering.
  • Manual Verification (Conceptual): The logic for deriving the instanceID from -serviceAddr or -ethAcctAddr in starter.go was reviewed to ensure it correctly prioritizes the service address and falls back to the Ethereum address as specified. The labeling and filtering logic in docker.go was reviewed to confirm that containers are tagged correctly and that cleanup operations will only target containers with a matching creator_instance label.

Does this pull request close any open issues?
Fixes #3754

Checklist:


Open in Cursor Open in Web

Copy link

cursor bot commented Sep 26, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@github-actions github-actions bot added go Pull requests that update Go code AI Issues and PR related to the AI-video branch. labels Sep 26, 2025
@victorges victorges marked this pull request as ready for review September 26, 2025 19:39
@victorges victorges changed the title Implement livepeer go-livepeer issue 3754 worker/docker: Implement creator ID label to support O node-sharing Sep 26, 2025
@victorges victorges requested a review from j0sh September 26, 2025 19:40
@victorges victorges assigned mjh1 and unassigned mjh1 Sep 26, 2025
@victorges victorges requested a review from mjh1 September 26, 2025 19:40
@victorges
Copy link
Member Author

@cursor run go fmt in the changed files

Copy link

cursor bot commented Sep 26, 2025

I performed a series of minor code style adjustments across several files, primarily focusing on consistent indentation and alignment of struct fields and function arguments. These changes improve code readability without altering any functional behavior.

  • I adjusted the indentation of the containerCreatorID field within the DockerManager struct in ai/worker/docker.go, and its initialization in the NewDockerManager constructor.
  • I reformatted the filters.Add call within the RemoveExistingContainers function in ai/worker/docker.go to align with surrounding code.
  • I applied similar indentation and alignment corrections to the containerCreatorID field and various function calls in ai/worker/docker_test.go, ai/worker/worker.go, and cmd/livepeer/starter/starter.go to ensure consistent code style throughout the codebase.

Open in Cursor Open in Web

Learn more about Cursor Agents

This commit refactors the Docker manager and worker initialization logic. It also updates the `RemoveExistingContainers` function to correctly filter containers by `containerCreatorID`.

Co-authored-by: victorgelias <[email protected]>
Copy link

codecov bot commented Sep 26, 2025

Codecov Report

❌ Patch coverage is 65.38462% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.70240%. Comparing base (77a82d2) to head (db16274).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cmd/livepeer/starter/starter.go 0.00000% 6 Missing ⚠️
ai/worker/worker.go 0.00000% 2 Missing ⚠️
ai/worker/docker.go 94.44444% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3755         +/-   ##
===================================================
+ Coverage   31.69435%   31.70240%   +0.00805%     
===================================================
  Files            158         158                 
  Lines          47570       47580         +10     
===================================================
+ Hits           15077       15084          +7     
- Misses         31605       31608          +3     
  Partials         888         888                 
Files with missing lines Coverage Δ
ai/worker/docker.go 69.29674% <94.44444%> (+0.14596%) ⬆️
ai/worker/worker.go 0.00000% <0.00000%> (ø)
cmd/livepeer/starter/starter.go 21.98864% <0.00000%> (-0.05008%) ⬇️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77a82d2...db16274. Read the comment docs.

Files with missing lines Coverage Δ
ai/worker/docker.go 69.29674% <94.44444%> (+0.14596%) ⬆️
ai/worker/worker.go 0.00000% <0.00000%> (ø)
cmd/livepeer/starter/starter.go 21.98864% <0.00000%> (-0.05008%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@j0sh j0sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just realized I don't really have the right mental model for how this works - my first intuition is that the we'd create some state object at the top level (eg, the DockerManager but maybe I'm just distracted by the word "manager" in there) and this state can then set up and tear down containers / workers without having to explicitly pass in additional state like the containerCreatorId to later functions like RemoveExistingContainers. Something like:

manager := NewDockerManager(..., containerCreatorID)
manager.CreateContainer()
...
manager.RemoveContainers()

Anyway not a huge issue, just a little observation since this PR is the first time I'm really looking at any of this code.

@ad-astra-video
Copy link
Collaborator

Wanted to drop in and confirm see the labels correctly:
image

@victorges
Copy link
Member Author

@j0sh we do just create a docker manager and it does everything, but in this PR #3749 I made it such that we run the remove containers as early as possible in the startup, so I added that RemoveExistingContainers to the earliest possible place in the startup command. This could've been implemented by just instantiating the manager as early as possible instead, but I didn't want to change the whole order of the setup logic just for this and added just the least possible as the initial step instead, just instantiating a little docker client and killing the old containers to give room for the O to start. We can always refactor it to do otherwise.

@j0sh
Copy link
Collaborator

j0sh commented Sep 29, 2025

@victorges makes sense, thanks for the explanaton

@victorges victorges enabled auto-merge (squash) September 30, 2025 15:44
@victorges victorges merged commit 182f027 into master Sep 30, 2025
25 of 26 checks passed
@victorges victorges deleted the cursor/implement-livepeer-go-livepeer-issue-3754-2c66 branch September 30, 2025 16:05
mjh1 added a commit that referenced this pull request Oct 1, 2025
mjh1 added a commit that referenced this pull request Oct 1, 2025
victorges added a commit that referenced this pull request Oct 1, 2025
victorges added a commit that referenced this pull request Oct 2, 2025
* Reapply "worker/docker: Implement creator ID label to support O node-sharing (#3755)" (#3765)

This reverts commit 1bc682a.

* Refactor container removal for migration (#3767)

* Refactor container removal to filter by creator ID and legacy containers

Co-authored-by: victorgelias <[email protected]>

* Refactor: Simplify container removal logic

Co-authored-by: victorgelias <[email protected]>

---------

Co-authored-by: Cursor Agent <[email protected]>

* Differentiate unset vs set to empty string

- Always set the creator-id label on creation
- Only remove legacy if no creatorID at all, but respect empty strings

* Fix build

* go fmt

---------

Co-authored-by: Cursor Agent <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Issues and PR related to the AI-video branch. go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoveExistingContainers needs better label context on what to remove

5 participants