From 7e2063a3054cfa34de4135ed97b02a7104356a73 Mon Sep 17 00:00:00 2001 From: Gonzalo Reyero Ferreras <87083379+greyerof@users.noreply.github.com> Date: Mon, 20 Nov 2023 18:51:07 +0100 Subject: [PATCH] Allow more than one skip functions per check. (#1645) The check.WithSkipFn(func) has been replaced by a variadic func version to allow more than one skip functions to be called: check.WithSkipFn(skipFunc1, skipFunc2...) By default, the check will be skipped if any skip function returns true when they're called (which happens before running the CheckFn). If we need to skip the check in case all the skip functions return true, the check.WithSkipModeAll() needs to be used. Examples: 1. Skip check if no deployments *or* no statefulsets were found (testhelper.SkipIfEmptyAny()): check := checksdb.NewCheck(id, labels). WithSkipCheckFn(testhelper.GetNoDeploymentsUnderTestSkipFn(&env), testhelper.GetNoStatefulSetsUnderTestSkipFn(&env)). WithCheckFn(func() error { .. }) 2. We want to skip the check if no deployments *and* no statefulsets were found (testhelper.SkipIfEmptyAll()): check := checksdb.NewCheck(id, labels). WithSkipCheckFn(testhelper.GetNoDeploymentsUnderTestSkipFn(&env), testhelper.GetNoStatefulSetsUnderTestSkipFn(&env)). WithSkipCheckModeAll(). WithCheckFn(func() error { .. }) The skip functions are appended to a slice in the same order as they're passed to the check.WithSkipCheckFn() calls, and it can be called more than once. E.g.: in the previous examples, this call: ... WithSkipCheckFn(testhelper.GetNoDeploymentsUnderTestSkipFn(&env), testhelper.GetNoStatefulSetsUnderTestSkipFn(&env)). ... is equal to: ... WithSkipCheckFn(testhelper.GetNoDeploymentsUnderTestSkipFn(&env)). WithSkipCheckFn(testhelper.GetNoStatefulSetsUnderTestSkipFn(&env)). ... Extra changes: - Added helper skip func creators in the testhelper package for no CRDs, no statefulSets and no deployments found. - Adapted the observability ts. --- cnf-certification-test/observability/suite.go | 36 ++---------- pkg/checksdb/check.go | 36 +++++++++++- pkg/checksdb/checksgroup.go | 58 ++++++++++++++++--- pkg/testhelper/testhelper.go | 34 ++++++++++- 4 files changed, 121 insertions(+), 43 deletions(-) diff --git a/cnf-certification-test/observability/suite.go b/cnf-certification-test/observability/suite.go index 5e02ff686..56272f487 100644 --- a/cnf-certification-test/observability/suite.go +++ b/cnf-certification-test/observability/suite.go @@ -27,7 +27,6 @@ import ( "github.com/test-network-function/cnf-certification-test/cnf-certification-test/identifiers" pdbv1 "github.com/test-network-function/cnf-certification-test/cnf-certification-test/observability/pdb" - // "github.com/test-network-function/cnf-certification-test/cnf-certification-test/results" "github.com/test-network-function/cnf-certification-test/internal/clientsholder" "github.com/test-network-function/cnf-certification-test/pkg/checksdb" "github.com/test-network-function/cnf-certification-test/pkg/provider" @@ -44,32 +43,6 @@ var ( env = provider.GetTestEnvironment() return nil } - - skipIfNoContainersFn = func() (bool, string) { - if len(env.Containers) == 0 { - logrus.Warnf("No containers to check...") - return true, "There are no containers to check. Please check under test labels." - } - - return false, "" - } - - skipIfNoCrdsFn = func() (bool, string) { - if len(env.Crds) == 0 { - logrus.Warn("No CRDs to check.") - return true, "There are no CRDs to check." - } - - return false, "" - } - - skipIfNoDeploymentsNorStatefulSets = func() (bool, string) { - if len(env.Deployments) == 0 && len(env.StatefulSets) == 0 { - logrus.Warn("No deployments nor statefulsets to check.") - return true, "There are no deployments nor statefulsets to check." - } - return false, "" - } ) func init() { @@ -80,7 +53,7 @@ func init() { testID, tags := identifiers.GetGinkgoTestIDAndLabels(identifiers.TestLoggingIdentifier) check := checksdb.NewCheck(testID, tags). - WithSkipCheckFn(skipIfNoContainersFn). + WithSkipCheckFn(testhelper.GetNoContainersUnderTestSkipFn(&env)). WithCheckFn(func(c *checksdb.Check) error { testContainersLogging(c, &env) return nil @@ -90,7 +63,7 @@ func init() { testID, tags = identifiers.GetGinkgoTestIDAndLabels(identifiers.TestCrdsStatusSubresourceIdentifier) check = checksdb.NewCheck(testID, tags). - WithSkipCheckFn(skipIfNoCrdsFn). + WithSkipCheckFn(testhelper.GetNoCrdsUnderTestSkipFn(&env)). WithCheckFn(func(c *checksdb.Check) error { testCrds(c, &env) return nil @@ -100,7 +73,7 @@ func init() { testID, tags = identifiers.GetGinkgoTestIDAndLabels(identifiers.TestTerminationMessagePolicyIdentifier) check = checksdb.NewCheck(testID, tags). - WithSkipCheckFn(skipIfNoContainersFn). + WithSkipCheckFn(testhelper.GetNoContainersUnderTestSkipFn(&env)). WithCheckFn(func(c *checksdb.Check) error { testTerminationMessagePolicy(c, &env) return nil @@ -110,7 +83,8 @@ func init() { testID, tags = identifiers.GetGinkgoTestIDAndLabels(identifiers.TestPodDisruptionBudgetIdentifier) check = checksdb.NewCheck(testID, tags). - WithSkipCheckFn(skipIfNoDeploymentsNorStatefulSets). + WithSkipCheckFn(testhelper.GetNoDeploymentsUnderTestSkipFn(&env), testhelper.GetNoStatefulSetsUnderTestSkipFn(&env)). + WithSkipModeAll(). WithCheckFn(func(c *checksdb.Check) error { testPodDisruptionBudgets(c, &env) return nil diff --git a/pkg/checksdb/check.go b/pkg/checksdb/check.go index 07269de59..5bc0572ea 100644 --- a/pkg/checksdb/check.go +++ b/pkg/checksdb/check.go @@ -17,6 +17,13 @@ const ( CheckResultAborted = "aborted" ) +type skipMode int + +const ( + SkipModeAny skipMode = iota + SkipModeAll +) + type CheckResult string func (cr CheckResult) String() string { @@ -31,7 +38,8 @@ type Check struct { BeforeCheckFn, AfterCheckFn func(check *Check) error CheckFn func(check *Check) error - SkipCheckFn func() (skip bool, reason string) + SkipCheckFns []func() (skip bool, reason string) + SkipMode skipMode Result CheckResult CapturedOutput string @@ -77,12 +85,34 @@ func (check *Check) WithAfterCheckFn(afterCheckFn func(check *Check) error) *Che return check } -func (check *Check) WithSkipCheckFn(skipCheckFn func() (skip bool, reason string)) *Check { +func (check *Check) WithSkipCheckFn(skipCheckFn ...func() (skip bool, reason string)) *Check { + if check.Error != nil { + return check + } + + check.SkipCheckFns = append(check.SkipCheckFns, skipCheckFn...) + + return check +} + +// This modifier is provided for the sake of completeness, but it's not necessary to use it, +// as the SkipModeAny is the default skip mode. +func (check *Check) WithSkipModeAny() *Check { + if check.Error != nil { + return check + } + + check.SkipMode = SkipModeAny + + return check +} + +func (check *Check) WithSkipModeAll() *Check { if check.Error != nil { return check } - check.SkipCheckFn = skipCheckFn + check.SkipMode = SkipModeAll return check } diff --git a/pkg/checksdb/checksgroup.go b/pkg/checksdb/checksgroup.go index 38ce53dae..08f1a7f04 100644 --- a/pkg/checksdb/checksgroup.go +++ b/pkg/checksdb/checksgroup.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "runtime/debug" + "strings" "github.com/sirupsen/logrus" ) @@ -195,13 +196,56 @@ func runAfterEachFn(group *ChecksGroup, check *Check, remainingChecks []*Check) return nil } -func shouldSkipCheck(check *Check) (skip bool, reason string) { - if check.SkipCheckFn == nil { - return false, "" +func shouldSkipCheck(check *Check) (skip bool, reasons []string) { + if len(check.SkipCheckFns) == 0 { + return false, []string{} } - logrus.Tracef("Running check %s skipCheck function.", check.ID) - return check.SkipCheckFn() + logrus.Tracef("Running check %s skipCheck functions (%d).", check.ID, len(check.SkipCheckFns)) + + // Short-circuit + if len(check.SkipCheckFns) == 0 { + return false, []string{} + } + + // Save the skipFn index in case it panics so it can be used in the log trace. + currentSkipFnIndex := 0 + + defer func() { + if r := recover(); r != nil { + stackTrace := fmt.Sprint(r) + "\n" + string(debug.Stack()) + logrus.Errorf("Skip check function (idx=%d) panic'ed: %s", currentSkipFnIndex, stackTrace) + skip = true + reasons = []string{fmt.Sprintf("skipCheckFn (idx=%d) panic:\n%s", currentSkipFnIndex, stackTrace)} + } + }() + + // Call all the skip functions first. + for _, skipFn := range check.SkipCheckFns { + if skip, reason := skipFn(); skip { + reasons = append(reasons, reason) + } + currentSkipFnIndex++ + } + + // If none of the skipFn returned true, exit now. + if len(reasons) == 0 { + return false, []string{} + } + + // Now we need to check the skipMode for this check. + switch check.SkipMode { + case SkipModeAny: + return true, reasons + case SkipModeAll: + // Only skip if all the skipFn returned true. + if len(reasons) == len(check.SkipCheckFns) { + return true, reasons + } + return false, []string{} + } + + return false, []string{} } func runCheck(check *Check, group *ChecksGroup, remainingChecks []*Check) (err error) { @@ -298,9 +342,9 @@ func (group *ChecksGroup) RunChecks(labelsExpr string, stopChan <-chan bool) (er if !beforeEachFailed { // Should we skip this check? - skip, reason := shouldSkipCheck(check) + skip, reasons := shouldSkipCheck(check) if skip { - skipCheck(check, reason) + skipCheck(check, strings.Join(reasons, ", ")) } else { err := runCheck(check, group, remainingChecks) if err != nil { diff --git a/pkg/testhelper/testhelper.go b/pkg/testhelper/testhelper.go index 8f6d40250..48c08f3ec 100644 --- a/pkg/testhelper/testhelper.go +++ b/pkg/testhelper/testhelper.go @@ -329,7 +329,7 @@ func ResultToString(result int) (str string) { func GetNoContainersUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, string) { return func() (bool, string) { if len(env.Containers) == 0 { - return true, "There are no containers to check. Please check under test labels." + return true, "no containers to check found" } return false, "" @@ -339,7 +339,37 @@ func GetNoContainersUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, func GetNoPodsUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, string) { return func() (bool, string) { if len(env.Pods) == 0 { - return true, "There are no pods to check. Please check under test labels." + return true, "no pods to check found" + } + + return false, "" + } +} + +func GetNoDeploymentsUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, string) { + return func() (bool, string) { + if len(env.Deployments) == 0 { + return true, "no deployments to check found" + } + + return false, "" + } +} + +func GetNoStatefulSetsUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, string) { + return func() (bool, string) { + if len(env.StatefulSets) == 0 { + return true, "no statefulSets to check found" + } + + return false, "" + } +} + +func GetNoCrdsUnderTestSkipFn(env *provider.TestEnvironment) func() (bool, string) { + return func() (bool, string) { + if len(env.Crds) == 0 { + return true, "no CRDs to check found" } return false, ""