make bosh release compatible with Resolute#2755
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 (1)
WalkthroughThe PR removes the PostgreSQL 10 blob entry, updates libpq package selection and build steps to PostgreSQL 15, adds a new director worker entrypoint and BPM process definitions, changes director monit to start and stop workers through bpm, removes legacy worker control and ps utility scripts, and adjusts MariaDB Connector/C build flags for SSL verification and GCC 15 warnings. 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 |
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bpm.yml`:
- Around line 42-59: The worker queue assignment in the template loop currently
leaves non-status workers subscribed to urgent in dedicated-status mode, so only
the first worker is isolated. Update the queue selection logic in the bpm.yml
worker process generation to use worker_1 for urgent-only and ensure the
remaining workers are set to normal when director.enable_dedicated_status_worker
is enabled and director.workers > 1, while preserving normal,urgent for the
non-dedicated case.
In `@packages/mysql/packaging`:
- Around line 11-14: Keep SSL certificate verification enabled in the mysql
packaging defaults and remove the DEFAULT_SSL_VERIFY_SERVER_CERT=0 override from
the connector build flags. Update the packaging logic around the
mysql2/mariadb-connector-c compatibility path so the GCC 15 CMAKE_C_FLAGS
workaround stays, but the TLS verification behavior is left secure by default in
the packaging/consumer layer.
- Line 12: The packaging command is overwriting the toolchain’s existing C flags
with a new CMAKE_C_FLAGS value, which discards any preconfigured hardening or
architecture flags. Update the MySQL packaging setup to append the
discarded-qualifiers workaround instead of replacing the base flags, using the
packaging command path that sets the CMake flags so existing CFLAGS are
preserved.
🪄 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: ea4448c9-0e1f-46b2-89bd-981ceb862c35
📒 Files selected for processing (9)
config/blobs.ymljobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/workerjobs/director/templates/worker_ctl.erbpackages/libpq/packagingpackages/libpq/specpackages/mysql/packaging
💤 Files with no reviewable changes (2)
- jobs/director/templates/worker_ctl.erb
- config/blobs.yml
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
aramprice
left a comment
aramprice
left a comment
There was a problem hiding this comment.
seems like a reasonable change.
mkocher
force-pushed
the
resolute-compat-fixes
branch
from
83baef2 to
585709a
Compare
|
I've updated the description, but figured I should leave a comment - moving director workers to using BPM will require Docker CPI users to set a new property - cpi_additional_volumes - to allow workers to access a docker socket. I considered giving up on BPM for workers, but since this only affects docker users and can be added to the bosh-deployment ops file it seems worth the short term pain. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bpm.yml`:
- Around line 54-57: The worker process definition in the BPM template is now
passing the BPM-style name instead of the legacy worker name to the binary.
Update the `worker` entry so the BPM process can still be named
`worker_#{index}`, but the `args` passed to `bosh-director-worker` should
preserve the legacy `worker-#{index}` identifier expected by the ps/drain
matching path. Use the `worker` hash in `jobs/director/templates/bpm.yml` as the
fix point and keep the child process command line compatible with the existing
`ps_utils` expectations.
In `@jobs/director/templates/worker`:
- Around line 5-9: The worker template is pointing PATH at an undeclared
Postgres package, which can reference a package directory the job does not
install. Update the PATH setup in the worker template to use the declared
package that actually provides the Postgres client binaries, and keep the
LD_LIBRARY_PATH consistent with that same package; use the existing
POSTGRES_PACKAGE and the worker template variables to locate and correct the
package reference.
🪄 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: 425b3b6f-e693-4b7a-bbee-0f01dafcbd7b
📒 Files selected for processing (10)
config/blobs.ymljobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/workerjobs/director/templates/worker_ctl.erbpackages/libpq/packagingpackages/libpq/specpackages/mysql/packagingspec/director_templates_spec.rb
💤 Files with no reviewable changes (2)
- jobs/director/templates/worker_ctl.erb
- config/blobs.yml
mkocher
force-pushed
the
resolute-compat-fixes
branch
from
585709a to
58cb063
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 `@spec/director_templates_spec.rb`:
- Around line 551-591: The CPI unrestricted_volumes assertion only covers
worker_* processes, but the same bpm.yml logic also applies to
dynamic_disks_worker_* processes. Extend the spec in director_templates_spec.rb
to set dynamic_disks_workers to a positive value and add an expectation on a
dynamic_disks_worker process that it includes the CPI job mount (and any
additional CPI volumes if relevant), using the existing worker_process helper
and rendered process lookup to keep the coverage aligned with the director
template behavior.
🪄 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: ca16b828-29c4-4bd8-8371-334d158b7a0a
📒 Files selected for processing (13)
config/blobs.ymljobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/ps_utils.shjobs/director/templates/workerjobs/director/templates/worker_ctl.erbpackages/libpq/packagingpackages/libpq/specpackages/mysql/packagingspec/director_templates_spec.rbspec/support/ps_utils_tests.shspec/utils_spec.rb
💤 Files with no reviewable changes (5)
- spec/utils_spec.rb
- config/blobs.yml
- jobs/director/templates/worker_ctl.erb
- jobs/director/templates/ps_utils.sh
- spec/support/ps_utils_tests.sh
* workers now use BPM! * workers using BPM will require Docker CPI users to set a new property - cpi_additional_volumes - to allow workers to access a docker socket * libpq is compiled from postgres-15 - it used to be compiled from postgres-10 which does not compile on Resolute. Otto says the line protocol has not changed, so compiling the client with the newest version is not a problem * update mysql compile to not treat discarded-qualifiers as an error, which was upgraded from a warning to an error in gcc
mkocher
force-pushed
the
resolute-compat-fixes
branch
from
58cb063 to
850eefe
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/bpm.yml`:
- Line 8: The `director.cpi_additional_volumes` value used by the `bpm.yml`
template can be `nil`, which breaks later `extra_volumes.map(&:dup)` processing
during rendering. Normalize the value right where `extra_volumes` is assigned in
the template logic so `nil` becomes an empty array, keeping the property
optional and preventing deployment failures. Use the existing `extra_volumes`
handling in the `bpm.yml` template as the place to apply this defaulting.
🪄 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: 2f98c087-2122-4d78-8f54-e6f682bff2df
📒 Files selected for processing (13)
config/blobs.ymljobs/director/monitjobs/director/specjobs/director/templates/bpm.ymljobs/director/templates/ps_utils.shjobs/director/templates/workerjobs/director/templates/worker_ctl.erbpackages/libpq/packagingpackages/libpq/specpackages/mysql/packagingspec/director_templates_spec.rbspec/support/ps_utils_tests.shspec/utils_spec.rb
💤 Files with no reviewable changes (5)
- spec/utils_spec.rb
- spec/support/ps_utils_tests.sh
- jobs/director/templates/worker_ctl.erb
- config/blobs.yml
- jobs/director/templates/ps_utils.sh
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
| tar xzf postgres/postgresql-15.*.tar.gz | ||
| cd postgresql-15* |
There was a problem hiding this comment.
Can / should we keep the more generic postgresql-*.tar.gz so that we don't have to update this each time we bump PostgreSQL?
There was a problem hiding this comment.
Hmmm, does the cd break since there are postgres-13 and postgres-15 blobs?
There was a problem hiding this comment.
I think that packages/libpq/spec will restrict which package blob gets included, so the more generic glob is ok.
... updated my comment below
There was a problem hiding this comment.
I guess this one can change as long as libpq spec only has one postgres blob specified
| name: libpq | ||
| files: | ||
| - postgres/postgresql-10.*.tar.gz | ||
| - postgres/postgresql-15.*.tar.gz |
There was a problem hiding this comment.
Same as https://github.com/cloudfoundry/bosh/pull/2755/changes#r3493630463 this needs to be specific so that only one postgresql version is included
2 participants
Summary
Please provide contextual information.
What tests have you run against this PR?
Compiled and deployed a director
How should this change be described in bosh release notes?
Bosh Director can now be deployed on a Resolute Raccoon stemcell
Does this PR introduce a breaking change?
Yes. Workers using BPM will require Docker CPI users to set a new property - cpi_additional_volumes - to allow workers to access a docker socket.