e2e: make the StorageClass definitions configurable - #10588

👷 Deploy request for velero pending review.

Visit the deploys page to approve it

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Good catch, that literal is evaluated before flag.Parse, so LoadAffinity would have kept the defaults. Moved it into Init(). It was the only package-level spot reading the names; CachePVCTestCase already builds its config there.

Re-ran Basic && (ClusterResource || NodePort || StorageClass) with both flags set, still 5/5. LoadAffinity itself I could not run on kind since it needs --snapshot-move-data and the kind class is still local-path, so that one is by inspection.

Really nice work on this one. The creation path catch is the part I'd highlight. Just swapping the constants for flags would have looked fine and been quietly broken, with the specs going off to find the new name while the apply still created e2e-storage-class. Building both classes from a single definition is the right fix, and decoding into a typed StorageClass rather than string-replacing saves us from a provider file that happens to carry a parameter called name. That's the kind of thing that's much cheaper to catch now than six months from now.

The node-agent-config.go spot was good too. I went through the other readers of those two vars and that was the only one initializing at package level, so you got all of it.

On your question: let's do both follow-ups here rather than splitting them. They're both about what the suite creates, and the flag doesn't buy us much until the first one lands, since pointing it at csi-hostpath-sc is really why we want it.

One suggestion on that first one. Skip-if-exists covers half the problem. AfterSuite deletes both names unconditionally (test/e2e/e2e_suite_test.go:826 and :835), so we'd stop clobbering csi-hostpath-sc on the way in and still delete it on the way out. It might be easier to think of it as ownership: if you passed the flag, the class is yours, so we neither create nor delete it, and we fail early with a clear message if it isn't there. No flag, everything behaves exactly as it does today. One rule, both ends covered.

For the second, I'd avoid trying to detect which specs need class 2. BeforeSuite runs before you can see what the label filter selected, so there's nothing reliable to check. Opt-out is simpler: an empty --storage-class-2 means don't create it, and the four places that use it (nodeagentconfig, resourcepolicies, storage-class-changing, migration) skip with a message explaining why.

Flag names look fine to me, no need to churn on those. Thanks for writing up the timings and the before/after comparison in the description, that made this much easier to review.

Both follow-ups are in, done as ownership like you suggested.

If --storage-class or --storage-class-2 is set, the class belongs to whoever set it up: not created, not deleted, and BeforeSuite fails early with a message if it is missing. Unset, everything behaves as before. Empty --storage-class-2 runs without a second class and the four cases that need one skip with a message saying how to enable them.

One thing that fell out while wiring it up: the Makefile was passing --storage-class on every run with a default value, so "set" was always true and the default path broke immediately, before any spec ran. The names are no longer defaulted in the Makefile and the flags are only passed when the caller sets them. A flag that is always passed cannot express "not set", which is really the same point you were making about AfterSuite.

Ran four cases on kind with Basic && (ClusterResource || NodePort || StorageClass):

Rebased on main. Description updated to match.

Thanks for working on this!

Configuring just the SC names (--storage-class / --storage-class-2) brings up a few tricky issues: it doesn't allow changing the provisioner or parameters, and targeting existing cluster SCs creates complex ownership problems around creation and teardown in BeforeSuite / AfterSuite.

Instead of configuring names and adopting existing cluster SCs, what about letting users provide a customized StorageClass manifest (e.g. --storage-class-file)?

• Clean lifecycle: E2E applies it in BeforeSuite and deletes it in AfterSuite (just like volume-snapshot-class). No ownership heuristics, and no risk of deleting cluster-preexisting resources.
• Full flexibility: Users can define the actual provisioner and parameters (e.g., hostpath CSI on kind), not just the name.
• Handling 1 vs. 2 SCs: Standard tests use the first SC. The few tests that verify remapping (storage-class-changing, resourcepolicies, etc.) can dynamically check if 2 SCs were provided and Skip() if only 1 was given—matching how multi-BSL and multi-cluster tests already behave.

This keeps the interface down to a single flag while making E2E lifecycle management much simpler and safer. What do you think?

Thanks, that is a fair point about the provisioner. Renaming cannot change it, which is the gap I flagged in the description: on kind the class is still local-path whatever it is called, so pointing at an existing csi-hostpath-sc is the only way to get a CSI-backed class today, and that is what the ownership rules are there for.

