Conversation
|
Unfortunately, KinD does not support volume expansion by default, so we will need to find a new PVC provisioner for our integration tests. |
|
I just handled a volume scale for customer's customer today. Disks were full and we did the dance of updating sts, deleting it in orphan mode, adjusting each PVC manually, waiting for resize, and rolling restart of sts. A true pain :) So is there perhaps any news on this, or is it an alternative to test this feature only on unit level or with mocks until we can use a true provisioner? |
|
Ok, I removed part of the test that relied on the volumes actually changing. The tests pass and, as expected, the operator successfully updates the size requested. This should be close to good-to-go. If you want to review, we can merge and hopefully get the next release out soon. |
Co-authored-by: Steffen Moldenhauer <54577793+smoldenhauer-ish@users.noreply.github.com>
|
Great. I trust Copilot’s review more than mine, so triggered one 😉 |
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support in the Solr Operator for expanding (resizing) persistent data PVCs, working around the current Kubernetes StatefulSet limitation by patching PVCs directly and triggering a rolling restart.
Changes:
- Introduces a new cluster operation (
PVCExpansion) and related annotations to coordinate PVC resizing + restart. - Expands RBAC to allow patch/update on PersistentVolumeClaims.
- Adds/updates E2E test coverage and supporting test harness/log collection, plus documentation/changelog updates.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
controllers/solrcloud_controller.go |
Adds PVC expansion reconciliation logic and hooks it into the reconcile loop. |
controllers/solr_cluster_ops_util.go |
Adds new PVCExpansion cluster-op type and handler. |
controllers/util/solr_util.go |
Adds storageMinimumSize annotation tracking on StatefulSet + pod template. |
config/rbac/role.yaml |
Grants patch/update permissions on PVCs for the manager role. |
helm/solr-operator/templates/role.yaml |
Grants patch/update permissions on PVCs for the Helm-installed operator role. |
tests/e2e/solrcloud_storage_test.go |
Adds an E2E test validating PVC request-size updates during expansion flow. |
tests/e2e/suite_test.go |
Enhances failure artifact collection (PVC info) and tweaks operator log filtering. |
tests/scripts/manage_e2e_tests.sh |
Patches KinD’s default StorageClass to allow expansion (workaround for kind issue). |
docs/solr-cloud/solr-cloud-crd.md |
Documents that PVC request-size can be updated for expansion, other fields remain immutable. |
docs/upgrade-notes.md |
Fixes a small typo. |
helm/solr/Chart.yaml |
Updates ArtifactHub changelog entry for the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| statusFile, err := os.Create(baseFilename + ".status.json") | ||
| defer statusFile.Close() | ||
| Expect(err).ToNot(HaveOccurred(), "Could not open file to save PVC status: %s", baseFilename+".status.json") |
There was a problem hiding this comment.
In writeAllPvcInfoToFiles, statusFile/eventsFile are deferred closed before checking the os.Create error. If Create fails, the deferred Close will panic on a nil file handle. Move the defer until after verifying err == nil (and consider handling Close errors consistently).
| if filterLinesWithString != "" { | ||
| filteredWriter := bytes.NewBufferString("") | ||
| scanner := bufio.NewScanner(podLogs) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, filterLinesWithString) { | ||
| io.WriteString(filteredWriter, line) | ||
| io.WriteString(filteredWriter, "\n") | ||
| io.WriteString(logFile, line) | ||
| io.WriteString(logFile, "\n") | ||
| } | ||
| } | ||
| logReader = filteredWriter | ||
| } else { | ||
| _, err = io.Copy(logFile, podLogs) | ||
| } | ||
|
|
||
| _, err = io.Copy(logFile, logReader) | ||
| Expect(err).ToNot(HaveOccurred(), "Could not write podLogs to file: %s", filename) | ||
| } |
There was a problem hiding this comment.
When filterLinesWithString != "", the function never assigns to err and does not check scanner.Err() / io.WriteString errors. As a result, read/write failures (including bufio.Scanner token-too-long) can be silently ignored and the final Expect(err) will still pass. Capture and assert scanner.Err(), and propagate write errors so the test output reflects failures.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
good suggestions by Copilot. Implemented, and so this should probably be done? |
|
So the integration tests are just noisier than they used to be. I'm not sure why that would be the case (after fixing the bug in the CoPilot code), but I'm going to hold-off merging until I can nail it down. |
Resolves #709