Reload CustomResourceState Config File on Change #1928
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
approved
mrueg
force-pushed
the
hotreloading-crs
branch
7 times, most recently
from
b4500fa
to
03f8459
Compare
|
/triage accepted |
k8s-ci-robot
added
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
internal/wrapper.go
Outdated
| // WatchConfig is taken and adapted from https://github.com/spf13/viper/blob/b89e554a96abde447ad13a26dcc59fd00375e555/viper.go#L429 MIT-Licensed | ||
| func WatchConfig(filename string, onConfigChange func(fsnotify.Event)) { |
There was a problem hiding this comment.
Couldn't we just create a Viper config for the custom resource config and reuse the original function?
There was a problem hiding this comment.
I'm not sure if Viper is able to handle multiple configs at the same time and independently.
There was a problem hiding this comment.
We could create new viper instances with viper.New() and stop using the global viper config for the general config.
A benefit to reusing viper directly here is that we would be able to use viper.ReadInConfig() and viper.ReadInConfig() like we do for the general config.
There was a problem hiding this comment.
I added two instances of viper now, so we can avoid using the adapted function
mrueg
force-pushed
the
hotreloading-crs
branch
from
03f8459
to
07c2ab6
Compare
k8s-ci-robot
removed
the
lgtm
mrueg
force-pushed
the
hotreloading-crs
branch
from
07c2ab6
to
e510f4a
Compare
mrueg
force-pushed
the
hotreloading-crs
branch
from
e510f4a
to
96ec001
Compare
This change adds hot reloading support for the customresourcestate config file. It also resolves a bug in which the customresourcestate config file was included in the ksm config file, in which it did not get detected. It also resolves a bug in which customresourcestatemetrics were not added when set resources were non-default resources. Fixes: kubernetes#1892
mrueg
force-pushed
the
hotreloading-crs
branch
from
96ec001
to
06268ab
Compare
| // Merge configFile values with opts so we get the CustomResourceConfigFile from config as well | ||
| configFile, err := os.ReadFile(filepath.Clean(file)) | ||
| if err != nil { | ||
| klog.ErrorS(err, "Custom Resource State Metrics file could not be opened") | ||
| klog.ErrorS(err, "failed to read options configuration file", "file", file) | ||
| } | ||
|
|
||
| yaml.Unmarshal(configFile, opts) |
There was a problem hiding this comment.
Since we do not need the entire config to be unmarshaled, could we perhaps grab only the CustomResourceConfigFile value with something like:
cfgViper.GetString("custom-resource-state-config-file")There was a problem hiding this comment.
I believe this would require further logic (as it could also be set via command line), so merging seemed easier (to use the same logic that we have in
kube-state-metrics/pkg/app/server.go
Line 129 in b7a7070
|
Leaving lgtm up to @rexagod /approve |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, mrueg, rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -85,11 +87,10 @@ func RunKubeStateMetricsWrapper(ctx context.Context, opts *options.Options, fact | |||
| // RunKubeStateMetrics will build and run the kube-state-metrics. | |||
| // Any out-of-tree custom resource metrics could be registered by newing a registry factory | |||
| // which implements customresource.RegistryFactory and pass all factories into this function. | |||
| func RunKubeStateMetrics(ctx context.Context, opts *options.Options, factories ...customresource.RegistryFactory) error { | |||
There was a problem hiding this comment.
Hi @mrueg, I found that this parameter factories ...customresource.RegistryFactory is deleted from function RunKubeStateMetrics.
As #1644 shows, this parameter is for some repository which does not use custom resource config file and wants to add their own metrics collecting logic in their codes. So a user could construct a new repository and vendor KSM as a dependency in go.mod. I believe some users have already built their project like this, e.g. #1644 (comment).
So do you have any ideas about this scenario?
What this PR does / why we need it:
This change adds hot reloading support for the customresourcestate config file.
It also resolves a bug in which the customresourcestate config file was
included in the ksm config file, in which it did not get detected.
It also resolves a bug in which customresourcestatemetrics were not
added when resources were non-default resources.
Code for this file change detection was reused from https://github.com/spf13/viper licensed under MIT license.
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
None
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1892