deep copy resources

strangelove-ventures · Oct 27, 2023 · f2384f5 · f2384f5
1 parent 26279fb
commit f2384f5
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 69 deletions.
diff --git a/api/v1/cosmosfullnode_types.go b/api/v1/cosmosfullnode_types.go
@@ -387,7 +387,7 @@ type AutoDataSource struct {
 	VolumeSnapshotSelector map[string]string `json:"volumeSnapshotSelector"`
 
 	// If true, the volume snapshot selector will make sure the PVC
-	// is restored for the same instance as the VolumeSnapshot.
+	// is restored from a VolumeSnapshot on the same node.
 	// This is useful if the VolumeSnapshots are local to the node, e.g. for topolvm.
 	MatchInstance bool `json:"matchInstance"`
 }

diff --git a/config/crd/bases/cosmos.strange.love_cosmosfullnodes.yaml b/config/crd/bases/cosmos.strange.love_cosmosfullnodes.yaml
@@ -388,8 +388,8 @@ spec:
                           properties:
                             matchInstance:
                               description: If true, the volume snapshot selector will
-                                make sure the PVC is restored for the same instance
-                                as the VolumeSnapshot. This is useful if the VolumeSnapshots
+                                make sure the PVC is restored from a VolumeSnapshot
+                                on the same node. This is useful if the VolumeSnapshots
                                 are local to the node, e.g. for topolvm.
                               type: boolean
                             volumeSnapshotSelector:
@@ -5824,9 +5824,9 @@ spec:
                     properties:
                       matchInstance:
                         description: If true, the volume snapshot selector will make
-                          sure the PVC is restored for the same instance as the VolumeSnapshot.
-                          This is useful if the VolumeSnapshots are local to the node,
-                          e.g. for topolvm.
+                          sure the PVC is restored from a VolumeSnapshot on the same
+                          node. This is useful if the VolumeSnapshots are local to
+                          the node, e.g. for topolvm.
                         type: boolean
                       volumeSnapshotSelector:
                         additionalProperties:

diff --git a/internal/fullnode/pod_builder_test.go b/internal/fullnode/pod_builder_test.go
@@ -37,6 +37,11 @@ func defaultCRD() cosmosv1.CosmosFullNode {
 					},
 				},
 			},
+			VolumeClaimTemplate: cosmosv1.PersistentVolumeClaimSpec{
+				Resources: corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("100Gi")},
+				},
+			},
 		},
 	}
 }

diff --git a/internal/fullnode/pvc_builder.go b/internal/fullnode/pvc_builder.go
@@ -56,7 +56,9 @@ func BuildPVCs(
 		} else {
 			for _, pvc := range currentPVCs {
 				if pvc.Name == name {
-					existingSize = pvc.Spec.Resources.Requests[corev1.ResourceStorage]
+					if pvc.DeletionTimestamp == nil && pvc.Status.Phase == corev1.ClaimBound {
+						existingSize = pvc.Status.Capacity[corev1.ResourceStorage]
+					}
 					break
 				}
 			}
@@ -82,6 +84,7 @@ func BuildPVCs(
 		kube.NormalizeMetadata(&pvc.ObjectMeta)
 
 		pvcs = append(pvcs, diff.Adapt(pvc, i))
+		pvc.Spec.DataSource = dataSource
 	}
 	return pvcs
 }
