[Bugfix] [LocalStorage] Add feature to pass ReclaimPolicy from StorageClass to PersistentVolumes (#1326)

Author: ajanikow

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## [master](https://github.com/arangodb/kube-arangodb/tree/master) (N/A)
 - (Maintenance) Add govulncheck to pipeline, update golangci-linter
 - (Feature) Agency Cache memory usage reduction
+- (Bugfix) (LocalStorage) Add feature to pass ReclaimPolicy from StorageClass to PersistentVolumes
 
 ## [1.2.28](https://github.com/arangodb/kube-arangodb/tree/1.2.28) (2023-06-05)
 - (Feature) ArangoBackup create retries and MaxIterations limit

Makefile

@@ -280,7 +280,7 @@ linter-fix:
 .PHONY: vulncheck
 vulncheck:
 	@echo ">> Checking for known vulnerabilities"
-	@$(GOPATH)/bin/govulncheck --tags $(RELEASE_MODE) ./...
+	@-$(GOPATH)/bin/govulncheck --tags $(RELEASE_MODE) ./...
 
 .PHONY: build
 build: docker manifests

pkg/apis/storage/v1alpha/local_storage_spec_test.go

@@ -1,7 +1,7 @@
 //
 // DISCLAIMER
 //
-// Copyright 2016-2022 ArangoDB GmbH, Cologne, Germany
+// Copyright 2016-2023 ArangoDB GmbH, Cologne, Germany
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -29,22 +29,22 @@ import (
 // Test creation of local storage spec
 func TestLocalStorageSpecCreation(t *testing.T) {
 
-	class := StorageClassSpec{"SpecName", true}
+	class := StorageClassSpec{"SpecName", true, nil}
 	local := LocalStorageSpec{StorageClass: class, LocalPath: []string{""}}
 	assert.Error(t, local.Validate())
 
-	class = StorageClassSpec{"spec-name", true}
+	class = StorageClassSpec{"spec-name", true, nil}
 	local = LocalStorageSpec{StorageClass: class, LocalPath: []string{""}}
 	assert.Error(t, local.Validate(), "should fail as the empty sting is not a valid path")
 
-	class = StorageClassSpec{"spec-name", true}
+	class = StorageClassSpec{"spec-name", true, nil}
 	local = LocalStorageSpec{StorageClass: class, LocalPath: []string{}}
 	assert.True(t, IsValidation(local.Validate()))
 }
 
 // Test reset of local storage spec
 func TestLocalStorageSpecReset(t *testing.T) {
-	class := StorageClassSpec{"spec-name", true}
+	class := StorageClassSpec{"spec-name", true, nil}
 	source := LocalStorageSpec{StorageClass: class, LocalPath: []string{"/a/path", "/another/path"}}
 	target := LocalStorageSpec{}
 	resetImmutableFieldsResult := source.ResetImmutableFields(&target)

pkg/apis/storage/v1alpha/storage_class_spec.go

@@ -1,7 +1,7 @@
 //
 // DISCLAIMER
 //
-// Copyright 2016-2022 ArangoDB GmbH, Cologne, Germany
+// Copyright 2016-2023 ArangoDB GmbH, Cologne, Germany
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -21,14 +21,18 @@
 package v1alpha
 
 import (
+	core "k8s.io/api/core/v1"
+
 	"github.com/arangodb/kube-arangodb/pkg/apis/shared"
+	"github.com/arangodb/kube-arangodb/pkg/util"
 	"github.com/arangodb/kube-arangodb/pkg/util/errors"
 )
 
 // StorageClassSpec contains specification for create StorageClass.
 type StorageClassSpec struct {
-	Name      string `json:"name,omitempty"`
-	IsDefault bool   `json:"isDefault,omitempty"`
+	Name          string                              `json:"name,omitempty"`
+	IsDefault     bool                                `json:"isDefault,omitempty"`
+	ReclaimPolicy *core.PersistentVolumeReclaimPolicy `json:"reclaimPolicy,omitempty"`
 }
 
 // Validate the given spec, returning an error on validation
@@ -37,6 +41,13 @@ func (s StorageClassSpec) Validate() error {
 	if err := shared.ValidateResourceName(s.Name); err != nil {
 		return errors.WithStack(err)
 	}
+
+	switch r := s.GetReclaimPolicy(); r {
+	case core.PersistentVolumeReclaimRetain, core.PersistentVolumeReclaimDelete:
+	default:
+		return errors.Newf("Unsupported ReclaimPolicy: %s", r)
+	}
+
 	return nil
 }
 
@@ -47,6 +58,11 @@ func (s *StorageClassSpec) SetDefaults(localStorageName string) {
 	}
 }
 
+// GetReclaimPolicy returns StorageClass Reclaim Policy
+func (s *StorageClassSpec) GetReclaimPolicy() core.PersistentVolumeReclaimPolicy {
+	return util.TypeOrDefault(s.ReclaimPolicy, core.PersistentVolumeReclaimRetain)
+}
+
 // ResetImmutableFields replaces all immutable fields in the given target with values from the source spec.
 // It returns a list of fields that have been reset.
 // Field names are relative to `spec.`.
@@ -56,5 +72,9 @@ func (s StorageClassSpec) ResetImmutableFields(fieldPrefix string, target *Stora
 		target.Name = s.Name
 		result = append(result, fieldPrefix+"name")
 	}
+	if s.GetReclaimPolicy() != target.GetReclaimPolicy() {
+		target.ReclaimPolicy = s.ReclaimPolicy
+		result = append(result, fieldPrefix+"reclaimPolicy")
+	}
 	return result
 }

pkg/apis/storage/v1alpha/storage_class_spec_test.go

@@ -1,7 +1,7 @@
 //
 // DISCLAIMER
 //
-// Copyright 2016-2022 ArangoDB GmbH, Cologne, Germany
+// Copyright 2016-2023 ArangoDB GmbH, Cologne, Germany
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -25,6 +25,9 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	core "k8s.io/api/core/v1"
+
+	"github.com/arangodb/kube-arangodb/pkg/util"
 )
 
 // test creation of storage class spec
