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

Refactor/monitor #897

Merged
merged 6 commits into from Jan 18, 2018
Merged

Refactor/monitor #897

merged 6 commits into from Jan 18, 2018

Conversation

@mercul3s
Copy link
Contributor

@mercul3s mercul3s commented Jan 18, 2018

What is this change?

Breaks out the monitor code into a separate package for use outside of keepalived.

Why is this change necessary?

Closes #865.

Does your change need a Changelog entry?

Added a bullet point under Changed.

Do you need clarification on anything?

Nope.

Were there any complications while making this change?

The monitor and keepalive code was fairly integrated, and it took some time to understand which components should be managed by keepalived vs monitor. I also had to do a lot of mucking about with mocks in the unit and integration tests.

Don't multiply a time.Duration by time.(Second|Minutes|Hours) or else you'll get ridiculous times like 27K hours for your monitor.

mercul3s and others added 6 commits Jan 9, 2018
Monitor code now lives in backend/monitor. Initial
refactoring to move keepalive specific code into
keeaplived and abstract common functions into
handlers within monitor.go.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Added test update and failure handlers, removed entity and keepalive
specific tests and functionality.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

WIP adding monitor to keepalived

Moved deregister back to keepalived, commented out event creator for now
while rolling that functionality into keepalived.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Added monitor.New function for creating monitors

Monitor now has a config for entity, timeout, and handlers.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

WIP adding in handler funcs

Removed Monitor config as we don't need it

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Those functions need access to the message bus and store.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Removed mock handlers for testing

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Created mocking package for monitor in testing, added interface in
monitor.go, and modified MonitorFactory to use that interface.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Removed event creator

That functionality is now contained in keepalived.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Added tests for checking handler returns

Check that the monitor code will return expected values when the handler
code is run.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Updated monitorfactory, keepalive test, and mocks

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Eric Chlebek <eric@sensu.io>

Appease linter.

Apparently it's not OK to have Go variable names with leading k's.

Signed-off-by: Eric Chlebek <eric@sensu.io>
Rebase fixes

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Refactored integration test to include message bus

Fixed time math issue in monitor so that it resets for the correct
amount of time.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>

Updated changelog

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
@mercul3s mercul3s requested review from palourde and amdprophet Jan 18, 2018
Copy link
Member

@palourde palourde left a comment

giphy

@mercul3s mercul3s merged commit b57dff7 into master Jan 18, 2018
3 checks passed
3 checks passed
DCO All commits have a DCO sign-off from the author
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@palourde palourde deleted the refactor/monitor branch Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.