Fix pseudo heartbeating when HB file missing #15483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@miq-bot assign @gtanzillo |
chessbyte
changed the title
jrafanie
reviewed
Jun 30, 2017
|
|
||
| around do |example| | ||
| ENV["WORKER_HEARTBEAT_METHOD"] = "file" | ||
| ENV["WORKER_HEARTBEAT_FILE"] = "/path/to/worker.hb" |
There was a problem hiding this comment.
If you need to rebase/fix a test, can you put this path into a let to DRY up some of the expectations below?
There was a problem hiding this comment.
@jrafanie I thought lets weren't able to be accessed within an "around" block, but I could be wrong. But that was my rational for it not being DRY.
There was a problem hiding this comment.
I was wrong... 😞
Will change.
jrafanie
reviewed
Jun 30, 2017
| expect(File).to receive(:exist?).with("/path/to/worker.hb") | ||
| .and_return(true, true) | ||
| expect(File).to receive(:mtime).with("/path/to/worker.hb") | ||
| .and_return(first_heartbeat, first_heartbeat+1) |
jrafanie
reviewed
Jun 30, 2017
| # 1. Sets the initial heartbeat | ||
| # 2. Doesn't update the worker's last_heartbeat value | ||
| # | ||
| # But the result should not change after the first loop |
jrafanie
reviewed
Jun 30, 2017
| miq_server.validate_heartbeat(worker) | ||
| end | ||
|
|
||
| expect(worker.reload.last_heartbeat).to eq(first_heartbeat+i) |
There was a problem hiding this comment.
You need to freeze time at first_heartbeat for this test so these expectations pass.
Same for the ones below.
|
LGTM so far @NickLaMuro, just minor test fixups. Nice find! |
gtanzillo
approved these changes
Jun 30, 2017
With the previous version of this code, the validate_heartbeat code:
def validate_heartbeat(w)
last_heartbeat = workers_last_heartbeat(w)
if w.last_heartbeat.nil?
last_heartbeat ||= Time.now.utc
w.update_attributes(:last_heartbeat => last_heartbeat)
elsif !last_heartbeat.nil? && last_heartbeat > w.last_heartbeat
w.update_attributes(:last_heartbeat => last_heartbeat)
end
end
Would update the `last_heartbeat` value for the worker every time
`validate_heartbeat` was called, which is not the intent.
When working correctly, the validate_heartbeat code should:
* If no value is set in the DB, initialize it to either the time
pulled from workers_last_heartbeat, or the current time (this should
only happen the first time this method is called for this worker)
* If workers_last_heartbeat is not nil, and it is a more resent
heartbeat time, update the DB with the new value
Otherwise, assume we have the most up to date heartbeat value in the DB.
This change just returns `nil` if we don't have anything to provide from
the heartbeat file. This is fine since the `validate_heartbeat` method
will take care of any initialization of the last_heartbeat value in the
DB, so workers_last_heartbeat_to_file is just an existence check, plus a
value if there is one.
NickLaMuro
force-pushed
the
fix_heartbeat_to_file_bug
branch
from
981f547
to
abd1b70
Compare
|
Checked commit NickLaMuro@abd1b70 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@jrafanie @gtanzillo Made a few updates:
|
jrafanie
reviewed
Jun 30, 2017
| miq_server.validate_heartbeat(worker) | ||
| end | ||
|
|
||
| expect(worker.reload.last_heartbeat).to be_within(1.second).of(first_heartbeat + i) |
jrafanie
approved these changes
Jun 30, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the previous version of this code, the validate_heartbeat code:
Would update the
last_heartbeatvalue for the worker every timevalidate_heartbeatwas called, which is not the intent.When working correctly, the validate_heartbeat code should:
Otherwise, assume we have the most up to date heartbeat value in the DB.
This change just returns
nilif we don't have anything to provide from the heartbeat file. This is fine since thevalidate_heartbeatmethod will take care of any initialization of the last_heartbeat value in the DB, so workers_last_heartbeat_to_file is just an existence check, plus a value if there is one.Links