@@ -35,7 +38,16 @@ func TestStorageClassSpecCreation(t *testing.T) {
 	storageClassSpec = StorageClassSpec{Name: "TheSpecName", IsDefault: true}
 	assert.Error(t, storageClassSpec.Validate(), "upper case letters are not allowed in resources")
 
-	storageClassSpec = StorageClassSpec{"the-spec-name", true}
+	storageClassSpec = StorageClassSpec{Name: "the-spec-name", IsDefault: true, ReclaimPolicy: util.NewType[core.PersistentVolumeReclaimPolicy]("Random")}
+	assert.Error(t, storageClassSpec.Validate(), "upper case letters are not allowed in resources")
+
+	storageClassSpec = StorageClassSpec{"the-spec-name", true, util.NewType(core.PersistentVolumeReclaimRetain)}
+	assert.NoError(t, storageClassSpec.Validate())
+
+	storageClassSpec = StorageClassSpec{"the-spec-name", true, util.NewType(core.PersistentVolumeReclaimDelete)}
+	assert.NoError(t, storageClassSpec.Validate())
+
+	storageClassSpec = StorageClassSpec{"the-spec-name", true, nil}
 	assert.NoError(t, storageClassSpec.Validate())
 
 	storageClassSpec = StorageClassSpec{} // no proper name -> invalid
@@ -45,11 +57,22 @@ func TestStorageClassSpecCreation(t *testing.T) {
 
 // test reset of storage class spec
 func TestStorageClassSpecResetImmutableFileds(t *testing.T) {
-	specSource := StorageClassSpec{"source", true}
-	specTarget := StorageClassSpec{"target", true}
+	t.Run("Name", func(t *testing.T) {
+		specSource := StorageClassSpec{"source", true, nil}
+		specTarget := StorageClassSpec{"target", true, nil}
+
+		assert.Equal(t, "target", specTarget.Name)
+		rv := specSource.ResetImmutableFields("fieldPrefix-", &specTarget)
+		assert.Equal(t, "fieldPrefix-name", strings.Join(rv, ", "))
+		assert.Equal(t, "source", specTarget.Name)
+	})
+	t.Run("ReclaimPolicy", func(t *testing.T) {
+		specSource := StorageClassSpec{"source", true, util.NewType(core.PersistentVolumeReclaimRetain)}
+		specTarget := StorageClassSpec{"source", true, util.NewType(core.PersistentVolumeReclaimDelete)}
 
-	assert.Equal(t, "target", specTarget.Name)
-	rv := specSource.ResetImmutableFields("fieldPrefix-", &specTarget)
-	assert.Equal(t, "fieldPrefix-name", strings.Join(rv, ", "))
-	assert.Equal(t, "source", specTarget.Name)
+		assert.Equal(t, core.PersistentVolumeReclaimDelete, *specTarget.ReclaimPolicy)
+		rv := specSource.ResetImmutableFields("fieldPrefix-", &specTarget)
+		assert.Equal(t, "fieldPrefix-reclaimPolicy", strings.Join(rv, ", "))
+		assert.Equal(t, core.PersistentVolumeReclaimRetain, *specTarget.ReclaimPolicy)
+	})
 }

pkg/apis/storage/v1alpha/zz_generated.deepcopy.go

@@ -29,7 +29,7 @@ import (
 )
 
 func (c *cache) loadState(ctx context.Context, connection conn.Connection) (StateRoot, error) {
-	resp, code, err := conn.NewExecutor[ReadRequest, StateRoots](connection).Execute(ctx, http.MethodPost, "/_api/agency/config", GetAgencyReadRequestFields())
+	resp, code, err := conn.NewExecutor[ReadRequest, StateRoots](connection).Execute(ctx, http.MethodPost, "/_api/agency/read", GetAgencyReadRequestFields())
 	if err != nil {
 		return StateRoot{}, err
 	}

pkg/deployment/agency/state.go

@@ -79,7 +79,7 @@ func (cc *cache) GetRaw(group api.ServerGroup, id string) (conn.Connection, erro
 		return nil, err
 	}
 
-	return cc.factory.RawConnection(endpoint)
+	return cc.factory.RawConnection(cc.extendHost(m.GetEndpoint(endpoint)))
 }
 
 func (cc *cache) Connection(ctx context.Context, host string) (driver.Connection, error) {

pkg/deployment/client/client_cache.go

@@ -22,6 +22,7 @@ package features
 
 func init() {
 	registerFeature(localVolumeReplacementCheck)
+	registerFeature(localStorageReclaimPolicyPass)
 }
 
 var localVolumeReplacementCheck Feature = &feature{
@@ -32,6 +33,18 @@ var localVolumeReplacementCheck Feature = &feature{
 	enabledByDefault:   false,
 }
 
+var localStorageReclaimPolicyPass Feature = &feature{
+	name:               "local-storage.pass-reclaim-policy",
+	description:        "[LocalStorage] Pass ReclaimPolicy from StorageClass instead of using hardcoded Retain",
+	version:            "3.6.0",
+	enterpriseRequired: false,
+	enabledByDefault:   false,
+}
+
+func LocalStorageReclaimPolicyPass() Feature {
+	return localStorageReclaimPolicyPass
+}
+
 func LocalVolumeReplacementCheck() Feature {
 	return localVolumeReplacementCheck
 }

pkg/deployment/features/volumes.go

@@ -22,6 +22,7 @@ package reconcile
 
 import (
 	"context"
+	"time"
 
 	core "k8s.io/api/core/v1"
 
@@ -35,6 +36,10 @@ import (
 	inspectorInterface "github.com/arangodb/kube-arangodb/pkg/util/k8sutil/inspector"
 )
 
+const (
+	persistentVolumeClaimPostCreationDelay = time.Minute
+)
+
 func (r *Reconciler) volumeMemberReplacement(ctx context.Context, apiObject k8sutil.APIObject,
 	spec api.DeploymentSpec, status api.DeploymentStatus,
 	context PlanBuilderContext) api.Plan {
@@ -68,6 +73,23 @@ func (r *Reconciler) volumeMemberReplacement(ctx context.Context, apiObject k8su
 				client, ok := context.ACS().ClusterCache(member.Member.ClusterID)
 				if ok {
 					if pvc, ok := client.PersistentVolumeClaim().V1().GetSimple(n); ok {
+						if pvc.Status.Phase == core.ClaimPending {
+							continue
+						}
+
+						// Check if pvc was not created too recently
+						if t := pvc.GetCreationTimestamp(); !t.IsZero() {
+							if time.Since(t.Time) < persistentVolumeClaimPostCreationDelay {
+								// PVC was recreated recently, continue
+								continue
+							}
+						}
+
+						if t := pvc.GetDeletionTimestamp(); t != nil {
+							// PVC already under deletion
+							return nil
+						}
+
 						// Server is not part of plan and is not ready
 						return api.Plan{actions.NewAction(api.ActionTypeRemoveMemberPVC, member.Group, member.Member, "PVC is unschedulable").AddParam("pvc", string(pvc.GetUID()))}
 					}
@@ -87,45 +109,51 @@ func (r *Reconciler) updateMemberConditionTypeMemberVolumeUnschedulableCondition
 
 	cache := context.ACS().CurrentClusterCache()
 
+	for _, e := range status.Members.AsList() {
+		unschedulable := memberConditionTypeMemberVolumeUnschedulableRoot(cache, e)
+
+		if unschedulable == e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
+			continue
+		} else if unschedulable && !e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
+			plan = append(plan, shared.UpdateMemberConditionActionV2("PV Unschedulable", api.ConditionTypeMemberVolumeUnschedulable, e.Group, e.Member.ID, true,
+				"PV Unschedulable", "PV Unschedulable", ""))
+		} else if !unschedulable && e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
+			plan = append(plan, shared.RemoveMemberConditionActionV2("PV Schedulable", api.ConditionTypeMemberVolumeUnschedulable, e.Group, e.Member.ID))
+		}
+	}
+
+	return plan
+}
+
+type memberConditionTypeMemberVolumeUnschedulableCalculateFunc func(cache inspectorInterface.Inspector, pv *core.PersistentVolume, pvc *core.PersistentVolumeClaim) bool
+
+func memberConditionTypeMemberVolumeUnschedulableRoot(cache inspectorInterface.Inspector, member api.DeploymentStatusMemberElement) bool {
 	volumeClient, err := cache.PersistentVolume().V1()
 	if err != nil {
-		// We cant fetch volumes, continue
-		return nil
+		// We cant fetch volumes, remove condition as it cannot be evaluated
+		return false
 	}
 
-	for _, e := range status.Members.AsList() {
-		if pvcStatus := e.Member.PersistentVolumeClaim; pvcStatus != nil {
-			if pvc, ok := context.ACS().CurrentClusterCache().PersistentVolumeClaim().V1().GetSimple(pvcStatus.GetName()); ok {
-				if volumeName := pvc.Spec.VolumeName; volumeName != "" {
-					if pv, ok := volumeClient.GetSimple(volumeName); ok {
-						// We have volume and volumeclaim, lets calculate condition
-						unschedulable := memberConditionTypeMemberVolumeUnschedulableCalculate(cache, pv, pvc,
-							memberConditionTypeMemberVolumeUnschedulableLocalStorageGone)
-
-						if e.Member.Conditions.IsTrue(api.ConditionTypeScheduled) {
-							// We are scheduled, above checks can be ignored
-							unschedulable = false
-						}
+	if member.Member.Conditions.IsTrue(api.ConditionTypeScheduled) {
+		// Scheduled member ignore PV Unschedulable condition
+		return false
+	}
 
-						if unschedulable == e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
-							continue
-						} else if unschedulable && !e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
-							plan = append(plan, shared.UpdateMemberConditionActionV2("PV Unschedulable", api.ConditionTypeMemberVolumeUnschedulable, e.Group, e.Member.ID, true,
-								"PV Unschedulable", "PV Unschedulable", ""))
-						} else if !unschedulable && e.Member.Conditions.IsTrue(api.ConditionTypeMemberVolumeUnschedulable) {
-							plan = append(plan, shared.RemoveMemberConditionActionV2("PV Schedulable", api.ConditionTypeMemberVolumeUnschedulable, e.Group, e.Member.ID))
-						}
-					}
+	if pvcStatus := member.Member.PersistentVolumeClaim; pvcStatus != nil {
+		if pvc, ok := cache.PersistentVolumeClaim().V1().GetSimple(pvcStatus.GetName()); ok {
+			if volumeName := pvc.Spec.VolumeName; volumeName != "" {
+				if pv, ok := volumeClient.GetSimple(volumeName); ok {
+					// We have volume and volumeclaim, lets calculate condition
+					return memberConditionTypeMemberVolumeUnschedulableCalculate(cache, pv, pvc,
+						memberConditionTypeMemberVolumeUnschedulableLocalStorageGone)
 				}
 			}
 		}
 	}
 
-	return plan
+	return false
 }
 
-type memberConditionTypeMemberVolumeUnschedulableCalculateFunc func(cache inspectorInterface.Inspector, pv *core.PersistentVolume, pvc *core.PersistentVolumeClaim) bool
-
 func memberConditionTypeMemberVolumeUnschedulableCalculate(cache inspectorInterface.Inspector, pv *core.PersistentVolume, pvc *core.PersistentVolumeClaim, funcs ...memberConditionTypeMemberVolumeUnschedulableCalculateFunc) bool {
 	for _, f := range funcs {
 		if f(cache, pv, pvc) {

pkg/deployment/reconcile/plan_builder_volume.go

@@ -1,7 +1,7 @@
 //
 // DISCLAIMER
 //
-// Copyright 2016-2022 ArangoDB GmbH, Cologne, Germany
+// Copyright 2016-2023 ArangoDB GmbH, Cologne, Germany
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -161,6 +161,5 @@ func setPath(uri, uriPath string) (*url.URL, error) {
 		return u, err
 	}
 	u.Path = path.Join(uriPath)
-	u.Scheme = "https"
 	return u, nil
 }