Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow BBR post unlock to proceed if CC api is remotely available #311

Merged

Conversation

evanfarrar
Copy link
Member

Previously the unlock health check failed if any single VM was unhealthy because it used the healthz endpoint over a local route

Thanks for contributing to the capi_release. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

  • An explanation of the use cases your change solves

  • Links to any other associated PRs

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the develop branch

  • I have run CF Acceptance Tests on bosh lite

Previously the unlock health check failed if any single VM was
unhealthy because it used the healthz endpoint over a local route
Copy link
Member

Choose a reason for hiding this comment

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

@@ -17,6 +17,10 @@ source /var/vcap/packages/capi_utils/syslog_utils.sh
<% (1..(p("cc.jobs.local.number_of_workers"))).each do |index| %>
monit_start_job cloud_controller_worker_local_<%= index %>
<% end %>
wait_for_server_to_become_healthy <%= "localhost:#{p("cc.external_port")}/healthz" %> <%= p("cc.post_bbr_healthcheck_timeout_in_seconds") %>
sleep 30
set +e
Copy link
Member

@sethboyles sethboyles Apr 24, 2023

Choose a reason for hiding this comment

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

I wonder if this script is even set to -e because #L13 might return 0 after setting +e

Copy link
Member

@sethboyles sethboyles left a comment

Choose a reason for hiding this comment

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

Might want a tech debt story to refactor the set -e stuff at some point, but LGTM

@tjvman tjvman merged commit 6bab5ca into cloudfoundry:develop Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants