Utilize the new errors package to encapsulate reconcile requeue/error…

… handling (#172) As part of #168 we added a new error handling package. The function IsReconcileAborted verifies if the reconcile needs a re-queue, needs to be re-queued after a delay or returned an error. This function can be utilized at other points of reconcilers where similar check happens.
vertica · Mar 11, 2022 · d9e4c2d · d9e4c2d
1 parent 320f8c7
commit d9e4c2d
Show file tree

Hide file tree

Showing 12 changed files with 62 additions and 35 deletions.
diff --git a/kuttl-test.yaml b/kuttl-test.yaml
@@ -20,8 +20,6 @@ testDirs:
   - tests/e2e-11.1
 parallel: 2
 timeout: 600
-suppress:
-  - events
 commands:
   # Create the kustomize overlay files to override the image, endpoint, etc.
   - command: scripts/setup-kustomize.sh

diff --git a/pkg/controllers/actor.go b/pkg/controllers/actor.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -68,7 +69,7 @@ func scaledownSubcluster(ctx context.Context, act ScaledownActor, sc *vapi.Subcl
 		if err != nil {
 			return res, fmt.Errorf("failed to scale down nodes in subcluster %s: %w", sc.Name, err)
 		}
-		if res.Requeue {
+		if verrors.IsReconcileAborted(res, err) {
 			return res, nil
 		}
 	}

diff --git a/pkg/controllers/dbaddnode_reconcile.go b/pkg/controllers/dbaddnode_reconcile.go
@@ -23,6 +23,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	corev1 "k8s.io/api/core/v1"
@@ -63,7 +64,7 @@ func (d *DBAddNodeReconciler) Reconcile(ctx context.Context, req *ctrl.Request)
 	}
 
 	for i := range d.Vdb.Spec.Subclusters {
-		if res, err := d.reconcileSubcluster(ctx, &d.Vdb.Spec.Subclusters[i]); err != nil || res.Requeue {
+		if res, err := d.reconcileSubcluster(ctx, &d.Vdb.Spec.Subclusters[i]); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}

diff --git a/pkg/controllers/dbaddsubcluster_reconcile.go b/pkg/controllers/dbaddsubcluster_reconcile.go
@@ -22,6 +22,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	"github.com/vertica/vertica-kubernetes/pkg/version"
@@ -72,7 +73,7 @@ func (d *DBAddSubclusterReconciler) addMissingSubclusters(ctx context.Context, s
 	d.ATPod = atPod
 
 	subclusters, res, err := d.fetchSubclusters(ctx)
-	if err != nil || res.Requeue {
+	if verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 

diff --git a/pkg/controllers/dbremovenode_reconcile.go b/pkg/controllers/dbremovenode_reconcile.go
@@ -24,6 +24,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/iter"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
@@ -88,7 +89,7 @@ func (d *DBRemoveNodeReconciler) Reconcile(ctx context.Context, req *ctrl.Reques
 	}
 
 	for i := range subclusters {
-		if res, err := d.reconcileSubcluster(ctx, subclusters[i]); err != nil || res.Requeue {
+		if res, err := d.reconcileSubcluster(ctx, subclusters[i]); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}

diff --git a/pkg/controllers/init_db.go b/pkg/controllers/init_db.go
@@ -26,6 +26,7 @@ import (
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cloud"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	"github.com/vertica/vertica-kubernetes/pkg/paths"
@@ -65,7 +66,7 @@ func (g *GenericDatabaseInitializer) checkAndRunInit(ctx context.Context) (ctrl.
 
 	if exists := g.PFacts.doesDBExist(); exists.IsFalse() {
 		res, err := g.runInit(ctx)
-		if err != nil || res.Requeue {
+		if verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	} else if exists.IsNone() {
@@ -86,7 +87,7 @@ func (g *GenericDatabaseInitializer) runInit(ctx context.Context) (ctrl.Result,
 	}
 	atPod := atPodFact.name
 
-	if res, err := g.ConstructAuthParms(ctx, atPod); err != nil || res.Requeue {
+	if res, err := g.ConstructAuthParms(ctx, atPod); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 	if err := g.initializer.preCmdSetup(ctx, atPod); err != nil {
@@ -119,7 +120,7 @@ func (g *GenericDatabaseInitializer) runInit(ctx context.Context) (ctrl.Result,
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	if res, err := g.initializer.execCmd(ctx, atPod, cmd); err != nil || res.Requeue {
+	if res, err := g.initializer.execCmd(ctx, atPod, cmd); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
@@ -196,12 +197,12 @@ func (g *GenericDatabaseInitializer) ConstructAuthParms(ctx context.Context, atP
 	}
 
 	content, res, err := contentGen(ctx)
-	if res.Requeue || err != nil {
+	if verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
 	if g.Vdb.HasKerberosConfig() {
-		if res = g.hasCompatibleVersionForKerberos(); res.Requeue {
+		if res = g.hasCompatibleVersionForKerberos(); verrors.IsReconcileAborted(res, nil) {
 			return res, nil
 		}
 		content = dedent.Dedent(fmt.Sprintf(`
@@ -226,7 +227,7 @@ func (g *GenericDatabaseInitializer) DestroyAuthParms(ctx context.Context, atPod
 func (g *GenericDatabaseInitializer) getS3AuthParmsContent(ctx context.Context) (string, ctrl.Result, error) {
 	// Extract the auth from the credential secret.
 	auth, res, err := g.getCommunalAuth(ctx)
-	if err != nil || res.Requeue {
+	if verrors.IsReconcileAborted(res, err) {
 		return "", res, err
 	}
 
@@ -272,7 +273,7 @@ func (g *GenericDatabaseInitializer) getKerberosAuthParmsContent() string {
 func (g *GenericDatabaseInitializer) getGCloudAuthParmsContent(ctx context.Context) (string, ctrl.Result, error) {
 	// Extract the auth from the credential secret.
 	auth, res, err := g.getCommunalAuth(ctx)
-	if err != nil || res.Requeue {
+	if verrors.IsReconcileAborted(res, err) {
 		return "", res, err
 	}
 
@@ -290,7 +291,7 @@ func (g *GenericDatabaseInitializer) getGCloudAuthParmsContent(ctx context.Conte
 // Azure Blob Storage
 func (g *GenericDatabaseInitializer) getAzureAuthParmsContent(ctx context.Context) (string, ctrl.Result, error) {
 	azureCreds, azureConfig, res, err := g.getAzureAuth(ctx)
-	if err != nil || res.Requeue {
+	if verrors.IsReconcileAborted(res, err) {
 		return "", res, err
 	}
 
@@ -357,7 +358,7 @@ func (g *GenericDatabaseInitializer) copyAuthFile(ctx context.Context, atPod typ
 // Value is returned in the format: <accessKey>:<secretKey>
 func (g *GenericDatabaseInitializer) getCommunalAuth(ctx context.Context) (string, ctrl.Result, error) {
 	secret, res, err := g.getCommunalCredsSecret(ctx)
-	if res.Requeue || err != nil {
+	if verrors.IsReconcileAborted(res, err) {
 		return "", res, err
 	}
 
@@ -384,7 +385,7 @@ func (g *GenericDatabaseInitializer) getCommunalAuth(ctx context.Context) (strin
 func (g *GenericDatabaseInitializer) getAzureAuth(ctx context.Context) (
 	cloud.AzureCredential, cloud.AzureEndpointConfig, ctrl.Result, error) {
 	secret, res, err := g.getCommunalCredsSecret(ctx)
-	if res.Requeue || err != nil {
+	if verrors.IsReconcileAborted(res, err) {
 		return cloud.AzureCredential{}, cloud.AzureEndpointConfig{}, res, err
 	}
 

diff --git a/pkg/controllers/obj_reconcile.go b/pkg/controllers/obj_reconcile.go
@@ -25,6 +25,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/builder"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/iter"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
@@ -66,7 +67,7 @@ func MakeObjReconciler(vdbrecon *VerticaDBReconciler, log logr.Logger, vdb *vapi
 func (o *ObjReconciler) Reconcile(ctx context.Context, req *ctrl.Request) (ctrl.Result, error) {
 	// Ensure any secrets/configMaps that we mount exist with the correct keys.
 	// We catch the errors here so that we can provide timely events.
-	if res, err := o.checkMountedObjs(ctx); res.Requeue || err != nil {
+	if res, err := o.checkMountedObjs(ctx); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
@@ -78,13 +79,13 @@ func (o *ObjReconciler) Reconcile(ctx context.Context, req *ctrl.Request) (ctrl.
 	// Check the objects for subclusters that should exist.  This will create
 	// missing objects and update existing objects to match the vdb.
 	for i := range o.Vdb.Spec.Subclusters {
-		if res, err := o.checkForCreatedSubcluster(ctx, &o.Vdb.Spec.Subclusters[i]); res.Requeue || err != nil {
+		if res, err := o.checkForCreatedSubcluster(ctx, &o.Vdb.Spec.Subclusters[i]); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
 
 	// Check to see if we need to remove any objects for deleted subclusters
-	if res, err := o.checkForDeletedSubcluster(ctx); res.Requeue || err != nil {
+	if res, err := o.checkForDeletedSubcluster(ctx); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
@@ -99,15 +100,15 @@ func (o *ObjReconciler) checkMountedObjs(ctx context.Context) (ctrl.Result, erro
 	if o.Vdb.Spec.LicenseSecret != "" {
 		_, res, err := getSecret(ctx, o.VRec, o.Vdb,
 			names.GenNamespacedName(o.Vdb, o.Vdb.Spec.LicenseSecret))
-		if res.Requeue || err != nil {
+		if verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
 
 	if o.Vdb.Spec.Communal.HadoopConfig != "" {
 		_, res, err := getConfigMap(ctx, o.VRec, o.Vdb,
 			names.GenNamespacedName(o.Vdb, o.Vdb.Spec.Communal.HadoopConfig))
-		if res.Requeue || err != nil {
+		if verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
@@ -116,13 +117,13 @@ func (o *ObjReconciler) checkMountedObjs(ctx context.Context) (ctrl.Result, erro
 
 	if o.Vdb.Spec.KerberosSecret != "" {
 		keyNames := []string{filepath.Base(paths.Krb5Conf), filepath.Base(paths.Krb5Keytab)}
-		if res, err := o.checkSecretHasKeys(ctx, "Kerberos", o.Vdb.Spec.KerberosSecret, keyNames); res.Requeue || err != nil {
+		if res, err := o.checkSecretHasKeys(ctx, "Kerberos", o.Vdb.Spec.KerberosSecret, keyNames); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
 
 	if o.Vdb.Spec.SSHSecret != "" {
-		if res, err := o.checkSecretHasKeys(ctx, "SSH", o.Vdb.Spec.SSHSecret, paths.SSHKeyPaths); res.Requeue || err != nil {
+		if res, err := o.checkSecretHasKeys(ctx, "SSH", o.Vdb.Spec.SSHSecret, paths.SSHKeyPaths); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
@@ -133,7 +134,7 @@ func (o *ObjReconciler) checkMountedObjs(ctx context.Context) (ctrl.Result, erro
 // checkSecretHasKeys is a helper to check that a secret has a set of keys in it
 func (o *ObjReconciler) checkSecretHasKeys(ctx context.Context, secretType, secretName string, keyNames []string) (ctrl.Result, error) {
 	secret, res, err := getSecret(ctx, o.VRec, o.Vdb, names.GenNamespacedName(o.Vdb, secretName))
-	if res.Requeue || err != nil {
+	if verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
@@ -178,7 +179,7 @@ func (o *ObjReconciler) checkForDeletedSubcluster(ctx context.Context) (ctrl.Res
 		// all pods in the subcluster.  If that isn't the case, we requeue to
 		// give those reconcilers a chance to do those actions.  Failure to do
 		// this will result in corruption of admintools.conf.
-		if r, e := o.checkForOrphanAdmintoolsConfEntries(0, &stss.Items[i]); r.Requeue || e != nil {
+		if r, e := o.checkForOrphanAdmintoolsConfEntries(0, &stss.Items[i]); verrors.IsReconcileAborted(r, e) {
 			return r, e
 		}
 
@@ -299,7 +300,7 @@ func (o *ObjReconciler) reconcileSts(ctx context.Context, sc *vapi.Subcluster) (
 	// and done the uninstall.  If we haven't yet done that we will requeue the
 	// reconciliation.  This will cause us to go through the remove node and
 	// uninstall reconcile actors to properly handle the scale down.
-	if r, e := o.checkForOrphanAdmintoolsConfEntries(sc.Size, curSts); r.Requeue || e != nil {
+	if r, e := o.checkForOrphanAdmintoolsConfEntries(sc.Size, curSts); verrors.IsReconcileAborted(r, e) {
 		return r, e
 	}
 

diff --git a/pkg/controllers/onlineupgrade_reconcile_test.go b/pkg/controllers/onlineupgrade_reconcile_test.go
@@ -368,7 +368,7 @@ var _ = Describe("onlineupgrade_reconcile", func() {
 		vdb.Spec.Image = OldImage
 		vdb.Spec.UpgradePolicy = vapi.OnlineUpgrade
 		vdb.Spec.TemporarySubclusterRouting.Names = []string{vdb.Spec.Subclusters[0].Name}
-		vdb.Spec.UpgradeRequeueTime = 100
+		vdb.Spec.UpgradeRequeueTime = 100 // Set a non-default UpgradeRequeueTime for the test
 		vdb.ObjectMeta.Annotations[vapi.VersionAnnotation] = version.OnlineUpgradeVersion
 
 		createVdb(ctx, vdb)

diff --git a/pkg/controllers/restart_reconcile.go b/pkg/controllers/restart_reconcile.go
@@ -25,6 +25,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	"github.com/vertica/vertica-kubernetes/pkg/paths"
@@ -127,14 +128,14 @@ func (r *RestartReconciler) reconcileCluster(ctx context.Context) (ctrl.Result,
 	// nodes.
 	// Include transient nodes since we may need to run re-ip against them.
 	downPods := r.PFacts.findRestartablePods(r.RestartReadOnly, true)
-	if res, err := r.killOldProcesses(ctx, downPods); res.Requeue || err != nil {
+	if res, err := r.killOldProcesses(ctx, downPods); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
 	// re_ip/start_db require all pods to be running that have run the
 	// installation.  This check is done when we generate the map file
 	// (genMapFile).
-	if res, err := r.reipNodes(ctx, r.PFacts.findReIPPods(false)); err != nil || res.Requeue {
+	if res, err := r.reipNodes(ctx, r.PFacts.findReIPPods(false)); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 
@@ -162,7 +163,7 @@ func (r *RestartReconciler) reconcileNodes(ctx context.Context) (ctrl.Result, er
 			return ctrl.Result{Requeue: true}, nil
 		}
 
-		if res, err := r.restartPods(ctx, downPods); res.Requeue || res.RequeueAfter > 0 || err != nil {
+		if res, err := r.restartPods(ctx, downPods); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
@@ -183,7 +184,7 @@ func (r *RestartReconciler) reconcileNodes(ctx context.Context) (ctrl.Result, er
 			r.Log.Info("No pod found to run admintools from. Requeue reconciliation.")
 			return ctrl.Result{Requeue: true}, nil
 		}
-		if res, err := r.reipNodes(ctx, reIPPods); res.Requeue || err != nil {
+		if res, err := r.reipNodes(ctx, reIPPods); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}
@@ -205,7 +206,7 @@ func (r *RestartReconciler) restartPods(ctx context.Context, pods []*PodFact) (c
 	vnodeList := genRestartVNodeList(downPods)
 	ipList := genRestartIPList(downPods)
 
-	if res, err := r.killOldProcesses(ctx, downPods); res.Requeue || err != nil {
+	if res, err := r.killOldProcesses(ctx, downPods); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 

diff --git a/pkg/controllers/uninstall_reconcile.go b/pkg/controllers/uninstall_reconcile.go
@@ -24,6 +24,7 @@ import (
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/atconf"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/iter"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -100,7 +101,7 @@ func (s *UninstallReconciler) Reconcile(ctx context.Context, req *ctrl.Request)
 	}
 
 	for i := range subclusters {
-		if res, err := s.reconcileSubcluster(ctx, subclusters[i]); err != nil || res.Requeue {
+		if res, err := s.reconcileSubcluster(ctx, subclusters[i]); verrors.IsReconcileAborted(res, err) {
 			return res, err
 		}
 	}

diff --git a/pkg/controllers/version_reconciler.go b/pkg/controllers/version_reconciler.go
@@ -21,6 +21,7 @@ import (
 	"github.com/go-logr/logr"
 	vapi "github.com/vertica/vertica-kubernetes/api/v1beta1"
 	"github.com/vertica/vertica-kubernetes/pkg/cmds"
+	verrors "github.com/vertica/vertica-kubernetes/pkg/errors"
 	"github.com/vertica/vertica-kubernetes/pkg/events"
 	"github.com/vertica/vertica-kubernetes/pkg/names"
 	"github.com/vertica/vertica-kubernetes/pkg/version"
@@ -67,7 +68,7 @@ func (v *VersionReconciler) Reconcile(ctx context.Context, req *ctrl.Request) (c
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	if res, err := v.reconcileVersion(ctx, pod); res.Requeue || err != nil {
+	if res, err := v.reconcileVersion(ctx, pod); verrors.IsReconcileAborted(res, err) {
 		return res, err
 	}
 

diff --git a/tests/e2e/crash-before-createdb/10-assert.yaml b/tests/e2e/crash-before-createdb/10-assert.yaml
@@ -10,7 +10,6 @@
 # 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.
-
 apiVersion: vertica.com/v1beta1
 kind: VerticaDB
 metadata:
@@ -20,3 +19,24 @@ status:
     - installCount: 3
       addedToDBCount: 0
       upNodeCount: 0
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: v-crash-before-createdb-defsc-0
+status:
+  phase: Running
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: v-crash-before-createdb-defsc-1
+status:
+  phase: Running
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: v-crash-before-createdb-defsc-2
+status:
+  phase: Running