diff --git a/changelogs/unreleased/5101-ywk253100 b/changelogs/unreleased/5101-ywk253100 new file mode 100644 index 000000000..ade00f2a9 --- /dev/null +++ b/changelogs/unreleased/5101-ywk253100 @@ -0,0 +1 @@ + Fix bsl validation bug: the BSL is validated continually and doesn't respect the validation period configured \ No newline at end of file diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index ec35a8916..1b08da897 100644 --- a/pkg/controller/backup_storage_location_controller.go +++ b/pkg/controller/backup_storage_location_controller.go @@ -24,12 +24,10 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/vmware-tanzu/velero/internal/storage" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -39,7 +37,10 @@ import ( ) const ( - backupStorageLocationSyncPeriod = 1 * time.Minute + // keep the enqueue period a smaller value to make sure the BSL can be validated as expected. + // The BSL validation frequency is 1 minute by default, if we set the enqueue period as 1 minute, + // this will cause the actual validation interval for each BSL to be 2 minutes + bslValidationEnqueuePeriod = 10 * time.Second ) // BackupStorageLocationReconciler reconciles a BackupStorageLocation object @@ -185,7 +186,7 @@ func (r *BackupStorageLocationReconciler) SetupWithManager(mgr ctrl.Manager) err r.Log, mgr.GetClient(), &velerov1api.BackupStorageLocationList{}, - backupStorageLocationSyncPeriod, + bslValidationEnqueuePeriod, // Add filter function to enqueue BSL per ValidationFrequency setting. func(object client.Object) bool { location := object.(*velerov1api.BackupStorageLocation) @@ -193,22 +194,8 @@ func (r *BackupStorageLocationReconciler) SetupWithManager(mgr ctrl.Manager) err }, ) return ctrl.NewControllerManagedBy(mgr). - For(&velerov1api.BackupStorageLocation{}). - // Handle BSL's creation event and spec update event to let changed BSL got validation immediately. - WithEventFilter(predicate.Funcs{ - CreateFunc: func(ce event.CreateEvent) bool { - return true - }, - UpdateFunc: func(ue event.UpdateEvent) bool { - return ue.ObjectNew.GetGeneration() != ue.ObjectOld.GetGeneration() - }, - DeleteFunc: func(de event.DeleteEvent) bool { - return false - }, - GenericFunc: func(ge event.GenericEvent) bool { - return false - }, - }). + // As the "status.LastValidationTime" field is always updated, this triggers new reconciling process, skip the update event that include no spec change to avoid the reconcile loop + For(&velerov1api.BackupStorageLocation{}, builder.WithPredicates(kube.SpecChangePredicate{})). Watches(g, nil). Complete(r) } diff --git a/pkg/util/kube/predicate.go b/pkg/util/kube/predicate.go new file mode 100644 index 000000000..3073ef881 --- /dev/null +++ b/pkg/util/kube/predicate.go @@ -0,0 +1,47 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package kube + +import ( + "reflect" + + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// SpecChangePredicate implements a default update predicate function on Spec change +// As Velero doesn't enable subresource in CRDs, we cannot use the object's metadata.generation field to check the spec change +// More details about the generation field refer to https://github.com/kubernetes-sigs/controller-runtime/blob/v0.12.2/pkg/predicate/predicate.go#L156 +type SpecChangePredicate struct { + predicate.Funcs +} + +func (SpecChangePredicate) Update(e event.UpdateEvent) bool { + if e.ObjectOld == nil { + return false + } + if e.ObjectNew == nil { + return false + } + oldSpec := reflect.ValueOf(e.ObjectOld).Elem().FieldByName("Spec") + // contains no field named "Spec", return false directly + if oldSpec.IsZero() { + return false + } + newSpec := reflect.ValueOf(e.ObjectNew).Elem().FieldByName("Spec") + return !reflect.DeepEqual(oldSpec.Interface(), newSpec.Interface()) +} diff --git a/pkg/util/kube/predicate_test.go b/pkg/util/kube/predicate_test.go new file mode 100644 index 000000000..d1c3be8df --- /dev/null +++ b/pkg/util/kube/predicate_test.go @@ -0,0 +1,180 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package kube + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + corev1api "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" +) + +func TestSpecChangePredicate(t *testing.T) { + cases := []struct { + name string + oldObj client.Object + newObj client.Object + changed bool + }{ + { + name: "Contains no spec field", + oldObj: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bsl01", + }, + }, + newObj: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bsl01", + }, + }, + changed: false, + }, + { + name: "ObjectMetas are different, Specs are same", + oldObj: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bsl01", + Annotations: map[string]string{"key1": "value1"}, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + }, + }, + newObj: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bsl01", + Annotations: map[string]string{"key2": "value2"}, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + }, + }, + changed: false, + }, + { + name: "Statuses are different, Specs are same", + oldObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + }, + Status: velerov1.BackupStorageLocationStatus{ + Phase: velerov1.BackupStorageLocationPhaseAvailable, + }, + }, + newObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + }, + Status: velerov1.BackupStorageLocationStatus{ + Phase: velerov1.BackupStorageLocationPhaseUnavailable, + }, + }, + changed: false, + }, + { + name: "Specs are different", + oldObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + }, + }, + newObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + }, + }, + changed: true, + }, + { + name: "Specs are same", + oldObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + Config: map[string]string{"key": "value"}, + Credential: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{ + Name: "secret", + }, + Key: "credential", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "bucket1", + Prefix: "prefix", + CACert: []byte{'a'}, + }, + }, + Default: true, + AccessMode: velerov1.BackupStorageLocationAccessModeReadWrite, + BackupSyncPeriod: &metav1.Duration{ + Duration: 1 * time.Minute, + }, + ValidationFrequency: &metav1.Duration{ + Duration: 1 * time.Minute, + }, + }, + }, + newObj: &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "azure", + Config: map[string]string{"key": "value"}, + Credential: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{ + Name: "secret", + }, + Key: "credential", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "bucket1", + Prefix: "prefix", + CACert: []byte{'a'}, + }, + }, + Default: true, + AccessMode: velerov1.BackupStorageLocationAccessModeReadWrite, + BackupSyncPeriod: &metav1.Duration{ + Duration: 1 * time.Minute, + }, + ValidationFrequency: &metav1.Duration{ + Duration: 1 * time.Minute, + }, + }, + }, + changed: false, + }, + } + + predicate := SpecChangePredicate{} + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + changed := predicate.Update(event.UpdateEvent{ + ObjectOld: c.oldObj, + ObjectNew: c.newObj, + }) + assert.Equal(t, c.changed, changed) + }) + } +}