Copilot commented on code in PR #712:
URL: https://github.com/apache/solr-operator/pull/712#discussion_r3010598439
##########
controllers/solrcloud_controller.go:
##########
@@ -1018,6 +1027,46 @@ func (r *SolrCloudReconciler) reconcileZk(ctx
context.Context, logger logr.Logge
return nil
}
+func (r *SolrCloudReconciler) expandPVCs(ctx context.Context, cloud
*solrv1beta1.SolrCloud, pvcLabelSelector map[string]string, newSize
resource.Quantity, logger logr.Logger) (expansionComplete bool, err error) {
+ var pvcList corev1.PersistentVolumeClaimList
+ pvcList, err = r.getPVCList(ctx, cloud, pvcLabelSelector)
+ if err != nil {
+ return
+ }
+ expansionCompleteCount := 0
+ for _, pvcItem := range pvcList.Items {
+ if pvcExpansionComplete, e := r.expandPVC(ctx, &pvcItem,
newSize, logger); e != nil {
+ err = e
+ } else if pvcExpansionComplete {
+ expansionCompleteCount += 1
+ }
+ }
+ // If all PVCs have been expanded, then we are done
+ expansionComplete = err == nil && expansionCompleteCount ==
len(pvcList.Items)
+ return
+}
+
+func (r *SolrCloudReconciler) expandPVC(ctx context.Context, pvc
*corev1.PersistentVolumeClaim, newSize resource.Quantity, logger logr.Logger)
(expansionComplete bool, err error) {
+ // If the current capacity is >= the new size, then there is nothing to
do, expansion is complete
+ if pvc.Status.Capacity.Storage().Cmp(newSize) >= 0 {
+ // TODO: Eventually use the pvc.Status.AllocatedResources and
pvc.Status.AllocatedResourceStatuses to determine the status of PVC Expansion
and react to failures
+ expansionComplete = true
+ } else if !pvc.Spec.Resources.Requests.Storage().Equal(newSize) {
+ // Update the pvc if the capacity request is different.
+ // The newSize might be smaller than the current size, but this
is supported as the last size might have been too
+ // big for the storage quota, so it was lowered.
+ // As long as the PVCs current capacity is lower than the new
size, we are still good to update the PVC.
+ originalPvc := pvc.DeepCopy()
+ pvc.Spec.Resources.Requests[corev1.ResourceStorage] = newSize
+ if err = r.Patch(ctx, pvc,
client.StrategicMergeFrom(originalPvc)); err != nil {
+ logger.Error(err, "Error while expanding
PersistentVolumeClaim size", "persistentVolumeClaim", pvc.Name, "size", newSize)
+ } else {
+ logger.Info("Expanded PersistentVolumeClaim size",
"persistentVolumeClaim", pvc.Name, "size", newSize)
Review Comment:
expandPVC assumes pvc.Status.Capacity.Storage() and
pvc.Spec.Resources.Requests.Storage() are non-nil. For Pending/unbound PVCs (or
unusual provisioners), these can be nil, leading to a nil dereference when
calling Cmp/Equal. Guard against nil (treat missing capacity as 0, and handle
missing requests) before comparing quantities.
```suggestion
// If the current capacity is >= the new size, then there is nothing to
do, expansion is complete.
// Treat missing capacity as zero.
capacityQty, hasCapacity := pvc.Status.Capacity[corev1.ResourceStorage]
if !hasCapacity {
capacityQty = resource.Quantity{}
}
if capacityQty.Cmp(newSize) >= 0 {
// TODO: Eventually use the pvc.Status.AllocatedResources and
pvc.Status.AllocatedResourceStatuses to determine the status of PVC Expansion
and react to failures
expansionComplete = true
} else {
// Determine if the current request already matches the desired
size.
requestQty, hasRequest :=
pvc.Spec.Resources.Requests[corev1.ResourceStorage]
sameRequest := hasRequest && requestQty.Equal(newSize)
if !sameRequest {
// Update the pvc if the capacity request is different.
// The newSize might be smaller than the current size,
but this is supported as the last size might have been too
// big for the storage quota, so it was lowered.
// As long as the PVCs current capacity is lower than
the new size, we are still good to update the PVC.
originalPvc := pvc.DeepCopy()
if pvc.Spec.Resources.Requests == nil {
pvc.Spec.Resources.Requests =
corev1.ResourceList{}
}
pvc.Spec.Resources.Requests[corev1.ResourceStorage] =
newSize
if err = r.Patch(ctx, pvc,
client.StrategicMergeFrom(originalPvc)); err != nil {
logger.Error(err, "Error while expanding
PersistentVolumeClaim size", "persistentVolumeClaim", pvc.Name, "size", newSize)
} else {
logger.Info("Expanded PersistentVolumeClaim
size", "persistentVolumeClaim", pvc.Name, "size", newSize)
}
```
##########
controllers/solrcloud_controller.go:
##########
@@ -560,6 +563,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx
context.Context, req ctrl.Request) (
clusterOpQueue[queueIdx] = *clusterOp
clusterOp = nil
}
+ clusterOp, retryLaterDuration, err =
determinePvcExpansionClusterOpLockIfNecessary(instance, statefulSet)
+ // If the new clusterOperation is an update to a queued
clusterOp, just change the operation that is already queued
+ if queueIdx, opIsQueued := queuedRetryOps[UpdateLock];
clusterOp != nil && opIsQueued {
Review Comment:
This queued-op replacement check uses queuedRetryOps[UpdateLock] even though
the new operation being considered is a PVC expansion. This can incorrectly
overwrite a queued RollingUpdate op (or fail to update an already-queued
PVCExpansion op). Use queuedRetryOps[PvcExpansionLock] here instead of
UpdateLock.
```suggestion
// If the new clusterOperation is an update to a queued
PVC expansion clusterOp, just change the operation that is already queued
if queueIdx, opIsQueued :=
queuedRetryOps[PvcExpansionLock]; clusterOp != nil && opIsQueued {
```
##########
tests/e2e/suite_test.go:
##########
@@ -489,22 +530,18 @@ func writePodLogsToFile(ctx context.Context, filename
string, podName string, po
Expect(logsErr).ToNot(HaveOccurred(), "Could not open stream to fetch
pod logs. namespace: %s, pod: %s", podNamespace, podName)
defer podLogs.Close()
- var logReader io.Reader
- logReader = podLogs
-
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)
}
Review 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.
##########
docs/solr-cloud/solr-cloud-crd.md:
##########
@@ -61,8 +61,9 @@ These options can be found in `SolrCloud.spec.dataStorage`
- **`pvcTemplate`** - The template of the PVC to use for the solr data PVCs.
By default the name will be "data".
Only the `pvcTemplate.spec` field is required, metadata is optional.
- Note: This template cannot be changed unless the SolrCloud is deleted and
recreated.
- This is a [limitation of StatefulSets and PVCs in
Kubernetes](https://github.com/kubernetes/enhancements/issues/661).
+ Note: Currently, [Kubernetes does not support PVC resizing (expanding) in
StatefulSets](https://github.com/kubernetes/enhancements/issues/661).
+ However, The Solr Operator will manage the PVC expansion for users until
this is supported by default in Kubernetes.
Review Comment:
Grammar/capitalization: "However, The Solr Operator" should be "However, the
Solr Operator".
```suggestion
However, the Solr Operator will manage the PVC expansion for users until
this is supported by default in Kubernetes.
```
##########
tests/e2e/suite_test.go:
##########
@@ -408,6 +423,32 @@ func writeAllStatefulSetInfoToFiles(baseFilename string,
statefulSet *appsv1.Sta
Expect(writeErr).ToNot(HaveOccurred(), "Could not write statefulSet
events json to file")
}
+// writeAllPvcInfoToFiles writes the following each to a separate file with
the given base name & directory.
+// - PVC Spec/Status
+// - PVC Events
+func writeAllPvcInfoToFiles(baseFilename string, pvc
*corev1.PersistentVolumeClaim) {
+ // Write PVC to a file
+ 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")
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
