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

Change check for start_retirement to not initialized vs retiring #281

Merged
merged 1 commit into from
May 7, 2018
Merged

Change check for start_retirement to not initialized vs retiring #281

merged 1 commit into from
May 7, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Apr 16, 2018

Previously this check was for retiring but with ManageIQ/manageiq#17280 we're adding a state of initializing to prevent more than one worker from entering the state machine for the same service. The previous retiring check needs to be an initializing check.

Depends on

ManageIQ/manageiq-automation_engine#173

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 16, 2018

@tinaafitz pls review

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 16, 2018

@miq-bot add_label enhancement

@@ -17,7 +17,7 @@
exit MIQ_ABORT
end

if stack.retiring?
Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u Lets put back this original code check

if stack.retiring?
  $evm.log('error', "Stack is in the process of being retired. Aborting current State Machine.")
  exit MIQ_ABORT
end

@@ -17,7 +17,7 @@
exit MIQ_ABORT
end

if stack.retiring?
unless stack.retirement_initialized
$evm.log('error', "Stack is in the process of being retired. Aborting current State Machine.")
Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u Let's change the error message to something like: "Stack has not been initialized for retirement. Aborting current State Machine."

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 17, 2018

@tinaafitz can you re-review please?

@@ -17,7 +17,7 @@
exit MIQ_ABORT
end

if vm.retiring?
Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u We need 3 separate checks in all of the start_retirement methods.
The methods already have 2 of the checks

if stack.retired?
  $evm.log('error', "Stack is already retired. Aborting current State Machine.")
  exit MIQ_ABORT
end

if stack.retiring?
  $evm.log('error', "Stack is in the process of being retired. Aborting current State Machine.")
  exit MIQ_ABORT
end

We need to add this check:

unless unless stack.retirement_initialized?
  $evm.log('error', "Stack has not been initialized for retirement. Aborting current State Machine.")
  exit MIQ_ABORT
end

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@d-m-u Could you modify the start_retirement methods to have the 3 checks as described in my last comment?

@d-m-u d-m-u changed the title Change check for start_retirement to not initialized vs retiring [WIP] Change check for start_retirement to not initialized vs retiring Apr 18, 2018
@miq-bot miq-bot added the wip label Apr 18, 2018
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@d-m-u Looks good.
@mkanoor Please review.

@@ -22,6 +22,11 @@
exit MIQ_ABORT
end

unless vm.retirement_initialized?
$evm.log('error', "VM has not been initialized for retirement. Aborting current State Machine.")
exit MIQ_ABORT
Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u Can you raise an exception instead of exiting, it helps when we test.

Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u @mkanoor and I discussed this and the exit is okay for now. We'll address the exit changes in a future PR.

@d-m-u d-m-u changed the title [WIP] Change check for start_retirement to not initialized vs retiring Change check for start_retirement to not initialized vs retiring Apr 20, 2018
@miq-bot miq-bot removed the wip label Apr 20, 2018
@miq-bot
Copy link
Member

miq-bot commented Apr 20, 2018

This pull request is not mergeable. Please rebase and repush.

@d-m-u d-m-u changed the title Change check for start_retirement to not initialized vs retiring [wip] Change check for start_retirement to not initialized vs retiring Apr 23, 2018
@miq-bot miq-bot added the wip label Apr 23, 2018
@d-m-u d-m-u changed the title [wip] Change check for start_retirement to not initialized vs retiring Change check for start_retirement to not initialized vs retiring Apr 23, 2018
@miq-bot miq-bot removed the wip label Apr 23, 2018
@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2018

Checked commit d-m-u@9aa367b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

@d-m-u
Copy link
Contributor Author

d-m-u commented May 7, 2018

@mkanoor bump

@mkanoor mkanoor merged commit 76d93f0 into ManageIQ:master May 7, 2018
@mkanoor mkanoor added this to the Sprint 85 Ending May 7, 2018 milestone May 7, 2018
@d-m-u d-m-u deleted the changing_retirement_check_to_include_initializing branch May 7, 2018 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants