Skip to content

adding new pod-count test to the observability suite #3084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

adding new pod-count test to the observability suite #3084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions CATALOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Depending on the workload type, not all tests are required to pass to satisfy be

## Test cases summary

### Total test cases: 119
### Total test cases: 120

### Total suites: 10

Expand All @@ -18,7 +18,7 @@ Depending on the workload type, not all tests are required to pass to satisfy be
|lifecycle|18|[lifecycle](#lifecycle)|
|manageability|2|[manageability](#manageability)|
|networking|12|[networking](#networking)|
|observability|5|[observability](#observability)|
|observability|6|[observability](#observability)|
|operator|12|[operator](#operator)|
|performance|6|[performance](#performance)|
|platform-alteration|14|[platform-alteration](#platform-alteration)|
Expand All @@ -36,11 +36,11 @@ Depending on the workload type, not all tests are required to pass to satisfy be
|---|---|---|
|8|1|

### Non-Telco specific tests only: 70
### Non-Telco specific tests only: 71

|Mandatory|Optional|
|---|---|---|
|43|27|
|43|28|

### Telco specific tests only: 27

Expand Down Expand Up @@ -1203,6 +1203,23 @@ Test Cases are the specifications used to perform a meaningful test. Test cases
|Non-Telco|Mandatory|
|Telco|Mandatory|

#### observability-pod-count

|Property|Description|
|---|---|
|Unique ID|observability-pod-count|
|Description|Checks that all pods running at the beginning of the tests, continue to run throughout the test|
|Suggested Remediation|Ensure all expected pods are running|
|Best Practice Reference|https://redhat-best-practices-for-k8s.github.io/guide/#observability-pod-count|
|Exception Process|No exceptions|
|Impact Statement|Inconsistency of running pods can cause instability of the application.|
|Tags|common,observability|
|**Scenario**|**Optional/Mandatory**|
|Extended|Optional|
|Far-Edge|Optional|
|Non-Telco|Optional|
|Telco|Optional|

#### observability-pod-disruption-budget

|Property|Description|
Expand Down
1 change: 1 addition & 0 deletions cmd/certsuite/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func TestCertsuiteInfoCmd(t *testing.T) {
| observability-termination-policy |
| observability-pod-disruption-budget |
| observability-compatibility-with-next-ocp-release |
| observability-pod-count |
------------------------------------------------------------
`
assert.Equal(t, expectedOutput, string(out))
Expand Down
1 change: 1 addition & 0 deletions expected_results.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ testCases:
- observability-pod-disruption-budget
- observability-compatibility-with-next-ocp-release
- observability-termination-policy
- observability-pod-count
- operator-crd-versioning
- operator-crd-openapi-schema
- operator-install-source
Expand Down
12 changes: 6 additions & 6 deletions pkg/autodiscover/autodiscover.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ var data = DiscoveredTestData{}
const labelRegex = `(\S*)\s*:\s*(\S*)`
const labelRegexMatches = 3

func createLabels(labelStrings []string) (labelObjects []labelObject) {
func CreateLabels(labelStrings []string) (labelObjects []labelObject) {
for _, label := range labelStrings {
r := regexp.MustCompile(labelRegex)

Expand Down Expand Up @@ -159,8 +159,8 @@ func DoAutoDiscover(config *configuration.TestConfiguration) DiscoveredTestData
log.Fatal("Failed to retrieve storageClasses - err: %v", err)
}

podsUnderTestLabelsObjects := createLabels(config.PodsUnderTestLabels)
operatorsUnderTestLabelsObjects := createLabels(config.OperatorsUnderTestLabels)
podsUnderTestLabelsObjects := CreateLabels(config.PodsUnderTestLabels)
operatorsUnderTestLabelsObjects := CreateLabels(config.OperatorsUnderTestLabels)

log.Debug("Pods under test labels: %+v", podsUnderTestLabelsObjects)
log.Debug("Operators under test labels: %+v", operatorsUnderTestLabelsObjects)
Expand All @@ -181,11 +181,11 @@ func DoAutoDiscover(config *configuration.TestConfiguration) DiscoveredTestData
data.AllPackageManifests = getAllPackageManifests(oc.OlmPkgClient.PackageManifests(""))

data.Namespaces = namespacesListToStringList(config.TargetNameSpaces)
data.Pods, data.AllPods = findPodsByLabels(oc.K8sClient.CoreV1(), podsUnderTestLabelsObjects, data.Namespaces)
data.Pods, data.AllPods = FindPodsByLabels(oc.K8sClient.CoreV1(), podsUnderTestLabelsObjects, data.Namespaces)
data.AbnormalEvents = findAbnormalEvents(oc.K8sClient.CoreV1(), data.Namespaces)
probeLabels := []labelObject{{LabelKey: probeHelperPodsLabelName, LabelValue: probeHelperPodsLabelValue}}
probeNS := []string{config.ProbeDaemonSetNamespace}
data.ProbePods, _ = findPodsByLabels(oc.K8sClient.CoreV1(), probeLabels, probeNS)
data.ProbePods, _ = FindPodsByLabels(oc.K8sClient.CoreV1(), probeLabels, probeNS)
data.ResourceQuotaItems, err = getResourceQuotas(oc.K8sClient.CoreV1())
if err != nil {
log.Fatal("Cannot get resource quotas, err: %v", err)
Expand Down Expand Up @@ -223,7 +223,7 @@ func DoAutoDiscover(config *configuration.TestConfiguration) DiscoveredTestData
}

// Best effort mode autodiscovery for operand (running-only) pods.
pods, _ := findPodsByLabels(oc.K8sClient.CoreV1(), nil, data.Namespaces)
pods, _ := FindPodsByLabels(oc.K8sClient.CoreV1(), nil, data.Namespaces)
if err != nil {
log.Fatal("Failed to get running pods, err: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/autodiscover/autodiscover_pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func findPodsMatchingAtLeastOneLabel(oc corev1client.CoreV1Interface, labels []l
return allPods
}

func findPodsByLabels(oc corev1client.CoreV1Interface, labels []labelObject, namespaces []string) (runningPods, allPods []corev1.Pod) {
func FindPodsByLabels(oc corev1client.CoreV1Interface, labels []labelObject, namespaces []string) (runningPods, allPods []corev1.Pod) {
runningPods = []corev1.Pod{}
allPods = []corev1.Pod{}
// Iterate through namespaces
Expand Down
2 changes: 1 addition & 1 deletion pkg/autodiscover/autodiscover_pods_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func TestFindPodsUnderTest(t *testing.T) {
testRuntimeObjects = append(testRuntimeObjects, generatePod(tc.testPodName, tc.testPodNamespace, tc.queryLabel))
oc := clientsholder.GetTestClientsHolder(testRuntimeObjects)

podResult, _ := findPodsByLabels(oc.K8sClient.CoreV1(), testLabel, testNamespaces)
podResult, _ := FindPodsByLabels(oc.K8sClient.CoreV1(), testLabel, testNamespaces)
assert.Equal(t, tc.expectedResults, podResult)
}
}
4 changes: 2 additions & 2 deletions pkg/autodiscover/autodiscover_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ func TestCreateLabels(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if gotLabelObjects := createLabels(tt.args.labelStrings); !reflect.DeepEqual(gotLabelObjects, tt.wantLabelObjects) {
t.Errorf("createLabels() = %v, want %v", gotLabelObjects, tt.wantLabelObjects)
if gotLabelObjects := CreateLabels(tt.args.labelStrings); !reflect.DeepEqual(gotLabelObjects, tt.wantLabelObjects) {
t.Errorf("CreateLabels() = %v, want %v", gotLabelObjects, tt.wantLabelObjects)
}
})
}
Expand Down
17 changes: 17 additions & 0 deletions tests/identifiers/identifiers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ var (
TestNamespaceResourceQuotaIdentifier claim.Identifier
TestPodDisruptionBudgetIdentifier claim.Identifier
TestAPICompatibilityWithNextOCPReleaseIdentifier claim.Identifier
TestPodCountIdentifier claim.Identifier
TestPodTolerationBypassIdentifier claim.Identifier
TestPersistentVolumeReclaimPolicyIdentifier claim.Identifier
TestContainersImageTag claim.Identifier
Expand Down Expand Up @@ -1677,6 +1678,22 @@ that Node's kernel may not have the same hacks.'`,
},
TagCommon)

TestPodCountIdentifier = AddCatalogEntry(
"pod-count",
common.ObservabilityTestKey,
`Checks that all pods running at the beginning of the tests, continue to run throughout the test`,
"Ensure all expected pods are running",
NoExceptions,
"https://redhat-best-practices-for-k8s.github.io/guide/#observability-pod-count",
true,
map[string]string{
FarEdge: Optional,
Telco: Optional,
NonTelco: Optional,
Extended: Optional,
},
TagCommon)

TestPodTolerationBypassIdentifier = AddCatalogEntry(
"pod-toleration-bypass",
common.LifecycleTestKey,
Expand Down
2 changes: 2 additions & 0 deletions tests/identifiers/impact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ const (
TestCrdsStatusSubresourceIdentifierImpact = `Missing status subresources prevent proper monitoring and automation based on custom resource states.`
TestPodDisruptionBudgetIdentifierImpact = `Improper disruption budgets can prevent necessary maintenance operations or allow too many pods to be disrupted simultaneously.`
TestAPICompatibilityWithNextOCPReleaseIdentifierImpact = `Deprecated API usage can cause applications to break during OpenShift upgrades, requiring emergency fixes.`
TestPodCountIdentifierImpact = `Inconsistency of running pods can cause instability of the application.`

// Manageability Test Suite Impact Statements
TestContainersImageTagImpact = `Missing image tags make it difficult to track versions, perform rollbacks, and maintain deployment consistency.`
Expand Down Expand Up @@ -277,6 +278,7 @@ var ImpactMap = map[string]string{
"observability-crd-status": TestCrdsStatusSubresourceIdentifierImpact,
"observability-pod-disruption-budget": TestPodDisruptionBudgetIdentifierImpact,
"observability-compatibility-with-next-ocp-release": TestAPICompatibilityWithNextOCPReleaseIdentifierImpact,
"observability-pod-count": TestPodCountIdentifierImpact,

// Manageability Test Suite
"manageability-containers-image-tag": TestContainersImageTagImpact,
Expand Down
82 changes: 78 additions & 4 deletions tests/observability/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ import (
"strings"

"github.com/Masterminds/semver/v3"
"github.com/redhat-best-practices-for-k8s/certsuite/tests/common"
"github.com/redhat-best-practices-for-k8s/certsuite/tests/identifiers"
pdbv1 "github.com/redhat-best-practices-for-k8s/certsuite/tests/observability/pdb"

apiserv1 "github.com/openshift/api/apiserver/v1"
"github.com/redhat-best-practices-for-k8s/certsuite/internal/clientsholder"
"github.com/redhat-best-practices-for-k8s/certsuite/internal/log"
"github.com/redhat-best-practices-for-k8s/certsuite/pkg/autodiscover"
"github.com/redhat-best-practices-for-k8s/certsuite/pkg/checksdb"
"github.com/redhat-best-practices-for-k8s/certsuite/pkg/provider"
"github.com/redhat-best-practices-for-k8s/certsuite/pkg/testhelper"
"github.com/redhat-best-practices-for-k8s/certsuite/tests/common"
"github.com/redhat-best-practices-for-k8s/certsuite/tests/identifiers"
pdbv1 "github.com/redhat-best-practices-for-k8s/certsuite/tests/observability/pdb"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -88,6 +88,13 @@ func LoadChecks() {
testAPICompatibilityWithNextOCPRelease(c, &env)
return nil
}))

checksGroup.Add(checksdb.NewCheck(identifiers.GetTestIDAndLabels(identifiers.TestPodCountIdentifier)).
WithSkipCheckFn(testhelper.GetNoPodsUnderTestSkipFn(&env)).
WithCheckFn(func(c *checksdb.Check) error {
testComparePodCount(c, &env)
return nil
}))
}

// containerHasLoggingOutput helper function to get the last line of logging output from
Expand Down Expand Up @@ -423,3 +430,70 @@ func testAPICompatibilityWithNextOCPRelease(check *checksdb.Check, env *provider
// Add test results
check.SetResult(compliantObjects, nonCompliantObjects)
}

// Function to compare the number of running pods to those loaded during autodiscover at the start of test execution.
func testComparePodCount(check *checksdb.Check, env *provider.TestEnvironment) {
oc := clientsholder.GetClientsHolder()

originalPods := env.Pods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of comparing directly pods, why not compare the status of the deployment and stateful set instead ( ready replicas for instance). So in this test, you could use the <statefulset/deployment name>-replica-/ to add a stable reference to pods in your results list. Otherwise, we would have a lot of false positives if the pod recreation test is triggered as a different uuid is appended to the pod after they are deleted and recreated.
For the orphan pods (the pods with owner references that are not a statefulset or replicaset), if any, we could compare them as already described here. See owner reference example: testPodsOwnerReference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the test the way it was described in the jira, if we want something other then pods before and pods after, then is it really a pod comparison tests?

I'm happy to write whatever, but it's odd that this question came up, so is what we are trying to accomplish even needed, if we are unsure of what we really want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the test the way it was described in the jira, if we want something other then pods before and pods after, then is it really a pod comparison tests?

The jira describes counting the number of ready and non ready pods before and after running the suite. See below:
Add a new test case that checks and collects the number of ready and non ready pods before and after running the certsuite to have an idea of any changes that have occurred while running the suite. This is a quick gauge to check the stability of the workload while running the suite.

I like the idea of adding more details in term of which pods became not ready (or ready) after running the suite, but in my understanding this would work only if the pod names are stable after being re-created. Most pods names that belong to deployment/statefulsets will be different after pod re-creation because of the randomized identifier added at the end. For instance in the self-node-remediation-ds-g26dh pod name "-g26dh" will change when this pod is re-created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you are saying, but if a pod gets a new uuid, doesn't that mean that it's not stable, and should be reported?

This is a quick gauge to check the stability of the workload while running the suite.

It was already stated that test will have false positives/negatives, this is why it's not mandatory anywhere (which IMO is odd, since other certifications don't work this way, but that's another story).

Copy link
Member

@edcdavid edcdavid Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you are saying, but if a pod gets a new uuid, doesn't that mean that it's not stable, and should be reported?

yep it could indicative of instability, especially if the pod keeps crashing with CrashLoopBackOff but not when simply being terminated and recreated. We know that we have a lifecycle-pod-recreation deleting pods and re-creating them so we expect these uuid to change. Instead, the goal of this test is to catch any degradation of the application, by checking after the test if pods that used to be ready are no longer ready, or pods that used to be not ready are now ready (whether uuid changed or not).

It was already stated that test will have false positives/negatives, this is why it's not mandatory anywhere

Even if not mandatory, in my opinion, we should keep the false positives/negatives to a minimum. I feel that if just comparing the name for most pods it would not match because most pods have an owner and add this uuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many tests in the test suite manipulate pods? Can tests be ordered / have priority?

Copy link
Member

@edcdavid edcdavid Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests that manipulate pods are identified by this function: GetNotIntrusiveSkipFn. This allows to have a switch to skip them. 4 tests: lifecycle-crd-scaling, lifecycle-deployment-scaling, lifecycle-statefulset-scaling, lifecycle-pod-recreation.
The tests are ordered in the order they are added in the code, we have not implemented changing the order or priority.
Maybe this test could be re-run after each intrusive tests?


currentPods, _ := autodiscover.FindPodsByLabels(oc.K8sClient.CoreV1(), autodiscover.CreateLabels(env.Config.PodsUnderTestLabels), env.Namespaces)

var compliantObjects []*testhelper.ReportObject
var nonCompliantObjects []*testhelper.ReportObject

// Compare pod counts
originalPodCount := len(originalPods)
currentPodCount := len(currentPods)

if originalPodCount == currentPodCount {
check.LogInfo("Pod count is consistent")
compliantObjects = append(compliantObjects,
testhelper.NewReportObject("Pod count is consistent", "PodCount", true).AddField("OriginalCount", fmt.Sprintf("%d", originalPodCount)).AddField("CurrentCount", fmt.Sprintf("%d", currentPodCount)))
} else {
check.LogError("Pod count mismatch: original=%d, current=%d", originalPodCount, currentPodCount)
nonCompliantObjects = append(nonCompliantObjects,
testhelper.NewReportObject("Pod count mismatch", "PodCount", false).AddField("OriginalCount", fmt.Sprintf("%d", originalPodCount)).AddField("CurrentCount", fmt.Sprintf("%d", currentPodCount)))
}

// Create maps for detailed comparison
originalPodsMap := make(map[string]struct{})
for _, pod := range originalPods {
key := fmt.Sprintf("%s/%s", pod.Namespace, pod.Name)
originalPodsMap[key] = struct{}{}
}

currentPodsMap := make(map[string]struct{})
for i := range currentPods {
pod := currentPods[i]
key := fmt.Sprintf("%s/%s", pod.Namespace, pod.Name)
currentPodsMap[key] = struct{}{}
}

// Check for missing pods (in original but not in current)
for _, originalPod := range originalPods {
podKey := fmt.Sprintf("%s/%s", originalPod.Namespace, originalPod.Name)
if _, exists := currentPodsMap[podKey]; !exists {
check.LogError("Pod %q is missing from current state", originalPod.String())
nonCompliantObjects = append(nonCompliantObjects,
testhelper.NewReportObject("Pod is missing from current state", testhelper.PodType, false).AddField(testhelper.PodName, originalPod.Name).AddField(testhelper.Namespace, originalPod.Namespace))
} else {
check.LogInfo("Pod %q is present in current state", originalPod.String())
compliantObjects = append(compliantObjects,
testhelper.NewReportObject("Pod is present in current state", testhelper.PodType, true).AddField(testhelper.PodName, originalPod.Name).AddField(testhelper.Namespace, originalPod.Namespace))
}
}

// Check for extra pods (in current but not in original)
for i := range currentPods {
currentPod := currentPods[i]
podKey := fmt.Sprintf("%s/%s", currentPod.Namespace, currentPod.Name)
if _, exists := originalPodsMap[podKey]; !exists {
check.LogError("Extra pod %s/%s found in current state", currentPod.Namespace, currentPod.Name)
nonCompliantObjects = append(nonCompliantObjects,
testhelper.NewReportObject("Extra pod found in current state", testhelper.PodType, false).AddField(testhelper.PodName, currentPod.Name).AddField(testhelper.Namespace, currentPod.Namespace))
}
}

check.SetResult(compliantObjects, nonCompliantObjects)
}
Loading