Support monolithic deployment mode #722

andreasgerstmayr · 2023-12-21T13:31:14Z

Support Tempo monolithic deployment mode with a new TempoMonolithic CR.
Partially resolves #710.

Signed-off-by: Andreas Gerstmayr <[email protected]>

codecov-commenter · 2023-12-21T14:30:01Z

Codecov Report

Attention: Patch coverage is 68.68381% with 207 lines in your changes missing coverage. Please review.

Project coverage is 76.78%. Comparing base (2d4c168) to head (c5eba0d).
Report is 198 commits behind head on main.

Files with missing lines	Patch %	Lines
controllers/tempo/tempomonolithic_controller.go	29.89%	58 Missing and 10 partials ⚠️
internal/manifests/monolithic/statefulset.go	77.13%	47 Missing and 4 partials ⚠️
apis/tempo/v1alpha1/tempomonolithic_webhook.go	56.89%	25 Missing ⚠️
controllers/tempo/common.go	61.90%	17 Missing and 7 partials ⚠️
internal/manifests/monolithic/configmap.go	71.42%	17 Missing and 3 partials ⚠️
internal/manifests/mutate.go	63.63%	6 Missing and 2 partials ⚠️
internal/manifests/monolithic/build.go	70.00%	4 Missing and 2 partials ⚠️
controllers/tempo/tempostack_create_or_update.go	71.42%	1 Missing and 1 partial ⚠️
internal/manifests/config/configmap.go	60.00%	2 Missing ⚠️
controllers/tempo/tempostack_controller.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
- Coverage   77.76%   76.78%   -0.99%     
==========================================
  Files          68       77       +9     
  Lines        5155     5733     +578     
==========================================
+ Hits         4009     4402     +393     
- Misses        949     1110     +161     
- Partials      197      221      +24

