docs: say what MinIO is for in the README, and correct the backup claims - #13
Merged
Conversation
hakan-persson
force-pushed
the
docs/minio-scope
branch
from
August 22, 2026 16:29
183ddd8 to
abf5f68
Compare
Member
Author
|
Rebased onto The two PRs independently fixed the same stale What remains here is just the MinIO scope question, which #10 did not touch: the |
There was a problem hiding this comment.
Pull request overview
Updates the repository’s primary documentation to prevent readers from misinterpreting MinIO as general-purpose, project-available S3 storage, and to correct what data is (and is not) stored/backed up via MinIO.
Changes:
- Rename MinIO in the architecture “common services” diagram to “observability store” to reduce misinterpretation.
- Rewrite the storage section to explicitly scope MinIO to Loki/Thanos, explain the single-replica/low-durability design, and direct projects to PostgreSQL/PVCs.
- Clarify MinIO’s role in the Common services table to reinforce “not for projects”.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| [onboarding.md](docs/onboarding.md). | ||
|
|
||
| Persistent volumes use a cheap StandardSSD StorageClass by default, with a | ||
| Premium class available opt-in. |
Comment on lines
63
to
+67
| Storage is in-cluster and portable: **MinIO** for S3-compatible object storage | ||
| (backs Loki, Thanos, and backups) and **CloudNativePG** for PostgreSQL — no Azure | ||
| data PaaS. Persistent volumes use a cheap StandardSSD StorageClass by default, | ||
| with a Premium class available opt-in. | ||
| and **CloudNativePG** for PostgreSQL — no Azure data PaaS. MinIO exists to back | ||
| **Loki and Thanos** and is deliberately **not** general-purpose storage for | ||
| projects: it is a single replica on one volume, holding only data those two can | ||
| rebuild, which is why it is backed up weekly rather than daily. A project needing |
Member
Author
There was a problem hiding this comment.
Both fixed in 37b5ea8, and the second one is the better catch.
k8s/argocd/infra-apps/minio.yaml still claimed MinIO backs "Loki, Thanos, backups" — the exact error this PR corrects in the README, surviving in a file I had not thought to look at. Velero backs up to Azure Blob; that backups outlive the cluster is the whole point of the design, so the comment was undermining it. It now says what MinIO backs and what it is not, in one line.
I also swept the repo for the same claim in any other phrasing — no remaining occurrences.
The grammar one is fair too: "with a Premium class available opt-in" read as a fragment. It is pre-existing text this PR had only re-wrapped, so it was cheap to fix here.
Rebased onto main while I was in there, since #11 and #12 landed after this branch was cut.
Two people reading the README have now taken "MinIO (object storage)" under common services as "there is S3 storage my project can use". That is a design smell rather than a misreading: the scope was recorded in decisions.md, but the README is what people actually read first, and it said nothing. MinIO is now described where it is named — backs Loki and Thanos, single replica on one volume, holding only data those two can rebuild, which is why it is backed up weekly rather than daily. The paragraph points a project needing persistent state at the shared PostgreSQL or a PVC instead. Kept self-contained rather than deferring to a decisions entry, since that is exactly the indirection that let the confusion happen. Two factual errors found while checking this, both verified against the live cluster: - "MinIO ... backs Loki, Thanos, and backups" — Velero's BackupStorageLocation has provider `azure`, not MinIO. Backups go to durable Azure Blob storage, which is the whole point: they have to survive the cluster. - "Backups (Velero -> MinIO, plus CloudNativePG's own backups) are the next planned addition" — Velero is deployed and Available, and its restore was verified end-to-end. CNPG archives continuously to Azure Blob. No forward references: an earlier draft pointed at decisions.md entry 11 and an onboarding section that exist only on an unmerged branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
From Copilot's review. Two findings, and the second is the substantive one. k8s/argocd/infra-apps/minio.yaml still said MinIO "backs Loki, Thanos, backups" — the exact claim this PR corrects in the README, surviving in a different file where it would have reintroduced the same confusion. Velero backs up to Azure Blob; the whole point is that backups outlive the cluster. The comment now says what MinIO backs and what it is not, in the same breath. Swept the rest of the repo for the same claim in any other form: no remaining occurrences. Also reworded "with a Premium class available opt-in", which read as a fragment. Pre-existing text that this PR had only re-wrapped, so fixing it here is cheap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hakan-persson
force-pushed
the
docs/minio-scope
branch
from
August 22, 2026 20:48
abf5f68 to
37b5ea8
Compare
hakan-persson
added a commit
that referenced
this pull request
Aug 24, 2026
The README already states that MinIO is not general-purpose storage (#13), so the decision entry restated a merged conclusion. It now records only the reasoning a decision log is for: why the derived-data backup assumption is what actually blocks it, the two unsolved blockers, and the revisit trigger. Also resolves two rebase conflicts. The projectset include-glob kept BOTH filenames — argocd-status-rbac.yaml from #16 and app-state-rbac.yaml here; dropping either would make that feature inert while ArgoCD still reports Synced. And the section A file count is now 8, not 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hakan-persson
added a commit
that referenced
this pull request
Aug 26, 2026
Answers two questions from projects — *"where do I put a few KB of state?"* and *"what about a few MB of files?"* — and records why MinIO is not the answer to either. **Not urgent.** Opened to marinate while test round 4 is prepared; happy for it to sit. ## Why this is worth merging even though it is mostly documentation `main` currently makes a promise it does not keep. The README tells developers: > A project needing persistent state uses the shared PostgreSQL or a PersistentVolumeClaim — see onboarding.md. That section **does not exist**. PR #13 merged the pointer without the destination, because the destination was on this branch. `azurefile` appeared nowhere in the repo before this. ## What it adds | | | |---|---| | New capability | ConfigMap-as-state (template + the include-glob line) | | Newly *available* | `files-shared` StorageClass — the shared-filesystem tier | | Newly documented | the four tiers, their limits, and their **cost model** | | Newly recorded | why not MinIO, and what a project object store would cost | ## Two things that came out of review and changed the design **1. `azurefile-csi` was an Azure-specific name leaking into every project's repo.** Pointing projects at the AKS built-in class would have put the provider's name in every PVC manifest, so a migration would mean editing every project's repository — exactly what *"portable by intent"* exists to prevent, and contrary to the Portability table's own rule (*"change the provisioner; keep the class names"*). This branch now ships **`files-shared`**, a repo-owned class following the same pattern as `disk-*`. Verified live: bound RWX, and a non-root PSA-compliant pod wrote and read through it. The Portability table gains a row, and the docs **offer** the tier rather than pushing it — the class name survives a move, the SMB semantics may not. **2. Azure Files bills per operation, and the cluster is paid centrally.** Prices read from the Azure retail API for `swedencentral`, not estimated: | Write pattern | `files-shared` | `disk-*` (E4, 32Gi) | |---|---|---| | Checkpoint every 30s | $0.56/mo | $2.40/mo | | Session state, 5 req/s | **$84.24/mo** | $2.40/mo | A fixed 32Gi disk costs the same as **~370,000 operations a month** — about 0.14 writes/second. So it is the cheapest tier for *state* and among the most expensive for *traffic*. That matters more here than the numbers suggest: **a project never sees the cost of its own choice**, so the guidance has to do what the invoice does not. The onboarding note now asks *how often* state is written rather than how big it is, and names the two patterns explicitly. It is also the strongest argument for a project object store later — its cost is the PVC underneath it, so S3 calls are in-cluster and free at the margin, and it puts one reviewable number on the shared bill instead of a variable per-project one nobody watches. ## The load-bearing non-doc change The `projectset.yaml` include-glob gains `app-state-rbac.yaml`. The `project-infra` ApplicationSet syncs an **explicit list of filenames**, and a file matching none of them is **ignored silently** while ArgoCD still reports `Synced`/`Healthy`. Without it, infra commits the RBAC, sees green, and the app has no permissions. It also keeps `argocd-status-rbac.yaml` from #16 — dropping either filename makes that feature inert. ## Verified - Rebased onto current `main` (`d786537`), two conflicts resolved deliberately. - Decision entries **15** and **16**, in numerical order; `main`'s own `entry 11`/`entry 12` references left pointing where they were. - The RBAC template and the StorageClass both **tested live** on the test cluster in both namespaces, then removed. - All five CI checks pass locally — 78 manifests, 48 decision pointers. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A developer at the design presentation read "MinIO (object storage)" in the
README's common-services list as "there is S3 storage my project can use".
Håkan had read it the same way earlier. Two people making the same inference is a
design smell, not a misreading — the scope was recorded in
decisions.md, but theREADME is what people read first and it said nothing.
What changed
MinIO is now described where it is named: backs Loki and Thanos, single
replica on one volume, holding only data those two can rebuild — which is why it
is backed up weekly rather than daily. The paragraph then points a project needing
persistent state at the shared PostgreSQL or a PVC.
Deliberately self-contained rather than deferring to a decisions entry. The
indirection is what allowed the confusion: nobody reads the decision log before
the README.
Two factual errors found while checking, both verified live
BackupStorageLocationhasprovider: azure, bucketvelero, phaseAvailable. Backups go to durable Azure Blob storage, which is the entire point— they have to survive the cluster. Saying they land in MinIO undersells the
design and would mislead anyone reasoning about recovery.
Available, and its restore was verified end-to-end. CNPG archives continuously
to Azure Blob.
Not in this PR
The larger question behind it — whether to offer projects a separate MinIO as a
real platform service — is unresolved and deliberately out of scope. This is the
part that pays off immediately either way: if a second instance is built later,
the README will need to name both and distinguish them, and this makes that
edit additive rather than a correction.
No forward references: an earlier draft pointed at
decisions.mdentry 11 and anonboarding.mdsection that exist only on an unmerged branch.Four CI jobs pass; all README links resolve.