-
Notifications
You must be signed in to change notification settings - Fork 693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add operator config reference #1798
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|Flag |Type|Default|Description | ||
|
||
|log-verbosity |int |0 |Verbosity level of logs. -2=Error, -1=Warn, 0=Info, >0=Debug | ||
|enable-debug-logs |bool |false |Enables debug logs. Equivalent to `log-verbosity=1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to that PR: that 2nd option and the first one feel weird. Looks like the first option would be enough? I don't remember if this is something we discussed @charith-elastic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-kow raised it in the original PR but we didn't really go into detail, might be worth revisiting
#1699 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, happy to merge this as is, but I realised while reading this that we probably have to add some more explanation about the operator roles (and maybe clean up our current implementation a bit)
docs/operator-config.asciidoc
Outdated
|log-verbosity |int |0 |Verbosity level of logs. -2=Error, -1=Warn, 0=Info, >0=Debug | ||
|enable-debug-logs |bool |false |Enables debug logs. Equivalent to `log-verbosity=1` | ||
|metrics-port |int |0 |Port to use for exposing metrics in the Prometheus format. Set 0 to disable | ||
|operator-roles |[]string |all |Roles this operator should assume. Valid values are namespace, global, webhook or all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a bit more explanation, maybe in a separate paragraph outside of this table. E.g. its important to note that namespace
is not the same as a setting --namespace=my-ns
and global
will not give you a fully working operator (unless combined with another instance running with --operator-roles=namespace
.
Even more counterintuitively if you want to restrict the operator to one namespace you have to run it with --operator-roles=global,namespace --namespace=my-namespace
(excluding the webhook role because I assume in single namespace mode the operator won't have permissions to register webhooks)
Finally in all likelyhood for the next release webhook
will be effectively a NOOP (unless we fix the webhooks in #1769 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally in all likelyhood for the next release webhook will be effectively a NOOP (unless we fix the webhooks in #1769 )
I figured I would at least document it as our desired state and we can modify it as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I just think that the doc is wrong regarding the metric server.
docs/operator-config.asciidoc
Outdated
|log-verbosity |int |0 |Verbosity level of logs. -2=Error, -1=Warn, 0=Info, >0=Debug | ||
|enable-debug-logs |bool |false |Enables debug logs. Equivalent to `log-verbosity=1` | ||
|metrics-port |int |0 |Port to use for exposing metrics in the Prometheus format. Set 0 to disable | ||
|operator-roles |[]string |all |Roles this operator should assume. Valid values are namespace, global, webhook or all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure that it's obvious for everyone that it could be some comma separated values (e.g. --operator-roles=global,namespace
)
docs/operator-config.asciidoc
Outdated
|
||
|log-verbosity |int |0 |Verbosity level of logs. -2=Error, -1=Warn, 0=Info, >0=Debug | ||
|enable-debug-logs |bool |false |Enables debug logs. Equivalent to `log-verbosity=1` | ||
|metrics-port |int |0 |Port to use for exposing metrics in the Prometheus format. Set 0 to disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default metric port is 8080, it means that it is enabled by default:
cloud-on-k8s/cmd/manager/main.go
Line 48 in 8f7265b
DefaultMetricPort = 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch
@pebrc can you take a look at the extra paragraph? It might need even more information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! 👍
Related: #1379
Adds reference documentation for operator configuration options. Deeeeefinitely open to presentation and wording feedback