A manifest flag would solve that more directly, and I agree the lifecycle gets simpler if the suite always creates and deletes what it applied, with no ownership rules at all.

The current shape is what @shubham-pampattiwar asked for on this PR, so I would rather you two agree on the interface than have me guess between you. Happy to switch to --storage-class-file; the change is fairly contained, mostly InstallStorageClasses and the flag wiring, and the skip behaviour for the second class works the same either way.

One thing worth keeping whichever way it goes: InstallStorageClasses currently takes the first class name from the provider file and derives the second by string-replacing a constant, so the two are coupled by an invariant nothing enforces. That is a live bug independent of the interface, and this PR fixes it by building both from one definition.

@blackpiglet is right, and I want to be clear that this is me changing my mind rather than @krishhna24 guessing wrong. The ownership model on this PR is there because I asked for it.

The thing that convinced me is the provisioner point. Renaming a class cannot change it, so on kind the class stays rancher.io/local-path whatever we call it. That means names only get us a CSI-backed class by adopting the csi-hostpath-sc that csi-driver-host-path ships, and adopting a class we did not create is what forced all the machinery: the Provided booleans, the existence check, the early failure, the $(origin) guard in the Makefile.

We do not actually need to adopt anything. We can apply our own class with provisioner: hostpath.csi.k8s.io and the suite owns everything it created again, which makes teardown unconditional and lets all of that go. It also matches how volume-snapshot-class and the VGSClass from #10582 already work, one file per situation. And with a manifest the name just comes from metadata.name, so the name flag stops being needed at all.

@krishhna24, sorry for the churn, two rounds on a model we are now dropping. The good news is most of it survives:

• Keep the creation path fix. InstallStorageClasses taking the first name from the file and deriving the second by string-replacing a constant is a real bug independent of the interface, and you are right to call that out.
• Keep the skip behaviour for the second class, it works the same way with files.
• Drop the Provided booleans, StorageClassExists, the early-fail, and the Makefile $(origin) guard.
• --storage-class-file and --storage-class-file-2, both defaulting to testdata/storage-class/<provider>.yaml so unset behaves exactly as today. Empty for the second one means run without it.

Should come out smaller than what is there now. @blackpiglet does that match what you had in mind?

Conflicts here are from #10592, which rewrote sc.go and InstallStorageClasses.

Worth flagging: that PR already fixes the creation path this one was carrying. CreateStorageClassFromYaml decodes into a typed StorageClass and takes a name override, and InstallStorageClasses now calls it twice from the same file, so the file-versus-constant coupling is gone on main.

That makes the rework smaller again. With yamlPath already a parameter, the file-based version is mostly choosing which path to pass. Still holding for @blackpiglet to confirm the shape, then I will rebase and push the reduced version rather than resolving conflicts against code we are removing.

Please rebase and resolve conflicts @krishhna24

Rebased. Rather than merging the ownership code and then deleting it, I reset onto main and rebuilt it as the file-based version, so this is one commit and 95 insertions where the old one was 155.

Two things main gave us. #10592 added CreateStorageClassFromYaml, which takes a yaml path and a name override, so the class names stay constants: every spec keeps referring to e2e-storage-class and only the definition varies. That removes the name flags entirely, along with the Provided booleans, StorageClassExists and the early-fail. DeleteStorageClass also swallows NotFound now, so teardown stays unconditional with no extra logic.

Gates on kind, Basic && (ClusterResource || NodePort || StorageClass) each time:

I ran that last one because the failure I would expect from this change is a missing file quietly falling back to the provider file, which would leave the flag looking like it worked while testing something else. It does not fall back.

One thing I did not manage to drop: STORAGE_CLASS_FILE_2 still goes through an $(origin) check in test/Makefile. An empty value means run without a second class, and a flag that is always passed cannot express "not set", so the Makefile only passes it when the caller sets it. If you would rather have a separate boolean for that, it is a two line change.

PTAL ! @shubham-pampattiwar @kaovilai

Noticed the title and description here were still the ownership version, which is probably confusing if you opened this expecting what @blackpiglet asked for. Both now match the branch. No code change, still c7031a978.

This test only changes. I think this is safe to merge post FC.