fix(reconciler): fix re-reconciler with 0 seconds not ready

pkg/reconciler/cluster.go

@@ -74,32 +74,33 @@ func (r *BaseCluster[T]) Paused(ctx context.Context) bool {
  return false
 }
 
-func (r *BaseCluster[T]) Ready(ctx context.Context) *Result {
+func (r *BaseCluster[T]) Ready(ctx context.Context) (ctrl.Result, error) {
  if r.Paused(ctx) {
  logger.Info("Reconciliation paused, skip ready check", "cluster", r.GetName(), "namespace", r.GetNamespace())
- return NewResult(true, 0, nil)
+ return ctrl.Result{}, nil
  }
  for _, resource := range r.resources {
- if result := resource.Ready(ctx); result.RequeueOrNot() {
- return result
+ logger.Info("Checking resource ready", "cluster", r.GetName(), "namespace", r.GetNamespace(), "resource", resource.GetName())
+ if result, err := resource.Ready(ctx); !result.IsZero() || err != nil {
+ return result, err
  }
+ logger.Info("Resource is ready", "cluster", r.GetName(), "namespace", r.GetNamespace(), "resource", resource.GetName())
  }
- return NewResult(false, 0, nil)
+ return ctrl.Result{}, nil
 }
 
-func (r *BaseCluster[T]) Reconcile(ctx context.Context) *Result {
+func (r *BaseCluster[T]) Reconcile(ctx context.Context) (ctrl.Result, error) {
  if r.Paused(ctx) {
  logger.Info("Reconciliation paused, skip reconcile", "cluster", r.GetName(), "namespace", r.GetNamespace())
- return NewResult(true, 0, nil)
+ return ctrl.Result{}, nil
  }
 
  for _, resource := range r.resources {
  logger.Info("Reconciling resource", "cluster", r.GetName(), "namespace", r.GetNamespace(), "resource", resource.GetName())
- result := resource.Reconcile(ctx)
- if result.RequeueOrNot() {
- return result
+ if result, err := resource.Reconcile(ctx); !result.IsZero() || err != nil {
+ return result, err
  }
- logger.Info("Reconcile completed", "cluster", r.GetName(), "namespace", r.GetNamespace())
+ logger.Info("Reconciled resource", "cluster", r.GetName(), "namespace", r.GetNamespace(), "resource", resource.GetName())
  }
- return NewResult(false, 0, nil)
+ return ctrl.Result{}, nil
 }

pkg/reconciler/cluster_test.go

@@ -161,9 +161,9 @@ var _ = Describe("Cluster reconciler", func() {
 
  By("Reconcile")
  Eventually(func() bool {
- result := clusterReconciler.Reconcile(ctx)
- return result.RequeueOrNot()
- }, time.Second*15, time.Microsecond*100).Should(BeFalse())
+ result, err := clusterReconciler.Reconcile(ctx)
+ return result.IsZero() && err == nil
+ }, time.Second*15, time.Microsecond*100).Should(BeTrue())
 
  By("Checking the service resource of cluster level")
  service := &corev1.Service{}

pkg/reconciler/deployment.go

@@ -3,9 +3,11 @@ package reconciler
 import (
  "context"
 
+ appv1 "k8s.io/api/apps/v1"
+ ctrl "sigs.k8s.io/controller-runtime"
+
  "github.com/zncdatadev/operator-go/pkg/builder"
  "github.com/zncdatadev/operator-go/pkg/client"
- appv1 "k8s.io/api/apps/v1"
 )
 
 var _ ResourceReconciler[builder.DeploymentBuilder] = &Deployment{}
@@ -18,7 +20,7 @@ type Deployment struct {
  Stopped bool
 }
 
-func (r *Deployment) Reconcile(ctx context.Context) *Result {
+func (r *Deployment) Reconcile(ctx context.Context) (ctrl.Result, error) {
  // TODO: Extract a doBuild method to invoke the implementation side's Build method and append some framework logic.
  // Consider abstracting a WorkloadReconciler on top of DeploymentReconciler to extract some of the logic into it.
  resourceBuilder := r.GetBuilder()
@@ -30,26 +32,26 @@ func (r *Deployment) Reconcile(ctx context.Context) *Result {
  resource, err := resourceBuilder.Build(ctx)
 
  if err != nil {
- return NewResult(true, 0, err)
+ return ctrl.Result{}, err
  }
  return r.ResourceReconcile(ctx, resource)
 }
 
-func (r *Deployment) Ready(ctx context.Context) *Result {
+func (r *Deployment) Ready(ctx context.Context) (ctrl.Result, error) {
 
  obj := appv1.Deployment{
  ObjectMeta: r.GetObjectMeta(),
  }
  logger.V(1).Info("Checking deployment ready", "namespace", obj.Namespace, "name", obj.Name)
  if err := r.Client.Get(ctx, &obj); err != nil {
- return NewResult(true, 0, err)
+ return ctrl.Result{}, err
  }
  if obj.Status.ReadyReplicas == *obj.Spec.Replicas {
  logger.Info("Deployment is ready", "namespace", obj.Namespace, "name", obj.Name, "replicas", *obj.Spec.Replicas, "readyReplicas", obj.Status.ReadyReplicas)
- return NewResult(false, 0, nil)
+ return ctrl.Result{}, nil
  }
  logger.Info("Deployment is not ready", "namespace", obj.Namespace, "name", obj.Name, "replicas", *obj.Spec.Replicas, "readyReplicas", obj.Status.ReadyReplicas)
- return NewResult(false, 5, nil)
+ return ctrl.Result{Requeue: true}, nil
 }
 
 func NewDeployment(

pkg/reconciler/deployment_test.go

@@ -90,21 +90,25 @@ var _ = Describe("Deloyment reconciler", func() {
  Expect(deploymentReconciler).ShouldNot(BeNil())
 
  By("reconcile the deployment")
- result := deploymentReconciler.Reconcile(ctx)
+ result, err := deploymentReconciler.Reconcile(ctx)
  Expect(result).ShouldNot(BeNil())
- Expect(result.Error).Should(BeNil())
- Expect(result.RequeueOrNot()).Should(BeTrue())
+ // Expect(result.Error).Should(BeNil())
+ // Expect(result.RequeueOrNot()).Should(BeTrue())
+ Expect(err).Should(BeNil())
+ Expect(result.IsZero()).Should(BeFalse())
+ Expect(result.Requeue).Should(BeTrue())
 
  By("Checking the deployment spec.replicas is valid")
  deployment := &appv1.Deployment{}
  Expect(k8sClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: name}, deployment)).Should(Succeed())
  Expect(deployment.Spec.Replicas).Should(Equal(&replcias))
 
- By("check the deployment is ready or not")
- result = deploymentReconciler.Ready(ctx)
+ By("check the deployment is not ready")
+ result, err = deploymentReconciler.Ready(ctx)
  Expect(result).ShouldNot(BeNil())
- Expect(result.Error).Should(BeNil())
- Expect(result.RequeueOrNot()).Should(BeTrue())
+ Expect(err).Should(BeNil())
+ Expect(result.IsZero()).Should(BeFalse())
+ Expect(result.Requeue).Should(BeTrue())
 
  // Because the envtest does not handle the pod, we need to mock that the statefulset is ready
  // Mock that the deployment is ready by updating the ready replicas to 3
@@ -115,11 +119,11 @@ var _ = Describe("Deloyment reconciler", func() {
  deployment.Status.ReadyReplicas = replcias
  Expect(k8sClient.Status().Update(ctx, deployment)).Should(Succeed())
 
- By("check the deployment is ready or not")
- result = deploymentReconciler.Ready(ctx)
+ By("check the deployment is ready")
+ result, err = deploymentReconciler.Ready(ctx)
  Expect(result).ShouldNot(BeNil())
- Expect(result.Error).Should(BeNil())
- Expect(result.RequeueOrNot()).Should(BeFalse())
+ Expect(err).Should(BeNil())
+ Expect(result.IsZero()).Should(BeTrue())
 
  By("check the container image pull policy of deployment is default value")
  deployment = &appv1.Deployment{}
@@ -129,17 +133,18 @@ var _ = Describe("Deloyment reconciler", func() {
  Expect(deployment.Spec.Template.Spec.Containers[0].ImagePullPolicy).Should(Equal(*builder.DefaultImagePullPolicy))
  })
 
- It("Should successfully reconcile a stopped whoami deployment", func() {
+ It("Should successfully reconcile deployment replicas to 0 when stopped", func() {
 
- By("Create a stopped deployment reconciler")
+ By("Create a stopped deployment reconciler normal")
  deploymentReconciler := reconciler.NewDeployment(resourceClient, name, deploymentBuilder, false)
  Expect(deploymentReconciler).ShouldNot(BeNil())
 
  By("reconcile the deployment")
- result := deploymentReconciler.Reconcile(ctx)
+ result, err := deploymentReconciler.Reconcile(ctx)
  Expect(result).ShouldNot(BeNil())
- Expect(result.Error).Should(BeNil())
- Expect(result.RequeueOrNot()).Should(BeTrue())
+ Expect(err).Should(BeNil())
+ Expect(result.IsZero()).Should(BeFalse())
+ Expect(result.Requeue).Should(BeTrue())
 
  By("checking the deployment spec replicas is valid")
  deployment := &appv1.Deployment{}
@@ -156,17 +161,18 @@ var _ = Describe("Deloyment reconciler", func() {
  builder.WorkloadOptions{},
  ),
  }
- By("create a stopped deployment reconciler")
+ By("create deployment reconciler with stopped is true")
  deploymentReconciler = reconciler.NewDeployment(resourceClient, name, deploymentBuilder, true)
  Expect(deploymentReconciler).ShouldNot(BeNil())
 
  By("reconcile the deployment")
- result = deploymentReconciler.Reconcile(ctx)
+ result, err = deploymentReconciler.Reconcile(ctx)
  Expect(result).ShouldNot(BeNil())
- Expect(result.Error).Should(BeNil())
- Expect(result.RequeueOrNot()).Should(BeTrue())
+ Expect(err).Should(BeNil())
+ Expect(result.IsZero()).Should(BeFalse())
+ Expect(result.Requeue).Should(BeTrue())
 
- By("checking the deployment spec replicas is updated")
+ By("checking the deployment spec replicas is updated to 0")
  deployment = &appv1.Deployment{}
  Expect(k8sClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: name}, deployment)).Should(Succeed())
  Expect(*deployment.Spec.Replicas).Should(BeEquivalentTo(int32(0)))

pkg/reconciler/reconciler.go

@@ -3,6 +3,8 @@ package reconciler
 import (
  "context"
 
+ ctrl "sigs.k8s.io/controller-runtime"
+
  "github.com/zncdatadev/operator-go/pkg/client"
 )
 
@@ -12,8 +14,8 @@ type Reconciler interface {
  GetName() string
  GetNamespace() string
  GetClient() *client.Client
- Reconcile(ctx context.Context) *Result
- Ready(ctx context.Context) *Result
+ Reconcile(ctx context.Context) (ctrl.Result, error)
+ Ready(ctx context.Context) (ctrl.Result, error)
 }
 
 var _ Reconciler = &BaseReconciler[AnySpec]{}
@@ -37,11 +39,11 @@ func (b *BaseReconciler[T]) GetNamespace() string {
  return b.Client.GetOwnerNamespace()
 }
 
-func (b *BaseReconciler[T]) Ready(ctx context.Context) *Result {
+func (b *BaseReconciler[T]) Ready(ctx context.Context) (ctrl.Result, error) {
  panic("unimplemented")
 }
 
-func (b *BaseReconciler[T]) Reconcile(ctx context.Context) *Result {
+func (b *BaseReconciler[T]) Reconcile(ctx context.Context) (ctrl.Result, error) {
  panic("unimplemented")
 }
 

pkg/reconciler/resource.go

@@ -2,13 +2,13 @@ package reconciler
 
 import (
  "context"
- "time"
 
- "github.com/zncdatadev/operator-go/pkg/builder"
- "github.com/zncdatadev/operator-go/pkg/client"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  ctrl "sigs.k8s.io/controller-runtime"
  ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+
+ "github.com/zncdatadev/operator-go/pkg/builder"
+ "github.com/zncdatadev/operator-go/pkg/client"
 )
 
 var (
@@ -20,7 +20,7 @@ type ResourceReconciler[B builder.ResourceBuilder] interface {
 
  GetObjectMeta() metav1.ObjectMeta
  GetBuilder() B
- ResourceReconcile(ctx context.Context, resource ctrlclient.Object) *Result
+ ResourceReconcile(ctx context.Context, resource ctrlclient.Object) (ctrl.Result, error)
 }
 
 var _ ResourceReconciler[builder.ResourceBuilder] = &GenericResourceReconciler[builder.ResourceBuilder]{}
@@ -63,7 +63,7 @@ func (r *GenericResourceReconciler[B]) GetBuilder() B {
 // If the resource is created or updated, it returns a Result with a requeue time of 1 second.
 //
 // Most of the time you should not call this method directly, but call the r.Reconcile() method instead.
-func (r *GenericResourceReconciler[B]) ResourceReconcile(ctx context.Context, resource ctrlclient.Object) *Result {
+func (r *GenericResourceReconciler[B]) ResourceReconcile(ctx context.Context, resource ctrlclient.Object) (ctrl.Result, error) {
  logExtraValues := []interface{}{
  "name", resource.GetName(),
  "namespace", resource.GetNamespace(),
@@ -72,29 +72,26 @@ func (r *GenericResourceReconciler[B]) ResourceReconcile(ctx context.Context, re
 
  if mutation, err := r.Client.CreateOrUpdate(ctx, resource); err != nil {
  resourceLogger.Error(err, "Failed to create or update resource", logExtraValues...)
- return NewResult(true, 0, err)
+ return ctrl.Result{}, err
  } else if mutation {
  resourceLogger.Info("Resource created or updated", logExtraValues...)
- // TODO: Different resources may have different retry times based on their characteristics,
- // for example: the creation time of a Deployment may be longer, so a longer retry time can be set,
- // while the creation time of a Service may be shorter, so a shorter retry time can be set.
- return NewResult(true, time.Second, nil)
+ return ctrl.Result{Requeue: true}, nil
  }
- return NewResult(false, 0, nil)
+ return ctrl.Result{}, nil
 }
 
-func (r *GenericResourceReconciler[B]) Reconcile(ctx context.Context) *Result {
+func (r *GenericResourceReconciler[B]) Reconcile(ctx context.Context) (ctrl.Result, error) {
  resource, err := r.GetBuilder().Build(ctx)
 
  if err != nil {
- return NewResult(true, 0, err)
+ return ctrl.Result{}, err
  }
  return r.ResourceReconcile(ctx, resource)
 }
 
 // GenericResourceReconciler[B] does not check anythins, so it is always ready.
-func (r *GenericResourceReconciler[B]) Ready(ctx context.Context) *Result {
- return NewResult(false, 0, nil)
+func (r *GenericResourceReconciler[B]) Ready(ctx context.Context) (ctrl.Result, error) {
+ return ctrl.Result{}, nil
 }
 
 type SimpleResourceReconciler[B builder.ResourceBuilder] struct {

pkg/reconciler/role_test.go

@@ -236,11 +236,11 @@ var _ = Describe("Role reconciler", func() {
  By("registering resources")
  Expect(roleReconciler.RegisterResources(ctx)).To(Succeed())
 
- By("reconciling resources")
+ By("reconciling resources until ready")
  Eventually(func() bool {
- result := roleReconciler.Reconcile(ctx)
- return result.RequeueOrNot()
- }, time.Second*3, time.Second*1).Should(BeFalse())
+ result, err := roleReconciler.Reconcile(ctx)
+ return result.IsZero() && err == nil
+ }, time.Second*3, time.Second*1).Should(BeTrue())
 
  By("mock deployment is ready")
  deployment := &appv1.Deployment{}
@@ -249,11 +249,11 @@ var _ = Describe("Role reconciler", func() {
  deployment.Status.ReadyReplicas = 1
  Expect(k8sClient.Status().Update(ctx, deployment)).Should(Succeed())
 
- By("check resource ready")
+ By("check resource until ready")
  Eventually(func() bool {
- result := roleReconciler.Ready(ctx)
- return result.RequeueOrNot()
- }, time.Second*3, time.Second*1).Should(BeFalse())
+ result, err := roleReconciler.Ready(ctx)
+ return result.IsZero() && err == nil
+ }, time.Second*3, time.Second*1).Should(BeTrue())
  })
  })