agent: tolerate more failure scenarios during service registration with central config enabled #6472
Conversation
rboyer
force-pushed
the
fix-central-config
branch
from
1cea19b
to
22cb5b9
Compare
There was a problem hiding this comment.
Nice job.
This is a pretty big diff and took some refactoring of the service manager and agent plumbing.
I'd say I've not reviewed it super in depth. But I have looked through all the code and commented only on a few minor things.
The tests look good at a high level but I didn't carefully analyse whether they cover every possible case etc.
I'm happy to approve, is there anything you are concerned about that should get closer scrutiny or additional manual/integration testing?
|
|
||
| return out, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Thought: I realise this is following the existing pattern, but do you think it would be worth factoring this out into a separate file? This file being big doesn't cause too many practical issues for me so maybe not worth the effort now but I wonder if all the persistence stuff in here would be better as a separate package at some point or at least a separate file in this one?
There was a problem hiding this comment.
I can move it out into its own file.
| // invoked to clear irrelevant fields. | ||
| // | ||
| // The ServiceManager.AddService signature is largely just a passthrough for | ||
| // addServiceLocked and should be treated as such. |
There was a problem hiding this comment.
🤔 my initial reaction to this was that is seems uglier than the massive argument lists it replaced and maybe the smell indicated that we should have just built this whole registration pipeline differently.
But after thinking some I'm not sure it's too bad and the alternatives don't seem obviously cleaner. I think it's OK as it is.
There was a problem hiding this comment.
If we start really using central service config more it might be worth the investment to rethink this whole Agent structure. It's not a quick fix.
…th central config enabled Also: * Finished threading replaceExistingChecks setting (from GH-4905) through service manager. * Respected the original configSource value that was used to register a service or a check when restoring persisted data. * Run several existing tests with and without central config enabled (not exhaustive yet). * Switch to ioutil.ReadFile for all types of agent persistence.
rboyer
force-pushed
the
fix-central-config
branch
from
467b30d
to
34d2d56
Compare
…th central config enabled (#6472) Also: * Finished threading replaceExistingChecks setting (from GH-4905) through service manager. * Respected the original configSource value that was used to register a service or a check when restoring persisted data. * Run several existing tests with and without central config enabled (not exhaustive yet). * Switch to ioutil.ReadFile for all types of agent persistence.
Also:
Finished threading
replaceExistingCheckssetting (from Add an option to register services and their checks idempotently #4905) through service manager.Respected the original
configSourcevalue that was used to register a service or a check when restoring persisted data.Run several existing tests with and without central config enabled (not exhaustive yet).
Switch to
ioutil.ReadFilefor all types of agent persistence.Fixes #6323