@@ -103,11 +106,11 @@ func pvcResources(
 	dataSource *dataSource,
 	existingSize resource.Quantity,
 ) corev1.ResourceRequirements {
-	var reqs = crd.Spec.VolumeClaimTemplate.Resources
+	var reqs = crd.Spec.VolumeClaimTemplate.Resources.DeepCopy()
 
 	if dataSource != nil {
 		reqs.Requests[corev1.ResourceStorage] = dataSource.size
-		return reqs
+		return *reqs
 	}
 
 	if autoScale := crd.Status.SelfHealing.PVCAutoScale; autoScale != nil {
@@ -125,5 +128,5 @@ func pvcResources(
 		reqs.Requests[corev1.ResourceStorage] = existingSize
 	}
 
-	return reqs
+	return *reqs
 }
diff --git a/internal/fullnode/pvc_builder_test.go b/internal/fullnode/pvc_builder_test.go
@@ -22,12 +22,7 @@ func TestBuildPVCs(t *testing.T) {
 		crd := defaultCRD()
 		crd.Name = "juno"
 		crd.Spec.Replicas = 3
-		crd.Spec.VolumeClaimTemplate = cosmosv1.PersistentVolumeClaimSpec{
-			StorageClassName: "test-storage-class",
-			Resources: corev1.ResourceRequirements{
-				Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("100G")},
-			},
-		}
+		crd.Spec.VolumeClaimTemplate.StorageClassName = "test-storage-class"
 
 		crd.Spec.InstanceOverrides = map[string]cosmosv1.InstanceOverridesSpec{
 			"juno-0": {},
@@ -80,20 +75,15 @@ func TestBuildPVCs(t *testing.T) {
 	t.Run("advanced configuration", func(t *testing.T) {
 		crd := defaultCRD()
 		crd.Spec.Replicas = 1
-		crd.Spec.VolumeClaimTemplate = cosmosv1.PersistentVolumeClaimSpec{
-			Metadata: cosmosv1.Metadata{
-				Labels:      map[string]string{"label": "value", "app.kubernetes.io/created-by": "should not see me"},
-				Annotations: map[string]string{"annot": "value"},
-			},
-			AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany},
-			VolumeMode:  ptr(corev1.PersistentVolumeBlock),
-			DataSource: &corev1.TypedLocalObjectReference{
-				Kind: "TestKind",
-				Name: "source-name",
-			},
-			Resources: corev1.ResourceRequirements{
-				Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("100G")},
-			},
+		crd.Spec.VolumeClaimTemplate.Metadata = cosmosv1.Metadata{
+			Labels:      map[string]string{"label": "value", "app.kubernetes.io/created-by": "should not see me"},
+			Annotations: map[string]string{"annot": "value"},
+		}
+		crd.Spec.VolumeClaimTemplate.AccessModes = []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany}
+		crd.Spec.VolumeClaimTemplate.VolumeMode = ptr(corev1.PersistentVolumeBlock)
+		crd.Spec.VolumeClaimTemplate.DataSource = &corev1.TypedLocalObjectReference{
+			Kind: "TestKind",
+			Name: "source-name",
 		}
 
 		pvcs := BuildPVCs(&crd, map[int32]*dataSource{
@@ -170,11 +160,8 @@ func TestBuildPVCs(t *testing.T) {
 			} {
 				crd := defaultCRD()
 				crd.Spec.Replicas = 1
-				crd.Spec.VolumeClaimTemplate = cosmosv1.PersistentVolumeClaimSpec{
-					Resources: corev1.ResourceRequirements{
-						Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse(tt.SpecQuant)},
-					},
-				}
+				crd.Spec.VolumeClaimTemplate.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(tt.SpecQuant)
+
 				crd.Status.SelfHealing.PVCAutoScale = map[string]*cosmosv1.PVCAutoScaleStatus{
 					"pvc-osmosis-0": {
 						RequestedSize: resource.MustParse(tt.AutoScaleQuant),
@@ -197,11 +184,8 @@ func TestBuildPVCs(t *testing.T) {
 			} {
 				crd := defaultCRD()
 				crd.Spec.Replicas = 1
-				crd.Spec.VolumeClaimTemplate = cosmosv1.PersistentVolumeClaimSpec{
-					Resources: corev1.ResourceRequirements{
-						Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse(tt.SpecQuant)},
-					},
-				}
+				crd.Spec.VolumeClaimTemplate.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(tt.SpecQuant)
+
 				crd.Status.SelfHealing.PVCAutoScale = map[string]*cosmosv1.PVCAutoScaleStatus{
 					"pvc-osmosis-0": {
 						RequestedSize: resource.MustParse(tt.AutoScaleQuant),
@@ -224,11 +208,8 @@ func TestBuildPVCs(t *testing.T) {
 			} {
 				crd := defaultCRD()
 				crd.Spec.Replicas = 1
-				crd.Spec.VolumeClaimTemplate = cosmosv1.PersistentVolumeClaimSpec{
-					Resources: corev1.ResourceRequirements{
-						Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse(tt.SpecQuant)},
-					},
-				}
+				crd.Spec.VolumeClaimTemplate.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(tt.SpecQuant)
+
 				crd.Status.SelfHealing.PVCAutoScale = map[string]*cosmosv1.PVCAutoScaleStatus{
 					"pvc-osmosis-0": {
 						RequestedSize: resource.MustParse(tt.AutoScaleQuant),

diff --git a/internal/fullnode/pvc_control.go b/internal/fullnode/pvc_control.go
@@ -3,7 +3,6 @@ package fullnode
 import (
 	"context"
 	"fmt"
-	"strconv"
 
 	snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	"github.com/samber/lo"
@@ -62,7 +61,13 @@ func (control PVCControl) Reconcile(ctx context.Context, reporter kube.Reporter,
 				}
 			}
 			if !found {
-				dataSources[i] = control.findDataSource(ctx, reporter, crd, i)
+				ds := control.findDataSource(ctx, reporter, crd, i)
+				if ds == nil {
+					ds = &dataSource{
+						size: crd.Spec.VolumeClaimTemplate.Resources.Requests[corev1.ResourceStorage],
+					}
+				}
+				dataSources[i] = ds
 			}
 		}
 	}
@@ -73,15 +78,13 @@ func (control PVCControl) Reconcile(ctx context.Context, reporter kube.Reporter,
 	)
 
 	for _, pvc := range diffed.Creates() {
-		ordinal, err := strconv.ParseInt(pvc.Annotations[kube.OrdinalAnnotation], 10, 32)
-		if err != nil {
-			return true, kube.TransientError(fmt.Errorf("parse ordinal from pvc %s: %w", pvc.Name, err))
-		}
-		dataSource, ok := dataSources[int32(ordinal)]
-		if ok && dataSource != nil {
-			pvc.Spec.DataSource = dataSource.ref
-		}
-		reporter.Info("Creating pvc", "name", pvc.Name)
+		size := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
+
+		reporter.Info(
+			"Creating pvc",
+			"name", pvc.Name,
+			"size", size.String(),
+		)
 		if err := ctrl.SetControllerReference(crd, pvc, control.client.Scheme()); err != nil {
 			return true, kube.TransientError(fmt.Errorf("set controller reference on pvc %q: %w", pvc.Name, err))
 		}
@@ -120,7 +123,12 @@ func (control PVCControl) Reconcile(ctx context.Context, reporter kube.Reporter,
 
 	// PVCs have many immutable fields, so only update the storage size.
 	for _, pvc := range diffed.Updates() {
-		reporter.Info("Patching pvc", "name", pvc.Name)
+		size := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
+		reporter.Info(
+			"Patching pvc",
+			"name", pvc.Name,
+			"size", size.String(), // TODO remove expensive operation
+		)
 		patch := corev1.PersistentVolumeClaim{
 			ObjectMeta: pvc.ObjectMeta,
 			TypeMeta:   pvc.TypeMeta,

diff --git a/internal/fullnode/pvc_control_test.go b/internal/fullnode/pvc_control_test.go
@@ -111,9 +111,7 @@ func TestPVCControl_Reconcile(t *testing.T) {
 		crd.Spec.VolumeClaimTemplate.AutoDataSource = &cosmosv1.AutoDataSource{
 			VolumeSnapshotSelector: map[string]string{"label": "vol-snapshot"},
 		}
-		crd.Spec.VolumeClaimTemplate.Resources = corev1.ResourceRequirements{
-			Requests: corev1.ResourceList{"storage": resource.MustParse("100Gi")},
-		}
+
 		var volCallCount int
 		control.recentVolumeSnapshot = func(ctx context.Context, lister kube.Lister, namespace string, selector map[string]string) (*snapshotv1.VolumeSnapshot, error) {
 			require.NotNil(t, ctx)
@@ -165,9 +163,6 @@ func TestPVCControl_Reconcile(t *testing.T) {
 			VolumeSnapshotSelector: map[string]string{"label": "vol-snapshot"},
 		}
 		crd.Spec.VolumeClaimTemplate.DataSource = crdDataSource
-		crd.Spec.VolumeClaimTemplate.Resources = corev1.ResourceRequirements{
-			Requests: corev1.ResourceList{"storage": resource.MustParse("100Gi")},
-		}
 
 		control.recentVolumeSnapshot = func(ctx context.Context, lister kube.Lister, namespace string, selector map[string]string) (*snapshotv1.VolumeSnapshot, error) {
 			panic("should not be called")
@@ -238,9 +233,7 @@ func TestPVCControl_Reconcile(t *testing.T) {
 
 		// Cause a change
 		crd.Spec.VolumeClaimTemplate.VolumeMode = ptr(corev1.PersistentVolumeMode("should not be in the patch"))
-		crd.Spec.VolumeClaimTemplate.Resources = corev1.ResourceRequirements{
-			Requests: corev1.ResourceList{"memory": resource.MustParse("1Gi")},
-		}
+		crd.Spec.VolumeClaimTemplate.Resources.Requests["memory"] = resource.MustParse("1Gi")
 
 		control := testPVCControl(&mClient)
 		requeue, rerr := control.Reconcile(ctx, nopReporter, &crd, &PVCStatusChanges{})
@@ -274,9 +267,8 @@ func TestPVCControl_Reconcile(t *testing.T) {
 		}
 
 		// Cause a change
-		crd.Spec.VolumeClaimTemplate.Resources = corev1.ResourceRequirements{
-			Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("1Ti")},
-		}
+		crd.Spec.VolumeClaimTemplate.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("1Ti")
+
 		control := testPVCControl(&mClient)
 		requeue, rerr := control.Reconcile(ctx, nopReporter, &crd, &PVCStatusChanges{})
 		require.NoError(t, rerr)