Add monitortest for HA policy management - #31449
Conversation
Signed-off-by: Naoya Horiguchi <nahorigu@redhat.com> Signed-off-by: Naoya Horiguchi <naoya-horiguchi@nec.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
ready-for-human-review
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nhoriguchi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds a universal HA policy management monitor that scans selected namespaces, validates workload probes, PodDisruptionBudgets, and anti-affinity, and reports aggregated namespace results as JUnit test cases. ChangesHA policy management monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MonitorFramework
participant haPolicyManagementChecker
participant KubernetesAPI
participant JUnitOutput
MonitorFramework->>haPolicyManagementChecker: StartCollection REST config
haPolicyManagementChecker->>KubernetesAPI: List selected namespaces and workloads
KubernetesAPI-->>haPolicyManagementChecker: Return resource objects
haPolicyManagementChecker->>haPolicyManagementChecker: Evaluate probes, PDBs, and anti-affinity
haPolicyManagementChecker->>JUnitOutput: Emit one test case per namespace
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/monitortests/monitoring/hapolicymanagement/monitortest.go`:
- Around line 53-71: Correct the boolean argument at all call sites of
checkPodReadinessLivenessProbes and checkPodStartupProbes: pass false for
regular Containers and true for InitContainers so probe validation runs only on
workload containers. Apply this consistently in the Deployment, StatefulSet,
DaemonSet, and Pod loops, preserving the existing skip behavior for init or
sidecar containers.
- Around line 119-135: Update checkPodAntiAffinity so a nil Affinity or
PodAntiAffinity is reported as a missing anti-affinity violation instead of
returning nil. Preserve the existing error message and continue rejecting
configurations where both required and preferred anti-affinity lists are empty.
- Around line 176-259: Handle and report errors from every Kubernetes List call
in the namespace monitoring flow, including PodDisruptionBudgets, Deployments,
StatefulSets, DaemonSets, and Pods. Replace each discarded error with explicit
handling that appends the collection failure to failures and prevents the
corresponding resource checks from treating an unavailable collection as empty;
preserve normal iteration when listing succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 728fa0b5-a7a9-4997-adff-cbe6295159a5
📒 Files selected for processing (2)
pkg/defaultmonitortests/types.gopkg/monitortests/monitoring/hapolicymanagement/monitortest.go
| func checkPodReadinessLivenessProbes(namespace string, initOrSidecar bool, container corev1.Container) error { | ||
| if initOrSidecar { | ||
| return nil | ||
| } | ||
|
|
||
| if container.LivenessProbe == nil { | ||
| return fmt.Errorf("container %s in ns/%s should have livenessProbe", container.Name, namespace) | ||
| } | ||
|
|
||
| if !checkNonMetricsPorts(container) { | ||
| return nil | ||
| } | ||
|
|
||
| if container.ReadinessProbe == nil { | ||
| return fmt.Errorf("container %s in ns/%s should have readinessProbe", container.Name, namespace) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
initOrSidecar boolean is passed inverted at every call site — probe checks never run on real containers.
checkPodReadinessLivenessProbes and checkPodStartupProbes both skip validation (return nil) when initOrSidecar is true. But every caller passes true for the main Containers loop and false for the InitContainers loop (Deployments: lines 180/183 vs 188/191; StatefulSets: 211/214 vs 219/222; DaemonSets: 242/245 vs 250/253; Pods: 267/270 vs 275/278). This means the checks are skipped for regular application containers — the actual target of this policy — and instead incorrectly enforced on init containers, which typically don't need liveness/readiness probes. The monitor as written will never catch a missing probe on a real workload container.
🐛 Proposed fix (apply to all 8 call sites)
- for _, container := range dep.Spec.Template.Spec.Containers {
- if err := checkPodReadinessLivenessProbes(ns.Name, true, container); err != nil {
+ for _, container := range dep.Spec.Template.Spec.Containers {
+ if err := checkPodReadinessLivenessProbes(ns.Name, false, container); err != nil {
failures = append(failures, err.Error())
}
- if err := checkPodStartupProbes(ns.Name, true, container); err != nil {
+ if err := checkPodStartupProbes(ns.Name, false, container); err != nil {
failures = append(failures, err.Error())
}
}
for _, container := range dep.Spec.Template.Spec.InitContainers {
- if err := checkPodReadinessLivenessProbes(ns.Name, false, container); err != nil {
+ if err := checkPodReadinessLivenessProbes(ns.Name, true, container); err != nil {
failures = append(failures, err.Error())
}
- if err := checkPodStartupProbes(ns.Name, false, container); err != nil {
+ if err := checkPodStartupProbes(ns.Name, true, container); err != nil {
failures = append(failures, err.Error())
}
}Apply the same swap to the StatefulSets, DaemonSets, and Pods loops.
Also applies to: 73-83, 178-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/monitortests/monitoring/hapolicymanagement/monitortest.go` around lines
53 - 71, Correct the boolean argument at all call sites of
checkPodReadinessLivenessProbes and checkPodStartupProbes: pass false for
regular Containers and true for InitContainers so probe validation runs only on
workload containers. Apply this consistently in the Deployment, StatefulSet,
DaemonSet, and Pod loops, preserving the existing skip behavior for init or
sidecar containers.
| func checkPodAntiAffinity(namespace string, name string, podTemplate corev1.PodTemplateSpec) error { | ||
| podSpec := podTemplate.Spec | ||
|
|
||
| if podSpec.Affinity == nil || podSpec.Affinity.PodAntiAffinity == nil { | ||
| return nil | ||
| } | ||
|
|
||
| antiAffinity := podSpec.Affinity.PodAntiAffinity | ||
| hasRequired := len(antiAffinity.RequiredDuringSchedulingIgnoredDuringExecution) > 0 | ||
| hasPreferred := len(antiAffinity.PreferredDuringSchedulingIgnoredDuringExecution) > 0 | ||
|
|
||
| if !hasRequired && !hasPreferred { | ||
| return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
checkPodAntiAffinity logic is inverted — workloads without any anti-affinity pass silently.
If podSpec.Affinity or PodAntiAffinity is nil (the common case for a workload that never configured anti-affinity), the function returns nil (no violation). Only when the struct exists but both RequiredDuringSchedulingIgnoredDuringExecution and PreferredDuringSchedulingIgnoredDuringExecution are empty does it report an error — an unusual, rare configuration. As written, this check will almost never fire for the actual non-compliant case it's meant to detect.
🐛 Proposed fix
func checkPodAntiAffinity(namespace string, name string, podTemplate corev1.PodTemplateSpec) error {
podSpec := podTemplate.Spec
- if podSpec.Affinity == nil || podSpec.Affinity.PodAntiAffinity == nil {
- return nil
- }
+ if podSpec.Affinity == nil || podSpec.Affinity.PodAntiAffinity == nil {
+ return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace)
+ }
antiAffinity := podSpec.Affinity.PodAntiAffinity
hasRequired := len(antiAffinity.RequiredDuringSchedulingIgnoredDuringExecution) > 0
hasPreferred := len(antiAffinity.PreferredDuringSchedulingIgnoredDuringExecution) > 0
if !hasRequired && !hasPreferred {
return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func checkPodAntiAffinity(namespace string, name string, podTemplate corev1.PodTemplateSpec) error { | |
| podSpec := podTemplate.Spec | |
| if podSpec.Affinity == nil || podSpec.Affinity.PodAntiAffinity == nil { | |
| return nil | |
| } | |
| antiAffinity := podSpec.Affinity.PodAntiAffinity | |
| hasRequired := len(antiAffinity.RequiredDuringSchedulingIgnoredDuringExecution) > 0 | |
| hasPreferred := len(antiAffinity.PreferredDuringSchedulingIgnoredDuringExecution) > 0 | |
| if !hasRequired && !hasPreferred { | |
| return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace) | |
| } | |
| return nil | |
| } | |
| func checkPodAntiAffinity(namespace string, name string, podTemplate corev1.PodTemplateSpec) error { | |
| podSpec := podTemplate.Spec | |
| if podSpec.Affinity == nil || podSpec.Affinity.PodAntiAffinity == nil { | |
| return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace) | |
| } | |
| antiAffinity := podSpec.Affinity.PodAntiAffinity | |
| hasRequired := len(antiAffinity.RequiredDuringSchedulingIgnoredDuringExecution) > 0 | |
| hasPreferred := len(antiAffinity.PreferredDuringSchedulingIgnoredDuringExecution) > 0 | |
| if !hasRequired && !hasPreferred { | |
| return fmt.Errorf("workload %s in ns/%s should have podAntiAffinity", name, namespace) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/monitortests/monitoring/hapolicymanagement/monitortest.go` around lines
119 - 135, Update checkPodAntiAffinity so a nil Affinity or PodAntiAffinity is
reported as a missing anti-affinity violation instead of returning nil. Preserve
the existing error message and continue rejecting configurations where both
required and preferred anti-affinity lists are empty.
| pdbList, _ := w.kubeClient.PolicyV1().PodDisruptionBudgets(ns.Name).List(ctx, metav1.ListOptions{}) | ||
| deployments, _ := w.kubeClient.AppsV1().Deployments(ns.Name).List(ctx, metav1.ListOptions{}) | ||
| for _, dep := range deployments.Items { | ||
| for _, container := range dep.Spec.Template.Spec.Containers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
| for _, container := range dep.Spec.Template.Spec.InitContainers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| if dep.Spec.Replicas != nil && *dep.Spec.Replicas <= 1 { | ||
| continue | ||
| } | ||
|
|
||
| if err := checkPodDisruptionBudget(ns.Name, dep.Name, dep.Spec.Template, dep.Spec.Selector, pdbList); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodAntiAffinity(ns.Name, dep.Name, dep.Spec.Template); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| statefulsets, _ := w.kubeClient.AppsV1().StatefulSets(ns.Name).List(ctx, metav1.ListOptions{}) | ||
| for _, sts := range statefulsets.Items { | ||
| for _, container := range sts.Spec.Template.Spec.Containers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
| for _, container := range sts.Spec.Template.Spec.InitContainers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| if sts.Spec.Replicas != nil && *sts.Spec.Replicas <= 1 { | ||
| continue | ||
| } | ||
|
|
||
| if err := checkPodDisruptionBudget(ns.Name, sts.Name, sts.Spec.Template, sts.Spec.Selector, pdbList); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodAntiAffinity(ns.Name, sts.Name, sts.Spec.Template); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| daemonsets, _ := w.kubeClient.AppsV1().DaemonSets(ns.Name).List(ctx, metav1.ListOptions{}) | ||
| for _, dms := range daemonsets.Items { | ||
| for _, container := range dms.Spec.Template.Spec.Containers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, true, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
| for _, container := range dms.Spec.Template.Spec.InitContainers { | ||
| if err := checkPodReadinessLivenessProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| if err := checkPodStartupProbes(ns.Name, false, container); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pods, _ := w.kubeClient.CoreV1().Pods(ns.Name).List(ctx, metav1.ListOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Kubernetes API list errors are silently discarded.
PodDisruptionBudgets/Deployments/StatefulSets/DaemonSets/Pods .List(...) calls all discard the error via _ (lines 176, 177, 208, 239, 259). A transient API failure or permission issue will be treated as "no resources found" rather than surfacing a failure, producing a false "compliant" JUnit result for that namespace instead of flagging the collection error.
As per path instructions for **/*.go, "Never ignore error returns."
🛡️ Example fix pattern (repeat for each List call)
- pdbList, _ := w.kubeClient.PolicyV1().PodDisruptionBudgets(ns.Name).List(ctx, metav1.ListOptions{})
- deployments, _ := w.kubeClient.AppsV1().Deployments(ns.Name).List(ctx, metav1.ListOptions{})
+ pdbList, err := w.kubeClient.PolicyV1().PodDisruptionBudgets(ns.Name).List(ctx, metav1.ListOptions{})
+ if err != nil {
+ failures = append(failures, fmt.Sprintf("failed to list PodDisruptionBudgets in ns/%s: %v", ns.Name, err))
+ }
+ deployments, err := w.kubeClient.AppsV1().Deployments(ns.Name).List(ctx, metav1.ListOptions{})
+ if err != nil {
+ failures = append(failures, fmt.Sprintf("failed to list Deployments in ns/%s: %v", ns.Name, err))
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/monitortests/monitoring/hapolicymanagement/monitortest.go` around lines
176 - 259, Handle and report errors from every Kubernetes List call in the
namespace monitoring flow, including PodDisruptionBudgets, Deployments,
StatefulSets, DaemonSets, and Pods. Replace each discarded error with explicit
handling that appends the collection failure to failures and prevents the
corresponding resource checks from treating an unavailable collection as empty;
preserve normal iteration when listing succeeds.
Source: Path instructions
1 participant
Enhancement: openshift/enhancements#1932
Summary by CodeRabbit