PMM-11603 pmm-agent reload. #2410
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2410 +/- ##
==========================================
- Coverage 42.86% 42.81% -0.06%
==========================================
Files 384 382 -2
Lines 48245 47918 -327
==========================================
- Hits 20681 20516 -165
+ Misses 25620 25473 -147
+ Partials 1944 1929 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
JiriCtvrtka
requested review from
idoqo,
artemgavrilov,
ademidoff and
BupycHuk
and removed request for
a team
Co-authored-by: Alex Tymchuk <[email protected]>
agent/client/client.go
Outdated
| if req == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
It will protect only if channel is done, but in case of nil value and opened channel it will fail on nil pointer there: https://github.com/percona/pmm/pull/2410/files#diff-4aa388772db1a19eca352e250851b7785c15d428909601614ef5cc4afb325f22R370
Should I remove it anyway?
There was a problem hiding this comment.
when we might have nil value for non-closed channel?
There was a problem hiding this comment.
We could send it there, but seems there is no place like that. Removed.
| if cfg != nil { | ||
| cleanupTmp(cfg.Paths.TempDir, l) | ||
| } | ||
| cancel() |
There was a problem hiding this comment.
We still need to clean up Tmp directory
There was a problem hiding this comment.
When its cancelled it will hit cleanupTmp there: https://github.com/percona/pmm/pull/2410/files/a370bbfd23a0fa3a8a3103eaeeeaad882d32874b#diff-ae8858561a752ed8fb97d610805944a2f0123026d91e30fdbcf8c34b3ae80f5bR115
|
|
||
| for { | ||
| _, err = configStorage.Reload(l) |
There was a problem hiding this comment.
Reload is called when you use pmm-admin config subcommand. So it should not be necessary do it here, or we want to cover also case that someone will modify config manually during its running? Or there another case how you could change config?
There was a problem hiding this comment.
Yes, we want to cover that as well. We have API endpoint for that reason.
There was a problem hiding this comment.
and actually the main reason to have /reload endpoint is to be able to reload configs without restarting pmm-agent
There was a problem hiding this comment.
Am I wrong or reload for reload endpoint should be done by https://github.com/percona/pmm/pull/2410/files#diff-9bb22a7f420e8dfd90097d05536858550d6f7c00dfedada00e0606305d667db1R190 anyway? Thank you.
agent/commands/run.go
Outdated
| select { | ||
| case <-reloadCh: | ||
| return true | ||
| default: | ||
| if ctx.Err() != nil { | ||
| l.Debug(ctx.Err()) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
| select { | |
| case <-reloadCh: | |
| return true | |
| default: | |
| if ctx.Err() != nil { | |
| l.Debug(ctx.Err()) | |
| return false | |
| } | |
| } | |
| select { | |
| case <-reloadCh: | |
| return | |
| case <- ctx.Done(): | |
| return | |
| default: | |
| } |
agent/commands/run.go
Outdated
| if !isReload { | ||
| return | ||
| } |
There was a problem hiding this comment.
| if !isReload { | |
| return | |
| } | |
| select { | |
| case <- ctx.Done(): | |
| return | |
| default: | |
| } |
There was a problem hiding this comment.
With current suggested changes pmm-agent will quit in case of reload. Context is done in case of SIGINT but also in case of reload by config subcommand.
Your suggestion:
select {
case <- ctx.Done():
return
default:
}
With both suggestion above we cannot determine which case it is and it will quit main loop in both cases.
There was a problem hiding this comment.
As discussed with rootCtx its OK.
|
|
||
| for { | ||
| _, err = configStorage.Reload(l) |
There was a problem hiding this comment.
and actually the main reason to have /reload endpoint is to be able to reload configs without restarting pmm-agent
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| configFilepath, err := configStorage.Reload(l) | ||
| if err != nil { | ||
| l.Fatalf("Failed to load configuration: %s.", err) | ||
| } |
There was a problem hiding this comment.
Do we really need to reload right after storage created?
There was a problem hiding this comment.
Its inside last func before loop https://github.com/percona/pmm/pull/2410/files#diff-ae8858561a752ed8fb97d610805944a2f0123026d91e30fdbcf8c34b3ae80f5bR62
In loop config is already used so it seems its late as possible. If you have an idea feel free to suggest it.
PMM-11603
Link to the Feature Build: Percona-Lab/pmm-submodules#3382
Fix issues: