-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(tracing)!: Improve control plane tracing configuration #14557
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
Conversation
This clarifies the tracing documentation that the trace collector must be meshed, along with the specifics of how the service account name for the collector should be set. Signed-off-by: Scott Fleener <[email protected]>
This separates out the service account namespace from the name to make it clearer how to set the correct service account for the trace collector. Signed-off-by: Scott Fleener <[email protected]>
…lector mesh identity Signed-off-by: Scott Fleener <[email protected]>
The current control plane tracing relies on the linkerd-jaeger extension, and does not work when using the native tracing configuration. This removes the previous configs, and adds new control plane tracing config that mirrors the existing proxy tracing configs. The previous configuration was meant entirely for internal testing purposes, and shouldn't be subject to any breaking change guarantees. Signed-off-by: Scott Fleener <[email protected]>
839876c
to
7f43cf4
Compare
// PodDisruptionBudget contains the fields to set the PDB | ||
PodDisruptionBudget struct { | ||
MaxUnavailable int `json:"maxUnavailable"` | ||
MaxUnavailable string `json:"maxUnavailable"` |
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.
This is a little surprising. Why is this necessary?
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.
This field can also be a percentage value, which can't parse as an int. Not sure if there's a better way to encode this
# -- Service account namespace for the trace collector. If there's no explicitly set service account for the | ||
# trace collector, this should be set to the namespace of the deployment/statefulset/daemonset of the trace | ||
# collector instead. | ||
serviceAccountNamespace: "" |
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.
serviceAccountNamespace: "" | |
namespace: "" |
blegh, sorry to yakshave this, but we are probably better off just calling this 'namespace'
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.
WDYT about "name" and "namespace" for the fields? That lets us leave the old one in with the same semantics to avoid a breaking change, and have a less confusing name since "Service account" isn't exactly the right name for this.
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.
Sorry, I think I'm partial to serviceAccountName
and namespace
here:
- these are how these fields are named on pod spec/metadata resources, so it's consistent
- the 'name' is ambiguous. i think it's clearer if we document that this is explicitly a serviceAccountName (as on the pod spec).
- the namespace is more general -- it includes the workloads and the services blah blah blah
# collector, this should be set to the name of the deployment/statefulset/daemonset of the trace collector | ||
# instead. |
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.
is this true? if the collector doesn't specify a service account, I thought it used the default service account for it's namespace.
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.
From all of my testing, the value here is correct. However, I'm not entirely sure "service account" is the most accurate name, since this is actually the name of the mesh identity the collector's proxy uses (which is either the service account or deployment/etc. name if not present).
# -- Service account namespace for the trace collector. If there's no explicitly set service account for the | ||
# trace collector, this should be set to the namespace of the deployment/statefulset/daemonset of the trace | ||
# collector instead. | ||
serviceAccountNamespace: "" |
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.
Prior to this change, did the namespace need to be specified as part of the serviceAccountName
? Is this breaking change necessary?
Furthermore, we're introducing a new, required value here so this breaking change will affect everyone who has proxy.tracing
enabled.
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.
The namespace did need to be specified before this, in the format <service account>.<namespace>
. I'd rather we have two separate fields with a clearer intent.
WDYT about #14557 (comment) as a better state?
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.
In that case, can we leave serviceAccountName in the values.yaml but document it as deprecated and that the name/namespace fields are preferred but that it still exists for backwards compatibility. Let's leave a paper trail of why there are two different configuration methods here.
# Configures tracing in the controllers and how traces are exported | ||
tracing: | ||
# -- Enables trace collection and export in the proxy | ||
enable: false |
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.
any point to having this boolean vs just looking at if collector.endpoints is empty?
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 vaguely remember @alpeb suggesting an explicit enable/disable field, but I don't remember the specifics of why.
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 can't remember either 😆
Looking through #13994, there was discussion around not using boolean values, and validating that tracing.enable
is consistent with the contents of collector.endpoint
in the proxy config below. Anyways, relying on collector.endpoints
emptiness sounds like a simpler approach to me too, but would be good to use the same approach here and in the proxy section.
Signed-off-by: Scott Fleener <[email protected]>
…e fields Signed-off-by: Scott Fleener <[email protected]>
# Conflicts: # cli/cmd/install_test.go # pkg/charts/linkerd2/values.go
if you will pardon the drive-by nitpick; i've renamed this to follow the conventional commit spec for breaking changes with a specified scope. |
Signed-off-by: Scott Fleener <[email protected]>
BUILD.md
Outdated
flag. | ||
[Distributed Tracing](https://opentracing.io/docs/overview/what-is-tracing/) | ||
for development purposes. It can be enabled globally i.e Control plane | ||
components and their proxies by using the |
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: there's an extra space character after "components"
# -- Service account namespace for the trace collector. If there's no explicitly set service account for the | ||
# trace collector, this should be set to the namespace of the deployment/statefulset/daemonset of the trace | ||
# collector instead. | ||
serviceAccountNamespace: "" |
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.
In that case, can we leave serviceAccountName in the values.yaml but document it as deprecated and that the name/namespace fields are preferred but that it still exists for backwards compatibility. Let's leave a paper trail of why there are two different configuration methods here.
Signed-off-by: Scott Fleener <[email protected]>
# Conflicts: # cli/cmd/install_test.go # pkg/charts/linkerd2/values.go # pkg/charts/linkerd2/values_test.go
Signed-off-by: Scott Fleener <[email protected]>
Signed-off-by: Scott Fleener <[email protected]>
# Conflicts: # cli/cmd/install_test.go # pkg/charts/linkerd2/values.go
Signed-off-by: Scott Fleener <[email protected]>
Signed-off-by: Scott Fleener <[email protected]>
Signed-off-by: Scott Fleener <[email protected]>
# Conflicts: # cli/cmd/install_test.go # pkg/charts/linkerd2/values.go # pkg/charts/linkerd2/values_test.go
The current control plane tracing relies on the linkerd-jaeger extension, and does not work when using the native tracing configuration.
This removes the previous configs, and adds new control plane tracing config that mirrors the existing proxy tracing configs.
The previous configuration was meant entirely for internal testing purposes, and shouldn't be subject to any breaking change guarantees.