Monitoring API: Add Metric server config #2322
Conversation
|
Hello @marioferh! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
marioferh
force-pushed
the
metrics_server_config_monitoring_api
branch
from
8592b9b to
4a25999
Compare
marioferh
force-pushed
the
metrics_server_config_monitoring_api
branch
from
4a25999 to
d2369a1
Compare
|
@everettraven hi, updated, aligned https://github.com/openshift/api/pull/2148/files |
marioferh
force-pushed
the
metrics_server_config_monitoring_api
branch
from
d2369a1 to
c38a1eb
Compare
|
/retest-required |
|
/test e2e-aws-ovn-hypershift-conformance |
| // | ||
| // see: https://kubernetes.io/docs/tasks/debug-application-cluster/audit/#audit-policy | ||
| // for more information about auditing and log levels. | ||
| // +optional |
There was a problem hiding this comment.
Should this be optional?
This is the only field in the struct and we generally want to avoid being able to set something like audit: {}
I think this made sense as a required field because the parent is optional and a pointer, meaning you can detect the need to set a default audit profile when the parent is nil controller side.
I could see this field being optional if you think this struct may expand in the future to support additional configuration options where you'd want the audit profile to still default to Metadata when unspecified.
If that is the case, we should add a minimum properties constraint to the Audit struct.
There was a problem hiding this comment.
I've changed to optional because is a default value then, it is not necessary to add min properties, correct?
There was a problem hiding this comment.
If you don't think you'll be expanding the options here, lets make this a required field when specifying the parent field (audit). Not specifying audit will mean we perform defaulting behavior.
Otherwise, lets add that at least one property must be specified so that setting audit: {} is never a valid configuration.
| // Valid values are positive integers, values over 10 are usually unnecessary. | ||
| // When omitted, this means no opinion and the platform is left to choose a reasonable default, that is subject to change over time. | ||
| // The current default value is `0`. | ||
| // default means minimal logging |
There was a problem hiding this comment.
Would it make sense for us to include here what commonly used ranges of values generally refer to?
As an example, thinking along the lines of:
When set to a value in the range 0 - 2, metrics server will log pertinent information, warnings, and errors.
When set to a value in the range 2 - 4, metrics server will log request information, all errors, and some warnings.
...
Without something like that included here, I wouldn't really know what value is reasonable to put here and what information I'll get out of that.
Building on this, I wonder if instead of accepting a numeric value we should follow the Alertmanager configuration approach and have more human-readable values which we assign an arbitrary verbosity value in the controller that stamps out the metrics server configuration?
There was a problem hiding this comment.
done.
You are right, but the config of metric server is numeric:
`Logging flags:
--log-flush-frequency duration Maximum number of seconds between log flushes (default 5s)
--log-json-info-buffer-size quantity [Alpha] In JSON format with split output streams, the info messages can be buffered for a while to increase performance. The default value of zero bytes disables buffering. The size can be specified as number of bytes (512), multiples of 1000 (1K), multiples of 1024 (2Ki), or powers of those (3M, 4G, 5Mi, 6Gi). Enable the LoggingAlphaOptions feature gate to use this.
--log-json-split-stream [Alpha] In JSON format, write error messages to stderr and info messages to stdout. The default is to write a single stream to stdout. Enable the LoggingAlphaOptions feature gate to use this.
--log-text-info-buffer-size quantity [Alpha] In text format with split output streams, the info messages can be buffered for a while to increase performance. The default value of zero bytes disables buffering. The size can be specified as number of bytes (512), multiples of 1000 (1K), multiples of 1024 (2Ki), or powers of those (3M, 4G, 5Mi, 6Gi). Enable the LoggingAlphaOptions feature gate to use this.
--log-text-split-stream [Alpha] In text format, write error messages to stderr and info messages to stdout. The default is to write a single stream to stdout. Enable the LoggingAlphaOptions feature gate to use this.
--logging-format string Sets the log format. Permitted formats: "json" (gated by LoggingBetaOptions), "text". (default "text")
-v, --v Level number for the log level verbosity
--vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format)
`
There was a problem hiding this comment.
The metric server may be numeric, but we don't have to follow that from the operator configuration perspective. In my opinion, we should focus on UX over choosing something that maps 1:1 with what the CLI option is for metrics-server.
For example, in the operator we can have a translation layer that takes something like Info and maps it to a -v=2.
I don't know what the best value options to put here are, but something like
Line 60 in 7605f9b
| // | ||
| // see: https://kubernetes.io/docs/tasks/debug-application-cluster/audit/#audit-policy | ||
| // for more information about auditing and log levels. | ||
| // +optional |
There was a problem hiding this comment.
If you don't think you'll be expanding the options here, lets make this a required field when specifying the parent field (audit). Not specifying audit will mean we perform defaulting behavior.
Otherwise, lets add that at least one property must be specified so that setting audit: {} is never a valid configuration.
config/v1alpha1/tests/clustermonitoring.config.openshift.io/ClusterMonitoringConfig.yaml
Outdated
Show resolved
Hide resolved
|
/retest-required |
marioferh
force-pushed
the
metrics_server_config_monitoring_api
branch
from
f88b3e5 to
6dfbbf6
Compare
|
The API looks good to me, but we have a lint failure that needs to be resolved. |
not sure why I have different issues with linter in local and in the CI, maybe different scripts? |
|
/retest-required |
Make sure you've rebase, make sure your code is checked out under |
Signed-off-by: Mario Fernandez <[email protected]>
marioferh
force-pushed
the
metrics_server_config_monitoring_api
branch
from
928d99d to
557334f
Compare
|
/approve Overriding |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, marioferh 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 |
openshift-ci
bot
added
the
approved
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@marioferh: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
First PR: #1929
Related: Enhancements Proposal openshift/enhancements#1627