Skip to content

Conversation

@VineethReddy02
Copy link
Contributor

@VineethReddy02 VineethReddy02 commented Aug 12, 2021

fixes: #156

@VineethReddy02 VineethReddy02 force-pushed the vineeth-multi-cluster-support branch from 3c53d03 to 12d5a1e Compare August 16, 2021 09:18
Copy link
Collaborator

@cevian cevian left a comment

Choose a reason for hiding this comment

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

Some optional suggestions but looks good to merge

"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/v0.47.0/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/v0.47.0/example/prometheus-operator-crd/monitoring.coreos.com_thanosrulers.yaml",
"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/v0.47.0/example/prometheus-operator-crd/monitoring.coreos.com_prometheusrules.yaml",
"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/v0.49.0/example/prometheus-operator-crd/monitoring.coreos.com_alertmanagerconfigs.yaml",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there anyway we can have a test to make sure this version is aligned with the kube-prometheus-stack version? Otherwise this is very easy to forget to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests definitely sound reasonable. But usually, during the kube-prometheus version bump, we read the upgrade guide in kube-prometheus helm chart to understand the changes for upgrades.

I will add the tests on this.

@@ -0,0 +1,128 @@
# Tobs helm values config
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm. I think there are two different types of config here.

  1. config which you need only for things that don't use the CLI (high-availability)
  2. config applicable to both helm chart and CLI. (multicluster)

Type 1 is ok here. I'd move type 2 stuff into a new top-level /docs directory. It does not belong under /chart and I'd add new links from the front-page readme here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So currently using CLI we offer only abstract features with flags like HA, integration with forge, etc... all these features can also be configured within values.yaml. All the flexibility lies in the values.yaml so the intention behind creating this file is to document all possible configurations across different components in tobs.

Yes, I agree with you only config related to values.yaml will reside in this file. Anything about CLI will be in a new file in the high level root directory of this repo.

externalLabels:
cluster: <clusterName>
remoteRead:
- url: "<PROMSCALE_ENDPOINT>/read"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- url: "<PROMSCALE_ENDPOINT>/read"
- url: "<PROMSCALE_SERVICE_ENDPOINT_OF_DATA_AGGREGATION_CLUSTER>/read"

replicas: 3
```

## Multi-cluster support
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we have a diagram for this section showing the various clusters etc.

@VineethReddy02 VineethReddy02 force-pushed the vineeth-multi-cluster-support branch from 12d5a1e to f24b62f Compare August 24, 2021 13:00
@VineethReddy02 VineethReddy02 force-pushed the vineeth-multi-cluster-support branch from f24b62f to 8b7909a Compare August 24, 2021 13:14
@VineethReddy02 VineethReddy02 requested a review from cevian August 24, 2021 19:03
Copy link
Collaborator

@cevian cevian left a comment

Choose a reason for hiding this comment

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

Looks good except for missing image


## Multi-cluster support

<img src="./../../docs/assets/multi-cluster.png" alt="multi-cluster setup diagram" width="800"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you forgot to add this image in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VineethReddy02 VineethReddy02 merged commit 6c627ee into timescale:master Aug 24, 2021
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.

Full support for multi-cluster

2 participants