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

Add function to check if nodes are reachable via bolt #433

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bastelfreak
Copy link
Collaborator

At the moment the plans assume that all nodes are available. I had a few customer setups where one of the compilers wasn't reachable during a convert/upgrade. To not put the PE infra into an undefined state, it makes sense to check the availability before running the plans.

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.

Changes include test coverage?

  • Yes
  • Not needed

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

functions/check_availability.pp Outdated Show resolved Hide resolved
plans/convert.pp Outdated Show resolved Hide resolved
plans/convert.pp Outdated Show resolved Hide resolved
#
# @author Tim Meusel <[email protected]>
#
function peadm::check_availability(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not peadm specific and I think other plans could benefit from it as well. But I'm not sure which module would be a good place for such a generic function. I think bolt has no generic module where we could add it? Maybe stdlib or extlib are good candidates?

true => "${messages.join("\n")}\n${end_message}",
false => $end_message,
}
fail_plan($fail_message)
Copy link

Choose a reason for hiding this comment

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

Something like this I meant above:

fail_plan('Some targets are not reachable', 'peadm/unreachable-nodes', error_set => $check_result.error_set)

@@ -5,6 +5,7 @@

describe 'basic functionality' do
it 'runs successfully with the minimum required parameters' do
allow_out_message
Copy link

Choose a reason for hiding this comment

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

I guess this is missing:

expect_plan('peadm::check_availability')

Copy link
Contributor

Choose a reason for hiding this comment

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

@bastelfreak this too

At the moment the plans assume that all nodes are available. I had a few
customer setups where one of the compilers wasn't reachable during a
convert/upgrade. To not put the PE infra into an undefined state, it
makes sense to check the availability before running the plans.
) >> Integer {
$check_result = wait_until_available($targets, wait_time => 2, _catch_errors => true)
unless $check_result.ok {
$end_message = "${check_result.error_set.count} targets are not reachable, stopping plan"
Copy link
Contributor

@Jo-Lillie Jo-Lillie Jun 10, 2024

Choose a reason for hiding this comment

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

Hi @bastelfreak I was chatting to our Docs and he has suggested this change for the messaging;
Bolt cannot reach the following targets: [email protected], [email protected]
<installation conversion upgrade> cannot be continued
Exiting

Is it possible to update it like this?

@Jo-Lillie
Copy link
Contributor

Hey @bastelfreak just wondering if you got a chance to check out the comments I left? Thanks 😃

@CoMfUcIoS
Copy link
Contributor

Hey @bastelfreak this PR is stall, can you please follow up with all comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants