Skip to content

Refactor/remove run monitor#127

Merged
rihter007 merged 4 commits intomainfrom
refactor/remove_run_monitor
Aug 18, 2022
Merged

Refactor/remove run monitor#127
rihter007 merged 4 commits intomainfrom
refactor/remove_run_monitor

Conversation

@rihter007
Copy link
Copy Markdown
Contributor

TestRunner: Remove RunMonitor. Make all targetHandlers explicitly return errors. Make TestRunner targets a local variable.
Finally got rid of conditional variable + complex state monitoring

@rihter007 rihter007 requested a review from mimir-d August 14, 2022 18:09
@rihter007 rihter007 changed the base branch from main to refactor/remove_step_stopping_loop August 14, 2022 18:09
@rihter007 rihter007 force-pushed the refactor/remove_run_monitor branch from 6a44115 to c5c6a63 Compare August 14, 2022 18:13
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 14, 2022

Codecov Report

Merging #127 (20372df) into main (fe65fea) will decrease coverage by 0.13%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   63.61%   63.47%   -0.14%     
==========================================
  Files         164      164              
  Lines       10330    10309      -21     
==========================================
- Hits         6571     6544      -27     
- Misses       3036     3043       +7     
+ Partials      723      722       -1     
Flag Coverage Δ
e2e 49.33% <81.25%> (-0.11%) ⬇️
integration 54.79% <91.66%> (-0.14%) ⬇️
unittests 49.02% <91.66%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/runner/test_runner.go 90.11% <91.66%> (-1.44%) ⬇️
pkg/runner/step_state.go 88.57% <0.00%> (-1.91%) ⬇️
pkg/jobmanager/jobmanager.go 76.92% <0.00%> (-1.10%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rihter007 rihter007 force-pushed the refactor/remove_run_monitor branch from c5c6a63 to ef74c93 Compare August 14, 2022 20:46
Comment thread pkg/runner/test_runner.go
// It also monitors steps for critical errors and cancels the whole run.
// Note: input channels remain open when cancellation is requested,
// plugins are expected to handle it explicitly.
func (tr *TestRunner) runMonitor(ctx xcontext.Context) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

iirc, this monitor was supposed to check for plugins misbehaving (or deadlocking, since theyre considered "user code"). what's handling that now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is checked implicitly inside targetHandler when it tries to add/wait for a target result from a misbehaving step, it gets an error.
I can make the check more explicit to avoid possible errors

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, if you could add the "detect deadlock" feature to be more explicit, that'd be nice

Comment thread pkg/runner/test_runner.go Outdated
Comment thread pkg/runner/test_runner.go Outdated
Comment thread pkg/runner/test_runner.go Outdated
Comment thread pkg/runner/test_runner.go
@rihter007 rihter007 force-pushed the refactor/remove_step_stopping_loop branch 2 times, most recently from ad10ec4 to e9a9bd1 Compare August 15, 2022 20:21
Base automatically changed from refactor/remove_step_stopping_loop to main August 16, 2022 14:36
…red in stepsTargetsCount

Signed-off-by: Ilya <rihter007@inbox.ru>
…urn errors. Make TestRunner targets a local variable

Signed-off-by: Ilya <rihter007@inbox.ru>
@rihter007 rihter007 force-pushed the refactor/remove_run_monitor branch from ef74c93 to 2ef0ed9 Compare August 17, 2022 10:10
Signed-off-by: Ilya <rihter007@inbox.ru>
@rihter007 rihter007 requested a review from mimir-d August 17, 2022 10:21
…the first one

Signed-off-by: Ilya <rihter007@inbox.ru>
@rihter007 rihter007 merged commit 8e5666e into main Aug 18, 2022
@rihter007 rihter007 deleted the refactor/remove_run_monitor branch August 18, 2022 12:41
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.

3 participants