Merge pull request #522 from qube-rt/fix/cn-clearresources-nil-cnspec fix(cn-controller): guard dereference while trying to clear resources
diff --git a/pkg/controller/sub_controller/be/controller_test.go b/pkg/controller/sub_controller/be/controller_test.go index 289d2f4..a5f0c47 100644 --- a/pkg/controller/sub_controller/be/controller_test.go +++ b/pkg/controller/sub_controller/be/controller_test.go
@@ -18,67 +18,40 @@ import ( "context" - "fmt" + "testing" + v1 "github.com/apache/doris-operator/api/doris/v1" "github.com/apache/doris-operator/pkg/common/utils/resource" appv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - k8sruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" - "path/filepath" - "runtime" - ctrl "sigs.k8s.io/controller-runtime" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" - "testing" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -var cfg *rest.Config -var testEnv *envtest.Environment -var mgr ctrl.Manager - -func init() { - testEnv = &envtest.Environment{ - Scheme: k8sruntime.NewScheme(), - BinaryAssetsDirectory: filepath.Join("..", "..", "..", "..", "bin", "k8s", - fmt.Sprintf("1.26.1-%s-%s", runtime.GOOS, runtime.GOARCH)), - CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: true, +func newBeController(t *testing.T, objects ...client.Object) (*Controller, client.Client) { + t.Helper() + scheme := runtime.NewScheme() + if err := appv1.AddToScheme(scheme); err != nil { + t.Fatalf("add appsv1 to scheme failed, err=%s", err.Error()) } - - var err error - cfg, err = testEnv.Start() - if err != nil { - fmt.Println("init failed, err=" + err.Error()) + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("add corev1 to scheme failed, err=%s", err.Error()) } - mgr, err = ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme}) - if err != nil { - fmt.Println("Test_safeScaleDown NewManager failed, err=" + err.Error()) - } - go func() { - err = mgr.Start(ctrl.SetupSignalHandler()) - }() + k8sclient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + return New(k8sclient, record.NewFakeRecorder(10)), k8sclient } func Test_ClearResources(t *testing.T) { - defer deferClear() - bc := New(mgr.GetClient(), mgr.GetEventRecorderFor("test-be")) cluster := &v1.DorisCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test", Namespace: "default", }, - //Spec: v1.DorisClusterSpec{ - // BeSpec: &v1.BeSpec{ - // BaseSpec: v1.BaseSpec{ - // Replicas: resource.GetInt32Pointer(3), - // }, - // }, - //}, Status: v1.DorisClusterStatus{ BEStatus: &v1.ComponentStatus{ ComponentCondition: v1.ComponentCondition{ @@ -159,26 +132,16 @@ }, }} - for _, dcrResource := range resources { - if err := mgr.GetClient().Create(context.Background(), dcrResource); err != nil { - t.Errorf("Test_ClearResources create resource name =%s, failed,err=%s", dcrResource.GetName(), err.Error()) - } - } + bc, k8sclient := newBeController(t, resources...) if _, err := bc.ClearResources(context.Background(), cluster); err != nil { t.Errorf("Test_ClearResources clear resources failed, err=%s", err.Error()) } - for i, _ := range resources { + for i := range resources { name := resources[i].GetName() - if err := mgr.GetClient().Get(context.Background(), types.NamespacedName{Namespace: "default", Name: name}, resources[i]); err != nil && !apierrors.IsNotFound(err) { - t.Errorf("Test_ClearResources the resource is not deleted, name=%s, err=%s", name, err.Error()) + err := k8sclient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: name}, resources[i]) + if !apierrors.IsNotFound(err) { + t.Errorf("Test_ClearResources the resource is not deleted, name=%s, err=%v", name, err) } } } - -func deferClear() { - err := testEnv.Stop() - if err != nil { - fmt.Println(err) - } -}
diff --git a/pkg/controller/sub_controller/cn/controller.go b/pkg/controller/sub_controller/cn/controller.go index 15cf355..5e15e3a 100644 --- a/pkg/controller/sub_controller/cn/controller.go +++ b/pkg/controller/sub_controller/cn/controller.go
@@ -19,6 +19,8 @@ import ( "context" + "time" + dorisv1 "github.com/apache/doris-operator/api/doris/v1" "github.com/apache/doris-operator/pkg/common/utils" "github.com/apache/doris-operator/pkg/common/utils/k8s" @@ -31,7 +33,6 @@ "k8s.io/client-go/tools/record" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" - "time" ) type Controller struct { @@ -223,7 +224,8 @@ } // clear autoscaler when autoscaler config deleted or the doriscluster deleted. - if dcr.Spec.CnSpec.AutoScalingPolicy == nil || !dcr.DeletionTimestamp.IsZero() { + // guard the dereference, only calls ClearResources when CnSpec is nil. + if dcr.Spec.CnSpec == nil || dcr.Spec.CnSpec.AutoScalingPolicy == nil || !dcr.DeletionTimestamp.IsZero() { if err := cn.DeleteAutoscaler(ctx, dcr); err != nil { cn.K8srecorder.Eventf(dcr, string(sub_controller.EventWarning), sub_controller.AutoScalerDeleteFailed, "cn autoscaler deleted failed."+err.Error()) }
diff --git a/pkg/controller/sub_controller/cn/controller_test.go b/pkg/controller/sub_controller/cn/controller_test.go new file mode 100644 index 0000000..d20fd11 --- /dev/null +++ b/pkg/controller/sub_controller/cn/controller_test.go
@@ -0,0 +1,94 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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 cn + +import ( + "context" + "testing" + + v1 "github.com/apache/doris-operator/api/doris/v1" + appv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func newCnController(t *testing.T, objects ...client.Object) *Controller { + t.Helper() + scheme := runtime.NewScheme() + if err := appv1.AddToScheme(scheme); err != nil { + t.Fatalf("add appsv1 to scheme failed, err=%s", err.Error()) + } + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("add corev1 to scheme failed, err=%s", err.Error()) + } + return New(fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build(), record.NewFakeRecorder(10)) +} + +// Sync only calls ClearResources when CnSpec is nil, so a cluster whose cnSpec was removed while +// status.cnStatus is still set must not dereference the spec. +func Test_ClearResources(t *testing.T) { + cc := newCnController(t) + cluster := &v1.DorisCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: v1.DorisClusterSpec{ + CnSpec: nil, + }, + Status: v1.DorisClusterStatus{ + CnStatus: &v1.CnStatus{ + ComponentStatus: v1.ComponentStatus{ + ComponentCondition: v1.ComponentCondition{ + Phase: v1.Reconciling, + }, + }, + }, + }, + } + + cleared, err := cc.ClearResources(context.Background(), cluster) + if err != nil { + t.Errorf("Test_ClearResources clear resources failed, err=%s", err.Error()) + } + if !cleared { + t.Error("Test_ClearResources expected resources to be cleared") + } +} + +// A cluster that never had cn returns early on the nil status, before reaching the spec. +func Test_ClearResources_NoCnStatus(t *testing.T) { + cc := newCnController(t) + cluster := &v1.DorisCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-no-cn", + Namespace: "default", + }, + } + + cleared, err := cc.ClearResources(context.Background(), cluster) + if err != nil { + t.Errorf("Test_ClearResources_NoCnStatus failed, err=%s", err.Error()) + } + if !cleared { + t.Error("Test_ClearResources_NoCnStatus expected resources to be cleared") + } +}
diff --git a/pkg/controller/sub_controller/fe/controller_test.go b/pkg/controller/sub_controller/fe/controller_test.go index 99af3dc..b6cdf5a 100644 --- a/pkg/controller/sub_controller/fe/controller_test.go +++ b/pkg/controller/sub_controller/fe/controller_test.go
@@ -18,129 +18,60 @@ import ( "context" - "fmt" + "testing" + v1 "github.com/apache/doris-operator/api/doris/v1" "github.com/apache/doris-operator/pkg/common/utils/resource" + appv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" kr "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - k8sruntime "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" - "path/filepath" - "runtime" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/envtest" - "testing" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -var cfg *rest.Config -var testEnv *envtest.Environment -var mgr ctrl.Manager - -func init() { - testEnv = &envtest.Environment{ - Scheme: k8sruntime.NewScheme(), - BinaryAssetsDirectory: filepath.Join("..", "..", "..", "..", "bin", "k8s", - fmt.Sprintf("1.26.1-%s-%s", runtime.GOOS, runtime.GOARCH)), - CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: true, +func newFeController(t *testing.T, objects ...client.Object) (*Controller, client.Client) { + t.Helper() + scheme := runtime.NewScheme() + if err := appv1.AddToScheme(scheme); err != nil { + t.Fatalf("add appsv1 to scheme failed, err=%s", err.Error()) } - - var err error - cfg, err = testEnv.Start() - if err != nil { - fmt.Println("init failed, err=" + err.Error()) + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("add corev1 to scheme failed, err=%s", err.Error()) } - mgr, err = ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme}) - if err != nil { - fmt.Println("Test_safeScaleDown NewManager failed, err=" + err.Error()) - } - go func() { - err = mgr.Start(ctrl.SetupSignalHandler()) - }() + k8sclient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + return New(k8sclient, record.NewFakeRecorder(10)), k8sclient } +// buildFePVC builds a pvc as the fe statefulset would name and label it. +func buildFePVC(name string) *corev1.PersistentVolumeClaim { + return &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + Labels: map[string]string{ + v1.OwnerReference: "test-fe", + v1.ComponentLabelKey: "fe", + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{"ReadWriteOnce"}, + Resources: corev1.VolumeResourceRequirements{ + Requests: map[corev1.ResourceName]kr.Quantity{ + "storage": kr.MustParse("500Gi"), + }, + }, + }, + } +} + +// Scaling fe down leaves pvcs above the new replica count behind, so ClearResources recycles them. func Test_ClearResources(t *testing.T) { - defer deferClear() - - pvcs := []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-fe-0", - Namespace: "default", - Labels: map[string]string{ - v1.OwnerReference: "test-fe", - v1.ComponentLabelKey: "fe", - }, - }, - Spec: corev1.PersistentVolumeClaimSpec{ - AccessModes: []corev1.PersistentVolumeAccessMode{"ReadWriteOnce"}, - Resources: corev1.VolumeResourceRequirements{ - Requests: map[corev1.ResourceName]kr.Quantity{ - "storage": kr.MustParse("500Gi"), - }, - }, - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - v1.OwnerReference: "test-fe", - v1.ComponentLabelKey: "fe", - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-fe-1", - Namespace: "default", - Labels: map[string]string{ - v1.OwnerReference: "test-fe", - v1.ComponentLabelKey: "fe", - }, - }, - Spec: corev1.PersistentVolumeClaimSpec{ - AccessModes: []corev1.PersistentVolumeAccessMode{"ReadWriteOnce"}, - Resources: corev1.VolumeResourceRequirements{ - Requests: map[corev1.ResourceName]kr.Quantity{ - "storage": kr.MustParse("500Gi"), - }, - }, - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - v1.OwnerReference: "test-fe", - v1.ComponentLabelKey: "fe", - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-fe-2", - Namespace: "default", - Labels: map[string]string{ - v1.OwnerReference: "test-fe", - v1.ComponentLabelKey: "fe", - }, - }, - Spec: corev1.PersistentVolumeClaimSpec{ - AccessModes: []corev1.PersistentVolumeAccessMode{"ReadWriteOnce"}, - Resources: corev1.VolumeResourceRequirements{ - Requests: map[corev1.ResourceName]kr.Quantity{ - "storage": kr.MustParse("500Gi"), - }, - }, - Selector: &metav1.LabelSelector{}, - }, - }, - } - - for i, _ := range pvcs { - if err := mgr.GetClient().Create(context.Background(), &pvcs[i]); err != nil { - t.Errorf("create pvc failed,err=%s", err.Error()) - } - } - - fc := New(mgr.GetClient(), mgr.GetEventRecorderFor("test-fe")) + fc, k8sclient := newFeController(t, buildFePVC("test-fe-0"), buildFePVC("test-fe-1"), buildFePVC("test-fe-2")) cluster := &v1.DorisCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test", @@ -149,7 +80,7 @@ Spec: v1.DorisClusterSpec{ FeSpec: &v1.FeSpec{ BaseSpec: v1.BaseSpec{ - Replicas: resource.GetInt32Pointer(3), + Replicas: resource.GetInt32Pointer(2), PersistentVolumes: []v1.PersistentVolume{ { MountPath: "/opt/apache-doris/fe/doris-meta", @@ -174,12 +105,22 @@ }, }, } - fc.ClearResources(context.Background(), cluster) -} -func deferClear() { - err := testEnv.Stop() + cleared, err := fc.ClearResources(context.Background(), cluster) if err != nil { - fmt.Println(err) + t.Errorf("Test_ClearResources clear resources failed, err=%s", err.Error()) + } + if !cleared { + t.Error("Test_ClearResources expected resources to be cleared") + } + + for _, name := range []string{"test-fe-0", "test-fe-1"} { + if err := k8sclient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: name}, &corev1.PersistentVolumeClaim{}); err != nil { + t.Errorf("Test_ClearResources the pvc %s should be kept, err=%s", name, err.Error()) + } + } + err = k8sclient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "test-fe-2"}, &corev1.PersistentVolumeClaim{}) + if !apierrors.IsNotFound(err) { + t.Errorf("Test_ClearResources the pvc test-fe-2 is not deleted, err=%v", err) } }
diff --git a/pkg/controller/sub_controller/fe/prepare_modify_test.go b/pkg/controller/sub_controller/fe/prepare_modify_test.go index 3a63023..8b7ed18 100644 --- a/pkg/controller/sub_controller/fe/prepare_modify_test.go +++ b/pkg/controller/sub_controller/fe/prepare_modify_test.go
@@ -17,22 +17,15 @@ package fe import ( + "testing" + dorisv1 "github.com/apache/doris-operator/api/doris/v1" "github.com/apache/doris-operator/pkg/common/utils/resource" appv1 "k8s.io/api/apps/v1" - "k8s.io/client-go/kubernetes/scheme" - ctrl "sigs.k8s.io/controller-runtime" - "testing" ) func Test_safeScaleDown(t *testing.T) { - - mgr, err := ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme}) - if err != nil { - t.Errorf("Test_safeScaleDown NewManager failed, err=%s", err.Error()) - } - - fc := New(mgr.GetClient(), mgr.GetEventRecorderFor("test-fe")) + fc, _ := newFeController(t) dcrs := []*dorisv1.DorisCluster{ {