make bpm optional for bosh director workers#2771
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds the Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobs/director/templates/worker_ctl.erb`:
- Around line 22-26: Update the legacy worker_ctl launcher command to export the
same QUEUE and RUBY_THREAD_* environment variables provided by BPM before
invoking bosh-director-worker. Use the worker configuration and existing symbols
in worker.rb as the source of truth, preserving the current non-BPM startup
command and log redirection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3ec6044-e044-4872-8e03-ebb60f1cb5fa
📒 Files selected for processing (4)
jobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/worker_ctl.erb
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Pull request overview
This PR adds a switch to optionally run BOSH Director worker processes outside BPM, intended to support environments (e.g., warden CPI in nested/containerized setups) where BPM isolation prevents required host access (like /dev).
Changes:
- Added
director.use_bpm_for_workersproperty (defaulttrue) to toggle BPM management of workers. - Updated the director BPM template and monit config to conditionally manage worker processes via BPM vs. monit.
- Introduced a new
worker_ctlhelper script to start/stop worker processes when not using BPM.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| jobs/director/templates/worker_ctl.erb | Adds a monit-managed worker start/stop script for the non-BPM path. |
| jobs/director/templates/bpm.yml | Makes worker process entries conditional on director.use_bpm_for_workers. |
| jobs/director/spec | Adds the new director.use_bpm_for_workers property and installs worker_ctl. |
| jobs/director/monit | Conditionally starts/stops workers via BPM or worker_ctl based on the new property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b2075c9 to
9f38f59
Compare
aramprice
left a comment
aramprice
left a comment
There was a problem hiding this comment.
Can you close or address the dangling copilot comments
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobs/director/templates/worker_ctl.erb`:
- Around line 53-60: Update the worker launch flow in the start case of
worker_ctl.erb to run the setpriv-wrapped bosh-director-worker in the
background, remove the foreground exec behavior, and write the spawned process
ID from $! to PIDFILE instead of the shell’s $$ so Monit can return successfully
after startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12494a2c-f8fc-4550-8faf-8997ecf8a6b1
📒 Files selected for processing (4)
jobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/worker_ctl.erb
9f38f59 to
7f9bf0e
Compare
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
7f9bf0e to
e20dd90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobs/director/templates/worker_ctl.erb`:
- Around line 69-75: Update list_child_processes so it filters using the parent
PID rather than the function’s name argument, preferably by using pgrep -P with
the PID passed by callers. If retaining the pipeline, remove the ineffective
grep -v ^$1$ check and prevent self-matching with [b]osh-director-worker or an
equivalent grep exclusion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7e5c9fc-1c1e-4b1e-8060-687f4092bd66
📒 Files selected for processing (4)
jobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/worker_ctl.erb
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
The warden CPI does not work under BPM for multiple reasons: * needs priveledged container to run sudo, but then must setpriv to run as vcap so the blobstore-config can be accessed by the director * needs /dev/ bind mounted into access loopback devices, but bind mounting /dev in causes other problems Long term deprecating the warden CPI in favor of the docker CPI makes the most sense. In the short term this will do.
4b9483f
e20dd90 to
4b9483f
Compare
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
the warden cpi does not work when run under BPM, so set the bosh property to disable it. cloudfoundry/bosh#2771
The warden CPI does not work under BPM for multiple reasons:
Long term deprecating the warden CPI in favor of the docker CPI makes the most sense. In the short term this will do.
Does this PR introduce a breaking change?
No