WINC-1967: OTE Migration Batch 2 - Monitoring & Observability - #4319
WINC-1967: OTE Migration Batch 2 - Monitoring & Observability#4319rrasouli wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rrasouli: This pull request references WINC-1967 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Windows e2e utilities now provide cluster queries, version parsing, Prometheus metric extraction, WMCO log parsing, pod execution, provider ID extraction, platform detection, and BYOH checks. Existing tests use shared WMCO deployment helpers. New tests validate Prometheus rules, containerd versions, windows-exporter configuration and HTTPS behavior, and provider ID consistency between Windows Nodes and Machines. Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ 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 |
openshift-ci
Bot
added
the
approved
|
/approve cancel |
openshift-ci
Bot
removed
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
ote/test/e2e/utils.go (1)
268-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
wait.Pollis deprecated.
wait.Poll(line 274) is deprecated in favor of context-aware polling (wait.PollUntilContextTimeout) and is documented as "Will be removed in a future release" ink8s.io/apimachinery. Sinceapimachineryis already pinned at v0.35.1 ingo.mod, consider migrating now to avoid a forced rewrite later.🤖 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 `@ote/test/e2e/utils.go` around lines 268 - 305, The waitUntilWMCOStatusChanged function still uses deprecated wait.Poll; migrate this polling call to wait.PollUntilContextTimeout using an appropriate context and the existing poll interval and timeout, while preserving the current log retrieval, retry behavior, and final assertion.
🤖 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 `@ote/cmd/wmco-tests-ext/main.go`:
- Around line 34-35: Ensure the error paths in main, including the additional
occurrence around the referenced later lines, flush logs before calling
os.Exit(1). Update the control flow around logs.FlushLogs and os.Exit so
deferred cleanup is not relied upon when terminating the process.
In `@ote/test/e2e/utils.go`:
- Around line 219-226: Update the error construction in the jq execution flow
around cmd.Output() to wrap err with %w instead of %v, preserving
errors.Is/errors.As unwrapping while retaining the existing resourceType context
and logging.
- Around line 173-184: Update getValueFromText to handle the case where no line
contains searchVal before indexing the split result. Return a safe empty value
(or otherwise preserve the caller’s expected clean-failure behavior) when
searchVal is absent, while retaining the existing trimmed substring behavior
when it is found.
In `@ote/test/e2e/winc.go`:
- Around line 311-317: Replace the assertion on len(winInternalIPs) in the
“Verifying HTTP is not allowed on metrics endpoint” test with a conditional skip
when fewer than two Windows internal IPs are available. Keep the cross-node curl
and response validation unchanged for clusters with at least two nodes.
- Around line 311-317: Update the metrics endpoint URL construction in the HTTP
rejection test to use IPv6-safe host/port joining via the existing
net.JoinHostPort pattern, preserving the winInternalIPs[1] address, port 9182,
and /metrics path.
- Line 193: The call sites invoking execInPod in the relevant test cases discard
execution errors; capture and log the returned error before evaluating the
command output. Update both the curl check and the other execInPod call noted by
the review, preserving their existing stdout assertions while ensuring transport
or execution failures are visible.
- Around line 262-285: Remove the public GitHub request from the
“Smokerun-Author:rrasouli-Medium-60814-Check containerd version is properly
reported” test and source the expected containerd version from an already
available in-cluster WMCO artifact via the existing oc-based helpers. Preserve
the comparison against getContainerdVersion for each Windows host, and ensure
any retained HTTP request uses a bounded timeout and response cleanup.
- Around line 113-117: Replace the shell-based file read in the smokerun test
with direct os.ReadFile(publicKey), adding the os import as needed. Preserve the
existing error assertion and public-key parsing behavior after reading the file.
---
Nitpick comments:
In `@ote/test/e2e/utils.go`:
- Around line 268-305: The waitUntilWMCOStatusChanged function still uses
deprecated wait.Poll; migrate this polling call to wait.PollUntilContextTimeout
using an appropriate context and the existing poll interval and timeout, while
preserving the current log retrieval, retry behavior, and final assertion.
🪄 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: Enterprise
Run ID: cfd8a973-dd13-441c-acd4-2059efa5ed34
⛔ Files ignored due to path filters (1)
ote/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
ote/OWNERSote/cmd/wmco-tests-ext/main.goote/go.modote/test/e2e/utils.goote/test/e2e/winc.goote/test/extended/cli/cli.goote/test/extended/cluster.go
💤 Files with no reviewable changes (2)
- ote/test/extended/cli/cli.go
- ote/test/extended/cluster.go
221c5b3 to
f08a92b
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ote/test/e2e/winc.go (1)
275-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getContainerdVersion(oc, winhost)is called twice per host.The comparison calls it once, and the
Failfmessage calls it again — doubling the remote exec cost per host and risking a slightly inconsistent message if the value could change between calls. Cache it once per iteration; also prefer!=overstrings.Compare(...) != 0for equality checks.♻️ Proposed fix
for _, winhost := range getWindowsHostNames(oc) { - if strings.Compare(submoduleContainerdVersion, getContainerdVersion(oc, winhost)) != 0 { - e2e.Failf("Containerd version mismatch expected %s actual %s", submoduleContainerdVersion, getContainerdVersion(oc, winhost)) - } + actualVersion := getContainerdVersion(oc, winhost) + if submoduleContainerdVersion != actualVersion { + e2e.Failf("Containerd version mismatch expected %s actual %s", submoduleContainerdVersion, actualVersion) + } }🤖 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 `@ote/test/e2e/winc.go` around lines 275 - 279, Within the host loop, update the version check around getContainerdVersion to assign its result to a local variable once, then compare that cached value directly with != against submoduleContainerdVersion and reuse it in e2e.Failf. Remove the unnecessary strings.Compare call while preserving the existing mismatch message and behavior.
🤖 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 `@ote/test/e2e/utils.go`:
- Line 155: Update the error construction in the kubelet-version retry flow to
use `%w` for the underlying err instead of `%v`, preserving the existing context
message and enabling callers to unwrap the error with errors.Is or errors.As.
- Around line 214-266: Update extractInstanceID to replace the jq subprocess
with native gjson parsing of .items, extracting each metadata.name and
spec.providerID while preserving the existing validation, logging, and error
behavior. Compile the providerID regex once before iterating over items, then
reuse it for every entry instead of calling regexp.MustCompile inside the loop.
In `@ote/test/e2e/winc.go`:
- Around line 344-367: Update the lookup in the validation loop around
getWindowsHostNames and nodeProviderIDs so both use the same node identifier
field. Prefer returning .metadata.name from getWindowsHostNames, or otherwise
make its contract match the nodeProviderIDs key, while preserving the existing
provider-ID and Machine association checks.
---
Nitpick comments:
In `@ote/test/e2e/winc.go`:
- Around line 275-279: Within the host loop, update the version check around
getContainerdVersion to assign its result to a local variable once, then compare
that cached value directly with != against submoduleContainerdVersion and reuse
it in e2e.Failf. Remove the unnecessary strings.Compare call while preserving
the existing mismatch message and behavior.
🪄 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: Enterprise
Run ID: c7ad0d25-e2d1-4879-8dd2-0e0b2d8df951
📒 Files selected for processing (2)
ote/test/e2e/utils.goote/test/e2e/winc.go
f08a92b to
53b93b7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
efa210f to
db56e27
Compare
|
/test vsphere-disconnected-e2e-operator |
|
/lgtm This looks great! |
|
/test aws-e2e-ote |
6767a9e to
bb5619a
Compare
|
/retest |
Migrate 5 monitoring/observability Smokerun tests from OTP to WMCO OTE: - OCP-33768: Prometheus alert rules ignore Windows nodes - OCP-60814: Containerd version matches Makefile - OCP-70922: Pod CPU/Memory/Filesystem metrics + OCPBUGS-85061 - OCP-77777: ServiceMonitor, HTTPS metrics, HTTP rejection - OCP-79251: Provider ID matching between nodes and machines Dropped OCP-33779 (node logs validation) - covered by WMCO e2e testNodeLogs. CI fixes included: - OCP-60814: Skip when WMCO version ends with -dirty (PR builds) - OCP-79251: Fix isNone() to query Machines instead of MachineSets - Fix OTE SSH key loading for CI environments - Fix OTE build for local and CI environments
bb5619a to
9e966c1
Compare
|
/hold @rrasouli aws-e2e-ote — Job failed. PTAL |
openshift-ci
Bot
added
the
do-not-merge/hold
@jrvaldes failed due to bug https://redhat.atlassian.net/browse/WINC-2009 |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@rrasouli can you point me to the log where it says the test failed due to kubelet mismatch? |
@rrasouli I dont think the changes introduced in the PR are affect in the e2e test suite. please open a PR in the release repo skipping the e2e test for changes in /ote dir |
|
/override ci/prow/aws-e2e-operator ci/prow/azure-e2e-operator ci/prow/azure-e2e-upgrade ci/prow/gcp-e2e-operator ci/prow/nutanix-e2e-operator ci/prow/platform-none-vsphere-e2e-operator ci/prow/vsphere-disconnected-e2e-operator ci/prow/vsphere-e2e-operator ci/prow/vsphere-proxy-e2e-operator ci/prow/images ci/prow/lint ci/prow/security ci/prow/unit ci/prow/wicd-unit-vsphere ci/prow/ci-bundle-wmco-bundle |
|
@jrvaldes: Overrode contexts on behalf of jrvaldes: ci/prow/aws-e2e-operator, ci/prow/azure-e2e-operator, ci/prow/azure-e2e-upgrade, ci/prow/ci-bundle-wmco-bundle, ci/prow/gcp-e2e-operator, ci/prow/images, ci/prow/lint, ci/prow/nutanix-e2e-operator, ci/prow/platform-none-vsphere-e2e-operator, ci/prow/security, ci/prow/unit, ci/prow/vsphere-disconnected-e2e-operator, ci/prow/vsphere-e2e-operator, ci/prow/vsphere-proxy-e2e-operator, ci/prow/wicd-unit-vsphere DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@rrasouli this is not a bug and is already tracked by https://redhat.atlassian.net/browse/WINC-1992 |
@jrvaldes anyway this issue is failing the test and the entire job @mansikulkarni96 |
|
/lgtm |
|
/approve |
|
/pj-rehearse ack |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rrasouli 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 |
can you approve as well? please |
|
/test aws-e2e-test |
|
/test aws-e2e-ote |
|
@rrasouli: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
Migrates 5 monitoring/observability tests from OTP to the OTE Ginkgo framework, continuing the batch migration tracked in WINC-1966. This PR stacks on Batch 1 (#4279).
New tests migrated:
Test removed (not migrated):
test/e2e/logs_test.go:testNodeLogs, which already validates the same 6 log paths viaoc adm node-logs. Disabling in OTP tracked in WINC-1978.Bug verification added:
instance:node_cpu_utilisation:rate1m{job="windows-exporter"}from Prometheus and asserts the value is < 0.5, confirming the recording rule reports CPU utilization (not idle rate).SSH Key Fix (CI Blocker)
The first aws-e2e-ote CI run failed all 10 tests because
compat_otp.GetPrivateKey()in BeforeEach tries to read../internal/config/keys/openshift-qe.pem, a path that only exists in Prow OTP environments. This blocked every test before it could run.Fix: Removed
compat_otp.GetPrivateKey()/GetPublicKey()from BeforeEach entirely. OTE tests useoc debug node/oc execper WINC-1931, not SSH. The only test that needed a public key (OCP-32615) now derives it from thecloud-private-keycluster secret viassh.ParsePrivateKey(), making the OTE binary portable across WMCO CI, Prow, and clusterBot.OCP-60814 Fix: Skip on PR builds
OCP-60814 fetches the Makefile from GitHub using the WMCO commit hash to compare
CONTAINERD_GIT_VERSIONagainst node containerd versions. In PR CI builds, the WMCO version isX.Y.Z-HASH-dirtywhere HASH is a PR-only commit that does not exist on upstream GitHub, causing a 404.Fix: Detect PR builds by checking for the
-dirtysuffix in the WMCO version string andg.Skipthe test. The test runs normally against released builds (FBC image) where the commit hash exists on GitHub.OCP-79251 Fix: isNone detection on AWS e2e
OCP-79251 was skipped on AWS e2e clusters because
isNone()failed to detect Windows MachineSets. The original implementation searched for MachineSets by name substring ("winworker"/"windows"), but WMCO e2e tests create MachineSets named$infraID-e2e-wmwhich did not match.Root cause: The
machine.openshift.io/os-id=Windowslabel is on the Machine template (applied to Machines when created), not on the MachineSet's own metadata. Querying MachineSets by that label returns nothing.Fix:
isNone()now queries Machines (not MachineSets) with-l machine.openshift.io/os-id=Windows, matching the approach used by OTP atopenshift-tests-private/test/extended/winc/utils.go:955. If Windows Machines exist, the cluster has Machine API support and the test runs. If none exist (BYOH-only / platform None), the test is skipped.OTE Test Validation (AWS e2e CI)
OCP-60814 correctly skips on PR CI (commit hash not on upstream GitHub) and passes on clusterBot with released builds.
Key Changes
SSH key removal from BeforeEach
Removed
compat_otp.GetPrivateKey()/GetPublicKey()calls and theprivateKey/publicKeypackage variables. AddedderivePublicKeyFromSecret()that reads thecloud-private-keyKubernetes secret and derives the public key usingssh.ParsePrivateKey(). This secret exists wherever WMCO is installed, making tests environment-agnostic.OCP-60814: GitHub fetch with PR build skip
Fetches
CONTAINERD_GIT_VERSIONfrom the upstream GitHub Makefile using the WMCO build commit hash. Skips withg.Skipwhen the WMCO version ends with-dirty(PR builds), since the commit hash only exists in the PR branch and not on upstream GitHub.OCP-79251: isNone queries Machines instead of MachineSets
isNone()now queries Machines with labelmachine.openshift.io/os-id=Windowsinstead of searching MachineSets by name. This correctly detects Windows machines regardless of MachineSet naming convention (e2e-wm, winworker, etc.).OCPBUGS-85061 verification in OCP-70922
Queries the
instance:node_cpu_utilisation:rate1mrecording rule viacompat_otp.NewPrometheusMonitorand asserts the value represents CPU utilization (< 0.5), not idle rate. Uses the same Prometheus access pattern as OCP-38188.New generic
execInPod()helper (WINC-1931)Replaces SSH/bastion access pattern with
oc execinto any pod. Used by OCP-33768 (prometheus pod), OCP-32554 and OCP-77777 (WMCO pod). Aligns with WINC-1931 refactoring initiative.Extracted
wmcoDeploymentvariable"deployment.apps/windows-machine-config-operator"extracted to a reusable variable, eliminating 5+ hardcoded occurrences.Simplified
truncatedVersion()with regexReplaced
truncatedVersion+removeOuterQuotes(17 lines, 2 functions) with a single 7-line regex-basedtruncatedVersion.removeOuterQuotesdeleted as unused.New utility functions (all with comments)
derivePublicKeyFromSecret- Reads cloud-private-key secret and derives SSH public keygetContainerdVersion- Node containerRuntimeVersion field parsinggetValueFromText- Line search and delimiter-based value extraction (with safe empty return)isNone- Platform None / no Windows Machines detection (queries Machines by label)extractInstanceID- JSON provider ID parsing for nodes and machineswaitUntilWMCOStatusChanged- WMCO log polling with timeoutisBYOH- BYOH label detection on nodesAdditional improvements
net.JoinHostPortfor metrics endpointg.Skipwhen < 2 Windows nodes for cross-node HTTP rejection testexecInPoderrors logged instead of silently discarded%winstead of%vinextractInstanceIDDocumentation
All 17 functions in utils.go now have one-line comments.
Test list (11 total: 6 batch 1 + 5 batch 2)
Jira
Dependencies
Stacked on Batch 1 PR #4279 - must merge first.