From f2384f5285497b3bf0fbb5b5cbd2007085fe9de9 Mon Sep 17 00:00:00 2001 From: Andrew Gouin Date: Fri, 27 Oct 2023 09:46:24 -0600 Subject: [PATCH] deep copy resources --- api/v1/cosmosfullnode_types.go | 2 +- .../cosmos.strange.love_cosmosfullnodes.yaml | 10 ++-- internal/fullnode/pod_builder_test.go | 5 ++ internal/fullnode/pvc_builder.go | 11 ++-- internal/fullnode/pvc_builder_test.go | 51 ++++++------------- internal/fullnode/pvc_control.go | 32 +++++++----- internal/fullnode/pvc_control_test.go | 16 ++---- 7 files changed, 58 insertions(+), 69 deletions(-) diff --git a/api/v1/cosmosfullnode_types.go b/api/v1/cosmosfullnode_types.go index 9bbb8cdc..eae4ce89 100644 --- 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 index e79930d7..581a229d 100644 --- 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 index 27be4668..96fc7d20 100644 --- 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 index 78582f15..012c6944 100644 --- 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 index 85f1c6d0..dca1390b 100644 --- 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 index ea10a1f9..6786f377 100644 --- 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 index a448d263..584e7d6c 100644 --- 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)