Skip to content
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 templates for Thanos Operator #729

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

saswatamcode
Copy link
Member

No description provided.

Signed-off-by: Saswata Mukherjee <[email protected]>
- asyncForwardWorkerCount: 50
externalLabels:
replica: $(POD_NAME)
image: quay.io/thanos/thanos:v0.37.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder should we template out the image and set this as default. Also we need to use the konflux build here

kind: ThanosStore
metadata:
creationTimestamp: null
name: default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt it a case that we want a store per bucket, so in this case two stores and within that three sets of time sharded stores? so in fact in the end i believe we would have a reference to 6 different ThanosStore CRs? Here we appear to have two. One for telemeter and one for mst.

Maybe mst is a better name than default here too but Im not fussy on it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also given what I just said I wonder if we should move the resources into individual templates just so its easier to track diffs?

lazyDownloadStrategy: lazy
lazyReaderIdleTimeout: 5m
logFormat: logfmt
logLevel: info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably can remove these values that will be sensibly defaulted to tidy things up. Again Im not too fussy on it

shardReplicas: 2
shards: 3
type: block
storageSize: 50GiB
Copy link
Contributor

@philipgough philipgough Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is way too large. We can go to 2GiB here based on what I currently see in stage and we can even go further lower if we intend (which I believe we do) in time splitting these shards?

optional: false
shardingStrategy:
shardReplicas: 2
shards: 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially some misunderstanding, but I thought given what we now know about block sharding we were going to shard by time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i see the other ThanosStores now at the bottom of the file. In that case this wont make sense though?

kind: ThanosCompact
metadata:
creationTimestamp: null
name: rhobs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have two identically named compactors here and below!

fiveMinutes: 365d
oneHour: 365d
raw: 365d
storageSize: ""
Copy link
Contributor

@philipgough philipgough Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a size here. looks like 2GB will suffice for telemter and 1Gb for mst

kind: ThanosCompact
metadata:
creationTimestamp: null
name: rhobs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate name!

matchLabels:
operator.thanos.io/query-api: "true"
queryRangeMaxRetries: 3
queryRangeSplitInterval: 24h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed previously, I think it makes more sense to go with 2d here

logFormat: logfmt
logLevel: info
name: telemeter
replicas: 15
Copy link
Contributor

@philipgough philipgough Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overkill for stage, lets go with 6

key: thanos.yaml
name: default-objectstorage
optional: false
replicas: 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets go with 3 as we have now

imagePullPolicy: IfNotPresent
logFormat: logfmt
logLevel: info
replicas: 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets go with 3

shardReplicas: 2
shards: 3
type: block
storageSize: 100GiB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2Gb should do here

Signed-off-by: Saswata Mukherjee <[email protected]>

// Stage images.
var StageImages = ParamMap[string]{
"STORE02W": "quay.io/thanos/thanos:v0.37.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets create a const for quay.io/thanos/thanos:v0.37.2 like currentImage or something and point to that in the map. Makes future updates easier

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
@saswatamcode saswatamcode marked this pull request as ready for review February 7, 2025 08:38
Containers: []corev1.Container{
{
Name: "kube-rbac-proxy",
Image: "gcr.io/kubebuilder/kube-rbac-proxy:v0.16.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need to pull an image from RH registry for this.

return v
}

const CurrentThanosImageStage = "quay.io/thanos/thanos:v0.37.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here. I think we can use the konflux fork

@@ -0,0 +1,341 @@
apiVersion: template.openshift.io/v1
kind: Template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will need a servicemonitor for this. Ideally, because of the way I intend on rolling them out, it should be in its own template.

valueFrom:
fieldRef:
fieldPath: metadata.name
image: registry.redhat.io/rhosdt/jaeger-agent-rhel8:1.57.0-10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not to worry about this for now but I wonder if we should swap this out for the otel collector at some point

Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants