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
Duplicate S3 collector and make it run inside the seed-ctrl-mgr #9765
Duplicate S3 collector and make it run inside the seed-ctrl-mgr #9765
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xrstf The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
LGTM label has been added. Git tree hash: 07c32a7296f3ca2968cbd46b0a2a0bccf99e7717
|
Any thoughts on mentioning the s3-exporter by name in the release note? The relationship isn't clear to an outsider IMHO. |
/retest |
/retest Review the full test history Silence the bot with an |
/hold temporarily |
/hold cancel |
What does this PR do / Why do we need it:
The S3-Exporter is only here to produce Prometheus metrics tailored towards KKP's etcd backups (which is the reason we're not just using a generic S3 exporter). It was added in #1482.
The exporter has a couple of issues:
minio
Helm chart, because the Minio chart is creating thekube-system/s3-credentials
secret, which is consumed by the S3-Exporter. So even though the exporter code itself is fairly generic, its chart is only useful if you also a) installed Minio and b) installed Minio using our Helm chart.As in the future we want to reconcile more things automatically on seeds (like #9748), so it would be nice if the tiny S3 exporter is also managed automatically and learns how to deal with the multiple backup destinations. This is what this PR achieves.
This PR duplicates the old S3 collector because the new implementation is built around having access to the S3 bucket via the credentials configured in KKP (as apposed to relying on the s3-credentials Secret). The old backup configuration doesn't allow to specify credentials (again, it relies on this Secret created by our Minio chart) and so the new exporter cannot handle the old configuration and the old exporter cannot handle the new configuration.
Until we deprecate the old backup configuration, it's simply easier to just have this new collector run side-by-side and deal with the new configuration (etcd backup/restore) only.
The new exporter also runs as part of the seed-ctrl-mgr. There is little to gain by splitting it apart into a dedicated binary (or at least I cannot see the advantage) and since the seed-ctrl-mgr already manages the backups, it makes sense IMHO to let it also handle the metrics.
Once we remove the old backup configuration stuff, we can
rm -rf charts/s3-exporter cmd/s3-exporter
. At that point we can then also finally get rid of thes3-credentials
secret (which I have hated for a long time).Does this PR introduce a user-facing change?: