ingress: Add ingress operator and operand logging level API #806
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
This enhancement describes the API and code changes necessary to expose a means to change the Ingress Operator and OpenShift Router's Logging Levels to cluster administrators. This enhancement is in support of https://issues.redhat.com/browse/NE-366.
|
|
||
| ``` | ||
|
|
||
| This new field would allow a clsuter administrator to specify the desired logging level of an IngressController's openshift-router Go program, in addition |
There was a problem hiding this comment.
| This new field would allow a clsuter administrator to specify the desired logging level of an IngressController's openshift-router Go program, in addition | |
| This new field would allow a cluster administrator to specify the desired logging level of an IngressController's openshift-router Go program, in addition |
| Trace LogLevel = "Trace" | ||
|
|
||
| // TraceAll is used when something is broken at the level of API content/decoding. It will dump complete body content. If you turn this on in a production cluster | ||
| // prepare from serious performance issues and massive amounts of logs. In kube, this is probably glog=8. |
There was a problem hiding this comment.
| // prepare from serious performance issues and massive amounts of logs. In kube, this is probably glog=8. | |
| // prepare for serious performance issues and massive amounts of logs. In kube, this is probably glog=8. |
|
|
||
| var ( | ||
| // Normal is the default. Normal, working log information, everything is fine, but helpful notices for auditing or common operations. In kube, this is probably glog=2. | ||
| Normal LogLevel = "Normal" |
There was a problem hiding this comment.
Maybe use "Info" rather than "Normal", it is a more common term.
There was a problem hiding this comment.
These values are already defined in https://github.com/openshift/api/blob/master/operator/v1/types.go. The enhancement document should say more explicitly that these definitions are repeated here for reference but are already defined in the API.
| Supporting a trivial way to raise the verbosity of the Ingress Operator and it's Operands (IngressControllers) would make debugging | ||
| cluster Ingress issues easier for cluster administrators and OpenShift developers. |
There was a problem hiding this comment.
| Supporting a trivial way to raise the verbosity of the Ingress Operator and it's Operands (IngressControllers) would make debugging | |
| cluster Ingress issues easier for cluster administrators and OpenShift developers. | |
| Supporting a trivial way to raise the verbosity of the Ingress Operator and its operands (IngressControllers) would make debugging | |
| cluster ingress issues easier for cluster administrators and OpenShift developers. |
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Ingress Operator](https://github.com/openshift/cluster-ingress-operator) | ||
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Router's](https://github.com/openshift/router) sub components. |
There was a problem hiding this comment.
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Ingress Operator](https://github.com/openshift/cluster-ingress-operator) | |
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Router's](https://github.com/openshift/router) sub components. | |
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Ingress Operator](https://github.com/openshift/cluster-ingress-operator). | |
| * Add a user-facing API for controlling the run-time verbosity of the [OpenShift Router's](https://github.com/openshift/router) subcomponents. |
| // operatorLogLevel controls the logging level of the Ingress Operator. | ||
| // See LogLevel for more information about each available logging level. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
We could use // +kubebuilder:default=Normal here, and we should avoid referring to Go types in the godoc. We can copy and paste the description of the field's possible values from OperatorSpec:
| // operatorLogLevel controls the logging level of the Ingress Operator. | |
| // See LogLevel for more information about each available logging level. | |
| // | |
| // +optional | |
| // operatorLogLevel controls the logging level of the Ingress Operator. | |
| // | |
| // Valid values are: "Normal", "Debug", "Trace", "TraceAll". | |
| // Defaults to "Normal". | |
| // | |
| // +optional | |
| // +kubebuilder:default=Normal |
| // logLevel describes the logging verbosity of the IngressController. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
Again, copy and paste godoc describing the logLevel field's possible and default values from OperatorSpec:
| // logLevel describes the logging verbosity of the IngressController. | |
| // | |
| // +optional | |
| // logLevel describes the logging verbosity of the IngressController. | |
| // | |
| // Valid values are: "Normal", "Debug", "Trace", "TraceAll". | |
| // Defaults to "Normal". | |
| // | |
| // +optional | |
| // +kubebuilder:default=Normal |
|
|
||
| var ( | ||
| // Normal is the default. Normal, working log information, everything is fine, but helpful notices for auditing or common operations. In kube, this is probably glog=2. | ||
| Normal LogLevel = "Normal" |
There was a problem hiding this comment.
These values are already defined in https://github.com/openshift/api/blob/master/operator/v1/types.go. The enhancement document should say more explicitly that these definitions are repeated here for reference but are already defined in the API.
| Setting the logging level for IngressController pods should not cause the IngressController pods to rollout. | ||
| This is expected behavior whenever an IngressController's pod template's environment variables are changed. |
There was a problem hiding this comment.
"This" is ambiguous. Maybe this wording is a little clearer:
| Setting the logging level for IngressController pods should not cause the IngressController pods to rollout. | |
| This is expected behavior whenever an IngressController's pod template's environment variables are changed. | |
| OpenShift router currently has a command-line option and environment variables to configure logging levels. | |
| However, changing a command-line option or environment variable in an IngressController's pod template would | |
| cause a rolling update of the IngressController pods. This enhancement should be implemented in a way that | |
| avoids this rolling update. |
| In theory, this would make debugging Ingress issues easier, as the Router logging level can be raised "on the fly" whenever a new issue is observed. | ||
| Presumably, in some cases, rolling out new router pods may "reset" the issue and make it harder to observe without a reliable reproducer handy. | ||
|
|
||
| It is worth noting the openshift-sdn team has already taken a similar approach for controlling the logging verbosity of some OpenShift networking components. |
There was a problem hiding this comment.
It would be useful to add a reference to the design or implementation of this approach.
| The Ingress Operator has end-to-end tests; for this enhancement, the following e2e tests will be added: | ||
|
|
||
| 1. A test where the Ingress Operator's logging level is increased via the Ingress Config API. | ||
| 1. A test where the IngressController's logging level is increased via the IngerssController API. |
There was a problem hiding this comment.
| 1. A test where the IngressController's logging level is increased via the IngerssController API. | |
| 1. A test where the IngressController's logging level is increased via the IngressController API. |
| * As an OpenShift support engineer, I want to have a means of quickly gathering detailed HAProxy access logs, in addition to detailed openshift-router logs, from customers | ||
| who are running into Ingress issues in production. | ||
|
|
||
| ### Implementation Details/Notes/Constraints [optional] |
There was a problem hiding this comment.
The enhancement should describe how configuring the operator's log level will be implemented, at least a short paragraph.
uber-go/zap#261 (comment) has example code for configuring the log level at runtime using Zap, so it looks like we just need to change how we set up the logger when the operator starts, and then the operator needs to watch the Ingress config (which it already does) and call the SetLevel method when the log level is changed in the Ingress config.
There was a problem hiding this comment.
Whoops! I should have scrolled just a bit further, and I'd've seen this comment: uber-go/zap#261 (comment)
...which points to a slightly different API for dynamically setting Zap's log level: https://godoc.org/go.uber.org/zap#AtomicLevel
|
We'll merge this PR, and whoever takes the enhancement over from Steve can make a follow-up PR. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah 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
|
/retitle ingress: Add ingress operator and operand logging level API |
openshift-ci
bot
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
This enhancement describes the API and code changes
necessary to expose a means to change the Ingress Operator
and OpenShift Router's Logging Levels to cluster administrators.
This enhancement is in support of https://issues.redhat.com/browse/NE-366.