Flag	Coverage Δ
unittests	`76.78% <68.68%> (-0.99%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Andreas Gerstmayr <[email protected]>

…ngle pod Signed-off-by: Andreas Gerstmayr <[email protected]>

Signed-off-by: Andreas Gerstmayr <[email protected]>

…ployment

Signed-off-by: Andreas Gerstmayr <[email protected]>

.env

pavolloffay · 2024-01-12T10:28:50Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+}
+
+// MonolithicObservabilityMetricsSpec defines the metrics settings of the Tempo deployment.
+type MonolithicObservabilityMetricsSpec struct {


Can't we reuse smth from the microservice type?

e.g. the whole observability spec

tempo-operator/apis/tempo/v1alpha1/tempostack_types.go

Line 141 in cbe369b

type ObservabilitySpec struct {

That would be inconsistent with the

<feature>: enabled: true

style of the CR. I'd prefer to migrate the TempoStack, maybe in the next CRD version? Shouldn't be too difficult to create a conversion webhook for this.

spec: # TempoMonolithicSpec defines the desired state of TempoMonolithic. observability: # Observability defines observability configuration for the Tempo deployment metrics: # Metrics defines the metrics configuration of the Tempo deployment prometheusRules: # ServiceMonitors defines the PrometheusRule configuration enabled: false # Enabled defines if the operator should create PrometheusRules for this Tempo deployment serviceMonitors: # ServiceMonitors defines the ServiceMonitor configuration enabled: false # Enabled defines if the operator should create ServiceMonitors for this Tempo deployment

so we agreed on using the enabled field which is better supported in tools like kustomize and kubectl edit (the empty structs are removed). Are we going to reuse some parts from the monolithic APIs?

Yes, we can reuse a TLS struct, the multitenancy structs, ManagementStateType, LimitSpec and the storage secret.

pavolloffay · 2024-01-12T10:30:54Z

apis/tempo/v1alpha1/tempomonolithic_webhook.go

+
+// Default implements webhook.Defaulter so a webhook will be registered for the type.
+func (r *TempoMonolithic) Default() {
+	log := ctrl.Log.WithName("tempomonolithic-webhook")


how is this rendered in the logs? Isn't it too long?

It'll print this line:

{"level":"debug","ts":"2024-01-15T19:07:08.746058013+01:00","logger":"tempomonolithic-webhook","msg":"running defaulter webhook","name":"sample"}

if debug logs are enabled (go run ./main.go --zap-log-level=debug start).

But as we'll set the defaults in the reconcile loop now, I'm removing this log statement.

I'll keep it there for the validating webhook, which is still in use.

apis/tempo/v1alpha1/tempomonolithic_types.go

docs/operator/api.md

docs/spec/tempomonolithic.yaml

Signed-off-by: Andreas Gerstmayr <[email protected]>

…ployment

Signed-off-by: Andreas Gerstmayr <[email protected]>

.chloggen/monolithic_mode.yaml

pavolloffay · 2024-01-17T18:31:24Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+
+// MonolithicTracesStorageSpec defines the traces storage for the Tempo deployment.
+type MonolithicTracesStorageSpec struct {
+	// Backend defines the backend for storing traces. Default: memory


I am a bit concerned about using the in-memory as default.

The upstream uses PV as default and the in-memory can be easily overlooked and use significant resources in the cluster.

If you're only concerned about OOM situations, the memory counts towards the container resource limit:

While tmpfs is very fast be aware that, unlike disks, files you write count against the memory limit of the container that wrote them

https://kubernetes.io/docs/concepts/storage/volumes/#emptydir

I'd prefer to keep memory as default, as it's great for quick testing/demos/showcases (and is the default for jaeger all-in-one), and changing it to pv is easy (and can/should be mentioned in the docs).

controllers/tempo/tempomonolithic_controller.go

Signed-off-by: Andreas Gerstmayr <[email protected]>

…ployment

Signed-off-by: Andreas Gerstmayr <[email protected]>

pavolloffay · 2024-01-25T09:17:57Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+	// Observability defines observability configuration for the Tempo deployment
+	//
+	// +kubebuilder:validation:Optional
+	Observability *MonolithicObservabilitySpec `json:"observability,omitempty"`


Asking again, can we reuse some of the structs from the microservies type?

We can, question is do we value consistency inside the same CR or consistency between the two CRs more?

spec: observability: # Observability defines observability configuration for the Tempo deployment metrics: # Metrics defines the metrics configuration of the Tempo deployment prometheusRules: # ServiceMonitors defines the PrometheusRule configuration enabled: false # Enabled defines if the operator should create PrometheusRules for this Tempo deployment serviceMonitors: # ServiceMonitors defines the ServiceMonitor configuration enabled: false

vs

spec: observability: # ObservabilitySpec defines how telemetry data gets handled. grafana: # Grafana defines the Grafana configuration for operands. createDatasource: false # CreateDatasource specifies if a Grafana Datasource should be created for Tempo. instanceSelector: # InstanceSelector specifies the Grafana instance where the datasource should be created. metrics: # Metrics defines the metrics configuration for operands. createPrometheusRules: false # CreatePrometheusRules specifies if Prometheus rules for alerts should be created for Tempo components. createServiceMonitors: false # CreateServiceMonitors specifies if ServiceMonitors should be created for Tempo components. tracing: # Tracing defines a config for operands. jaeger_agent_endpoint: "localhost:6831" # JaegerAgentEndpoint defines the jaeger endpoint data gets send to. sampling_fraction: ""

The first example is consistent with the rest of the Monolithic CR, and allows additional settings for prometheusRules or serviceMonitors in the future.

I would prefer consistency on the same CRD, just one question, in the future is possible to have some sort of consolidation in order to get both? That will imply a breaking change though

Yes, I was thinking of maybe doing this change in v1alpha2 of TempoStack if we have a consensus.

apis/tempo/v1alpha1/tempomonolithic_types.go

pavolloffay · 2024-01-25T09:21:00Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+	// Ingress defines the ingress configuration for Jaeger UI
+	//
+	// +kubebuilder:validation:Optional
+	Ingress *MonolithicJaegerUIIngressSpec `json:"ingress,omitempty"`


Can be the ingress spec reused from the microservices? There are more settings users might want to configure

Already ready in #755:

spec: jaegerui: # JaegerUI defines the Jaeger UI configuration enabled: false # Enabled defines if the Jaeger UI should be enabled ingress: # Ingress defines the ingress configuration for Jaeger UI annotations: # Annotations defines the annotations of the Ingress object. "key": "" enabled: false # Enabled defines if an Ingress object should be created for Jaeger UI host: "" # Host defines the hostname of the Ingress object. ingressClassName: "" # IngressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource. route: # Route defines the route configuration for Jaeger UI annotations: # Annotations defines the annotations of the Ingress object. "key": "" enabled: false # Enabled defines if a Route object should be created for Jaeger UI host: "" # Host defines the hostname of the Ingress object. ingressClassName: "" # IngressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource. termination: "edge" # Termination specifies the termination type. Default: edge.

vs spec of TempoStack:

jaegerQuery: # JaegerQuerySpec defines Jaeger Query specific options. enabled: false # Enabled is used to define if Jaeger Query component should be created. ingress: # Ingress defines Jaeger Query Ingress options. annotations: # Annotations defines the annotations of the Ingress object. "key": "" host: "" # Host defines the hostname of the Ingress object. ingressClassName: "" # IngressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource. route: # Route defines OpenShift Route specific options. termination: "" # Termination specifies the termination type. By default "edge" is used. type: "" # Type defines the type of Ingress for the Jaeger Query UI. Currently ingress, route and none are supported.

(ingressClassName should have been under ingress, as it doesn't apply to route)

Do we value we value consistency inside the same CR or consistency between the two CRs more?

Same comment, I'd prefer consistency on the same CR

controllers/tempo/tempomonolithic_controller.go

pavolloffay · 2024-01-25T09:28:15Z

internal/manifests/mutate.go

+}
+
+func (m *ImmutableErr) Error() string {
+	return fmt.Sprintf("update to immutable field %s is forbidden", m.field)


shouldn't it print the existing and desired as well?

I did initially, but printing structs with fmt.Sprintf("%v", some_struct) is an unreadable mess if the struct is big.

shall we then remove fields that are not used in the error struct?

I updated the message to show the result of cmp.Diff() now - it's still a bit unreadable as it's a single line, but when replacing the \n and \t we see a nice diff in the logs, and we already had a dependency on this library anyway (https://github.com/google/go-cmp).

Signed-off-by: Andreas Gerstmayr <[email protected]>

rubenvp8510 · 2024-01-26T06:40:30Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+	// ExtraConfig defines any extra (overlay) configuration for components
+	//
+	// +kubebuilder:validation:Optional
+	ExtraConfig *MonolithicExtraConfigSpec `json:"extraConfig,omitempty"`


Any reason for not reusing the same as microservices? The only reason I think is because the microservices could include other configs in the future. If that is the reason I'm ok

I think we worked on the same feature at the same time. I'll check if I can reuse the struct and logic.

I've updated the PR and reused the struct and logic from the TempoStack now.

rubenvp8510 · 2024-01-26T06:41:44Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+}
+
+// MonolithicStorageSpec defines the storage for the Tempo deployment.
+type MonolithicStorageSpec struct {


One question, Why this have it's own spec and inside only have one structure? or why to not use MonolithicTracesStorageSpec directly. Is this for mimic the tempo configuration?

Yes, it's to mimic the tempo configuration. I thought maybe tempo has plans to store other things in the future, so I'll keep the same here also.

But I don't have very strong opinions on this, I could remove that extra layer if you like.

rubenvp8510 · 2024-01-26T06:43:15Z

apis/tempo/v1alpha1/tempomonolithic_types.go

+	// OTLP defines the ingestion configuration for OTLP
+	//
+	// +kubebuilder:validation:Optional
+	OTLP *MonolithicIngestionOTLPSpec `json:"otlp,omitempty"`


It will be the only protocol supported?

I want to make the gateway a drop-in feature, so the service ports should not change if gateway is enabled or not. So I can only support protocols which the gateway also supports.
Afaics the gateway only supports otlp/grpc and otlp/http, right?

With the general move to the OTEL SDK, and using the OTEL collector, I think it's fine to only support OTLP.

Signed-off-by: Andreas Gerstmayr <[email protected]>

…ployment

Signed-off-by: Andreas Gerstmayr <[email protected]>

andreasgerstmayr added 3 commits December 21, 2023 14:28

Support monolithic deployment mode

edb2a72

Signed-off-by: Andreas Gerstmayr <[email protected]>

Add changelog

51a2b26

Signed-off-by: Andreas Gerstmayr <[email protected]>

update mutate_test.go

41dbed1

Signed-off-by: Andreas Gerstmayr <[email protected]>

andreasgerstmayr added 16 commits December 21, 2023 18:45

add webhook tests

8aaa8b5

Signed-off-by: Andreas Gerstmayr <[email protected]>

add generated files

442004c

Signed-off-by: Andreas Gerstmayr <[email protected]>

support pruning unmanaged objects

7d3ce55

Signed-off-by: Andreas Gerstmayr <[email protected]>

use common reconcile/prune function

84cba0c

Signed-off-by: Andreas Gerstmayr <[email protected]>

fix linter

cd797bc

Signed-off-by: Andreas Gerstmayr <[email protected]>

add sts tests

355a79e

Signed-off-by: Andreas Gerstmayr <[email protected]>

add service tests

82fef56

Signed-off-by: Andreas Gerstmayr <[email protected]>

add configmap tests

0f490dd

Signed-off-by: Andreas Gerstmayr <[email protected]>

use a single service, because the monolithic deployment will use a si…

c84f6bd

…ngle pod Signed-off-by: Andreas Gerstmayr <[email protected]>

drop component from labels

bd54f6b

Signed-off-by: Andreas Gerstmayr <[email protected]>

fix configmap

8c66093

Signed-off-by: Andreas Gerstmayr <[email protected]>

TestBuildAll()

5874547

Signed-off-by: Andreas Gerstmayr <[email protected]>

add e2e test

3b86d3a

Signed-off-by: Andreas Gerstmayr <[email protected]>

drop defaulter webhook for TempoMonolithic

3faae98

Signed-off-by: Andreas Gerstmayr <[email protected]>

delete operator namespace and webhooks in make run

a027746

Signed-off-by: Andreas Gerstmayr <[email protected]>

Merge remote-tracking branch 'upstream/main' into tempo-monolithic-de…

7af52c4

…ployment

andreasgerstmayr marked this pull request as ready for review January 11, 2024 14:05

andreasgerstmayr added 3 commits January 11, 2024 17:36

change the way to include .env file

19d6e07

Signed-off-by: Andreas Gerstmayr <[email protected]>

rebuild bundle, fix linter

f29369f

Signed-off-by: Andreas Gerstmayr <[email protected]>

fix typo in jaeger-grpc port name

f0c9ffc

Signed-off-by: Andreas Gerstmayr <[email protected]>

pavolloffay reviewed Jan 12, 2024

View reviewed changes

andreasgerstmayr added 3 commits January 15, 2024 19:17

add warning when overriding tempo config

c0343f1

Signed-off-by: Andreas Gerstmayr <[email protected]>

specify default value of storage.traces.backend

0c9b6e6

Signed-off-by: Andreas Gerstmayr <[email protected]>

update storage docs

fd37ff8

Signed-off-by: Andreas Gerstmayr <[email protected]>

andreasgerstmayr mentioned this pull request Jan 17, 2024

Local Backend type #709

Closed

Merge remote-tracking branch 'upstream/main' into tempo-monolithic-de…

9acad7b

…ployment

drop .env file

754486e

Signed-off-by: Andreas Gerstmayr <[email protected]>

pavolloffay reviewed Jan 17, 2024

View reviewed changes

.chloggen/monolithic_mode.yaml Outdated Show resolved Hide resolved

pavolloffay reviewed Jan 17, 2024

View reviewed changes

andreasgerstmayr added 6 commits January 18, 2024 13:26

reconcile: retry on conflict

3f57be4

Signed-off-by: Andreas Gerstmayr <[email protected]>

Merge remote-tracking branch 'upstream/main' into tempo-monolithic-de…

852d0e2

…ployment

update changelog, set size defaults, add api docs

19ca826

Signed-off-by: Andreas Gerstmayr <[email protected]>

update changelog

cd64880

Signed-off-by: Andreas Gerstmayr <[email protected]>

fix test

a9e41e9

Signed-off-by: Andreas Gerstmayr <[email protected]>

add controller test

6657c89

Signed-off-by: Andreas Gerstmayr <[email protected]>

andreasgerstmayr mentioned this pull request Jan 19, 2024

Support creating Ingress and Route objects in the TempoMonolithic CR #755

Merged

enable OTLP/HTTP by default

9a95fce

Signed-off-by: Andreas Gerstmayr <[email protected]>

pavolloffay reviewed Jan 25, 2024

View reviewed changes

andreasgerstmayr added 2 commits January 25, 2024 11:36

drop TLS struct (will reuse TLS struct from TempoStack in follow-up PR)

1cf6976

Signed-off-by: Andreas Gerstmayr <[email protected]>

add comment to default function

16b7030

Signed-off-by: Andreas Gerstmayr <[email protected]>

rubenvp8510 reviewed Jan 26, 2024

View reviewed changes

andreasgerstmayr added 4 commits January 26, 2024 12:40

show diff using cmp.Diff() in logs when immutable field is changed

822c016

Signed-off-by: Andreas Gerstmayr <[email protected]>

re-use ExtraConfig from TempoStack

bf76bef

Signed-off-by: Andreas Gerstmayr <[email protected]>

Merge remote-tracking branch 'upstream/main' into tempo-monolithic-de…

4c6715a

…ployment

add comment on exported function

c5eba0d

Signed-off-by: Andreas Gerstmayr <[email protected]>

pavolloffay approved these changes Jan 26, 2024

View reviewed changes

andreasgerstmayr merged commit d8afdec into grafana:main Jan 26, 2024
11 checks passed

andreasgerstmayr mentioned this pull request Feb 6, 2024

Support Tempo deployments in monolithic mode #710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support monolithic deployment mode #722

Support monolithic deployment mode #722

andreasgerstmayr commented Dec 21, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

pavolloffay Jan 12, 2024

andreasgerstmayr Jan 18, 2024

pavolloffay Jan 25, 2024

andreasgerstmayr Jan 25, 2024

pavolloffay Jan 12, 2024

andreasgerstmayr Jan 15, 2024

andreasgerstmayr Jan 15, 2024

pavolloffay Jan 17, 2024

andreasgerstmayr Jan 18, 2024

pavolloffay Jan 25, 2024

andreasgerstmayr Jan 25, 2024

andreasgerstmayr Jan 25, 2024

rubenvp8510 Jan 26, 2024

andreasgerstmayr Jan 26, 2024

pavolloffay Jan 25, 2024

andreasgerstmayr Jan 25, 2024 •

edited

Loading

rubenvp8510 Jan 26, 2024

pavolloffay Jan 25, 2024

andreasgerstmayr Jan 25, 2024

pavolloffay Jan 26, 2024

andreasgerstmayr Jan 26, 2024

rubenvp8510 Jan 26, 2024

andreasgerstmayr Jan 26, 2024

andreasgerstmayr Jan 26, 2024

rubenvp8510 Jan 26, 2024 •

edited

Loading

andreasgerstmayr Jan 26, 2024

rubenvp8510 Jan 26, 2024

andreasgerstmayr Jan 26, 2024

Support monolithic deployment mode #722

Support monolithic deployment mode #722

Conversation

andreasgerstmayr commented Dec 21, 2023 • edited Loading

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasgerstmayr Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasgerstmayr commented Dec 21, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

andreasgerstmayr Jan 25, 2024 •

edited

Loading

rubenvp8510 Jan 26, 2024 •

edited

Loading