Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

appliance: deploy prometheus#62876

Merged
craigfurman merged 12 commits into
mainfrom
appliance-deploy-prometheus
May 24, 2024
Merged

appliance: deploy prometheus#62876
craigfurman merged 12 commits into
mainfrom
appliance-deploy-prometheus

Conversation

@craigfurman

@craigfurman craigfurman commented May 23, 2024

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/REL-15/service-definition-prometheus

This is a very large PR (sorry!). I highly recommend reviewing one commit at a time, reading the commit messages. I presented it as one, rather than a stack, because I suspect the history won't be that useful after review, and can probably be squashed. Holler if this is just too much, I'm happy to split it up into a stack if need be.

Please see self-review notes too.

Changelog:

New YAML package to hold yaml-related utilities

First function: ConvertYAMLStringsToMultilineLiterals.

appliance: remove getter method for pgsql config

In favor of just accessing the package-level variable.

appliance: add Prometheus config element

Disable it in all current golden tests.

appliance: deploy Prometheus

appliance: preserve multiline literals in golden fixtures

appliance: compare-helm: normalize yaml

Standardize indentation and represent multiline strings as literals.
This makes diffs of large nested documents easier to read (e.g. the
prometheus configmap).

appliance: compare-helm: allow cleanup to run when diff exits non-zero

appliance: prometheus privileged config

Optionally provision RBAC with cluster-level privileges, and
corresponding scrape config.

Due to kubernetes rules around namespaced objects not owning namespaced
ones, our ConfigMap cannot own the ClusterRole(Binding)s provisioned by
this config. As such, they will not be garbage-collected when the
ConfigMap is deleted. These cluster-scoped resources are given a
qualified metadata.name, in order to minimise the risk of clashing with
existing non-namespaced resources.

appliance: prometheus can optionally reference an existing configmap

Rather than creating one.

Test plan

Golden tests included.

@cla-bot cla-bot Bot added the cla-signed label May 23, 2024
Comment thread internal/yaml/yaml.go Outdated
Comment thread internal/appliance/config/embed.go Outdated
Comment thread internal/appliance/reconciler/prometheus_test.go Outdated
Comment thread internal/appliance/reconciler/golden_test.go Outdated
Comment thread internal/appliance/reconciler/golden_test.go Outdated
Comment thread internal/appliance/reconciler/prometheus_test.go Outdated
Comment on lines 78 to 87

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we use a finalizer to clean these up? It might be difficult to manually keep "ownership" of it though 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I wrote this up and split it out into https://linear.app/sourcegraph/issue/REL-92/investigate-should-we-try-to-clean-up-cluster-scoped-resources, because I'm unlikely to get to it today, and I want to land the golden file formatting part of this PR so that I don't get conflicts with other service definition PRs that come through in the next few days. In hindsight this would have been a good use for a stack 😅

@craigfurman craigfurman force-pushed the appliance-deploy-prometheus branch 2 times, most recently from 0e8c069 to 8fe4d9d Compare May 23, 2024 11:23
@craigfurman craigfurman requested review from a team and jdpleiness May 23, 2024 15:53

@jdpleiness jdpleiness left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😅 that was a long one, but not too bad. I made a few comments, but nothing blocking. I did skim more lightly than normal over the yaml testing pkg as well.

Comment thread internal/k8s/resource/pod/pod.go Outdated
Comment thread internal/slices/sliceutils.go Outdated
Comment thread internal/yaml/yaml.go Outdated
Craig Furman added 10 commits May 24, 2024 09:37
First function: ConvertYAMLStringsToMultilineLiterals.
Disable it in all current golden tests.
Standardize indentation and represent multiline strings as literals.
This makes diffs of large nested documents easier to read (e.g. the
prometheus configmap).
Optionally provision RBAC with cluster-level privileges, and
corresponding scrape config.

Due to kubernetes rules around namespaced objects not owning namespaced
ones, our ConfigMap cannot own the ClusterRole(Binding)s provisioned by
this config. As such, they will not be garbage-collected when the
ConfigMap is deleted. These cluster-scoped resources are given a
qualified metadata.name, in order to minimise the risk of clashing with
existing non-namespaced resources.
I thought a generic map function was added to the Go stdlib recently,
but I was wrong.
@craigfurman craigfurman force-pushed the appliance-deploy-prometheus branch from 04a3475 to 506aff0 Compare May 24, 2024 08:49
@craigfurman craigfurman marked this pull request as ready for review May 24, 2024 08:49
@github-actions

Copy link
Copy Markdown
Contributor

Caution

License checking failed, please read: how to deal with third parties licensing.

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Caution

License checking failed, please read: how to deal with third parties licensing.

@craigfurman craigfurman force-pushed the appliance-deploy-prometheus branch from ca725dc to b768057 Compare May 24, 2024 08:55
@craigfurman craigfurman merged commit 6bb8209 into main May 24, 2024
@craigfurman craigfurman deleted the appliance-deploy-prometheus branch May 24, 2024 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants