[target-allocator] Exit on invalid config #1767
Conversation
| @@ -76,6 +76,7 @@ func main() { | |||
|
|
|||
| if validationErr := config.ValidateConfig(&cfg, &cliConf); validationErr != nil { | |||
| setupLog.Error(validationErr, "Invalid configuration") | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
should we also fail fast above when we can't load the CLI config?
There was a problem hiding this comment.
We already do, it was fixed in a different PR.
I think so. It'd just be a blanket assertion that the containers don't start with some settings, but that should work as a smoke test. |
|
Added a E2E test where target allocator is enabled, but PrometheusCR is disabled, and there aren't any scrape configs defined in prometheus receiver configuration. This is a valid Collector config, but an invalid Target Allocator config, so TA doesn't start. I guess we should find a way to reject this at admission, but that requires parsing the prometheus config, which we'd rather not do. Maybe we should actually accept this configuration in TA and just do nothing instead? |
TylerHelmuth
added
the
Skip Changelog
swiatekm
force-pushed
the
fix/ta-exit-on-invalid-config
branch
from
0b2aedb to
afd4dce
Compare
|
As per our discussion during the SIG, I've changed this so the TA only exits if it can't parse the config. If we don't have any scrape configs and no PrometheusCR, we still start, but do nothing. As we plan to reject this combination at admission in the future, I'm not adding an E2E test for it @jaronoff97 . |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jaronoff97, swiatekm-sumo, TylerHelmuth The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
swiatekm
force-pushed
the
fix/ta-exit-on-invalid-config
branch
from
202b8cc to
68a3a1b
Compare
|
@swiatekm-sumo are we good to merge this (after a rebase?) |
swiatekm
force-pushed
the
fix/ta-exit-on-invalid-config
branch
from
68a3a1b to
2ad8985
Compare
swiatekm
force-pushed
the
fix/ta-exit-on-invalid-config
branch
from
2ad8985 to
fa46c0f
Compare
|
@jaronoff97 Yes, it's ready to be merged. |
Target allocator should exit if its combined configuration is invalid. This change should've been part of #1729 .