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

Move "prometheus-federation" to use template configuration #101

Merged
merged 8 commits into from
Oct 17, 2017

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Oct 17, 2017

This change update the prometheus-federation configuration to use templates.

This change removes the per-project prometheus-federation directories in favor of a single template directory. This change also adds three project template variable files.


This change is Reviewable

@nkinkade
Copy link
Contributor

A couple small questions.


Reviewed 60 of 62 files at r1.
Review status: 45 of 46 files reviewed at latest revision, 2 unresolved discussions.


k8s/prometheus-federation/deployments/prometheus.yml, line 59 at r1 (raw file):

               "-storage.local.retention=2880h",
               "-alertmanager.url=http://alertmanager-public-service.default.svc.cluster.local:9093",
               "-web.external-url=http://status-{{GCLOUD_PROJECT}}.measurementlab.net:9090",

This might be a good time to go ahead and use the newer, GCD domains, no (e.g., status.stating.measurementlab.net)? Though I imagine it would require defining another variable in the per-project configs, or some way of variable manipulation like Ansible has with Jinja2.


k8s/prometheus-federation/deployments/prometheus.yml, line 66 at r1 (raw file):

        resources:
          requests:
            memory: "{{PROMETHEUS_RAM}}"

Question: The kexpand documentation indicates that {{<var>}} is an legacy format that is not recommended. They seem to recommend $(var) or $((var)). What is the rationale for using this "legacy" syntax?


Comments from Reviewable

@stephen-soltesz
Copy link
Contributor Author

Review status: 45 of 46 files reviewed at latest revision, 2 unresolved discussions.


k8s/prometheus-federation/deployments/prometheus.yml, line 59 at r1 (raw file):

Previously, nkinkade wrote…

This might be a good time to go ahead and use the newer, GCD domains, no (e.g., status.stating.measurementlab.net)? Though I imagine it would require defining another variable in the per-project configs, or some way of variable manipulation like Ansible has with Jinja2.

I would like to defer this to a future PR.


k8s/prometheus-federation/deployments/prometheus.yml, line 66 at r1 (raw file):

Previously, nkinkade wrote…

Question: The kexpand documentation indicates that {{<var>}} is an legacy format that is not recommended. They seem to recommend $(var) or $((var)). What is the rationale for using this "legacy" syntax?

kexpand is supposed to implement a proposed template format that was never formally adopted by the kubernetes project.

scraper and scraper-sync use the double {{}} format as does mlabconfig.py.

Since there is no official standard outside of kexpand, I would would prefer to normalize on a single format. And, I also happen to like the double {{}} better. :)

Also, using the double {{}} format in kubernetes files helps distinguish between variables that we expected to be template expanded vs variables that we expect to be expanded by kubernetes from environment variables like $(GITHUB_AUTH_TOKEN).


Comments from Reviewable

@nkinkade
Copy link
Contributor

:lgtm:


Reviewed 2 of 62 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

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