test(e2e): skip failing specs on openshift#2034
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Go/Ginkgo E2E test suite to mark specific specs as incompatible/flaky on OpenShift by adding the skip-on-openshift label, so they are excluded by the OpenShift E2E job’s label filter (!skip-on-openshift).
Changes:
- Added
Label("skip-on-openshift")to 10 targetedIt(...)specs that fail in the OpenShift E2E job. - Kept test logic unchanged; only Ginkgo metadata (labels) was modified to control execution in OpenShift runs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/tenant_resource_quota_exceeded_test.go | Labels the “should block new Pods” spec to be skipped on OpenShift. |
| e2e/storage_pv_cross_tenant_mount_test.go | Labels the PV cross-tenant mount prevention spec to be skipped on OpenShift. |
| e2e/rules_enforce_qos_test.go | Labels the QoS computation spec to be skipped on OpenShift. |
| e2e/rules_enforce_metadata_test.go | Labels the apiVersion/core-v1 metadata rule spec to be skipped on OpenShift. |
| e2e/replications_tenantresource_test.go | Labels the prune-enabled lifecycle spec to be skipped on OpenShift. |
| e2e/replications_globaltenantresource_test.go | Labels the cluster-scoped update-guard spec to be skipped on OpenShift. |
| e2e/performance_scalability_test.go | Labels the scalability lifecycle spec to be skipped on OpenShift. |
| e2e/namespace_metadata_forbidden_test.go | Labels the namespace update failure spec to be skipped on OpenShift. |
| e2e/customquota_namespaced_test.go | Labels the “missing quantity path” deny-create spec to be skipped on OpenShift. |
| e2e/customquota_global_test.go | Labels the global custom quota ledger settling spec to be skipped on OpenShift. |
3eb04b1 to
ad1e720
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Makefile:291
- This PR is described as only adding
skip-on-openshiftlabels to E2E specs, but it also changes dev setup targets and rewriteswait-for-helmreleaseswith a new timeout and additional diagnostics. Please either (a) split these Makefile changes into a separate PR, or (b) update the PR title/description to reflect the expanded scope so reviewers and release notes stay accurate.
# Maximum time (seconds) to wait for all HelmReleases to reconcile before failing.
# Overridable, e.g. `HELMRELEASE_WAIT_TIMEOUT=900 make wait-for-helmreleases`.
HELMRELEASE_WAIT_TIMEOUT ?= 600
.PHONY: wait-for-helmreleases
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
Makefile:291
- The PR title/description says only E2E test metadata is being updated, but this change introduces functional Makefile updates (new HELMRELEASE_WAIT_TIMEOUT + rewritten wait-for-helmreleases loop, and dev-setup target dependency changes). Please either (a) split these Makefile changes into a separate PR, or (b) update the PR title/description to explicitly include the Makefile behavior changes and their rationale.
# Maximum time (seconds) to wait for all HelmReleases to reconcile before failing.
# Overridable, e.g. `HELMRELEASE_WAIT_TIMEOUT=900 make wait-for-helmreleases`.
HELMRELEASE_WAIT_TIMEOUT ?= 600
.PHONY: wait-for-helmreleases
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
Makefile:245
- This PR is described as only updating E2E test metadata, but this change alters the developer setup workflow:
dev-setup-cert-managerno longer depends ondev-setup-fluxcd. If this dependency removal is intentional, it should be in a separate PR (or the PR description updated); otherwise it should be reverted to keep dev setup behavior unchanged.
dev-setup-cert-manager:
e2e/service_allowed_external_ips_test.go:21
- Adding
skip-on-openshiftat theDescribelevel labels all specs in this suite (including "should fail creating an evil service") asskip-on-openshift, which is broader than labeling only the failing spec(s). Consider keeping the label on the specific failingItblocks instead.
var _ = Describe("enforcing an allowed set of Service external IPs", Ordered, Label("tenant", "networking", "service", "skip-on-openshift"), func() {
e2e/service_forbidden_metadata_test.go:21
- Adding
skip-on-openshiftat theDescribelevel labels all specs in this suite asskip-on-openshift(multipleItblocks are defined under thisDescribe). If only a subset of specs are failing on OpenShift, the label should be applied only to those specificItblocks to avoid unnecessarily reducing OpenShift coverage.
var _ = Describe("creating a Service with user-specified labels and annotations", Ordered, Label("tenant", "networking", "service", "skip-on-openshift"), func() {
e2e/customquota_global_test.go:214
- Adding
skip-on-openshiftto theDescribelabel makes all specs in this suite carry that label, including specs that are not individually marked in this PR (e.g., the "marks the quota not ready..." spec further down). If the goal is to skip only the known failing specs, keepskip-on-openshifton the specific failingItblocks and avoid applying it to the wholeDescribe.
var _ = Describe("when GlobalCustomQuota uses ledger-backed reconciliation", Ordered, Label("global", "globalcustomquota", "customquota", "ledger", "skip-on-openshift"), Ordered, func() {
31aac59 to
5c75e66
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Makefile:245
- This PR is described as only updating E2E test metadata, but it also changes Makefile dev-setup target dependencies (e.g., removing
dev-setup-fluxcdprerequisite fromdev-setup-cert-managerand adding it todev-setup-capsule-example). If this is intentional, the PR description should be updated; otherwise, consider reverting these Makefile changes to keep the PR focused.
dev-setup-cert-manager:
Fix the OpenShift e2e bootstrap and skip the specs that still fail on OpenShift/MicroShift: - Drop the dev-setup-fluxcd prerequisite from dev-setup-cert-manager so the OpenShift path no longer re-applies the non-OpenShift flux manifests over the SCC-patched ones (which restored source-controller's fsGroup 1337, rejected by every OpenShift SCC and hanging the run). - Label the remaining OpenShift-failing specs with skip-on-openshift so they are excluded by the e2e-openshift label filter. Signed-off-by: Hristo Hristov <me@hhristov.info>
9ae3f0a to
dec5f4f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
Makefile:245
- The PR description says only E2E test metadata is updated, but this target change modifies developer setup behavior in the Makefile. If this change is unrelated to skipping OpenShift E2Es, it should be reverted or moved to a separate PR (and documented in the PR description if intentional).
dev-setup-cert-manager:
Makefile:264
- This PR is scoped to E2E test labeling, but this change alters dev setup ordering by forcing dev-setup-fluxcd when running dev-setup-capsule-example. If this isn’t required for the OpenShift E2E skip-label work, consider reverting/moving it to a separate PR.
dev-setup-capsule-example: dev-setup-fluxcd
e2e/customquota_global_test.go:214
- Adding
skip-on-openshiftat the Describe level will skip the entire GlobalCustomQuota ledger suite on OpenShift (this file contains 40Itspecs). If only a subset is failing, label just thoseItspecs instead so OpenShift E2E coverage isn’t unnecessarily reduced.
var _ = Describe("when GlobalCustomQuota uses ledger-backed reconciliation", Ordered, Label("global", "globalcustomquota", "customquota", "ledger", "skip-on-openshift"), Ordered, func() {
e2e/service_allowed_external_ips_test.go:21
- Moving
skip-on-openshiftto the Describe-level label skips all specs in this suite on OpenShift, includingIt("should fail creating an evil service"), which previously ran. If only the CIDR-allow tests need skipping, keep the suite label set unchanged and applyLabel("skip-on-openshift")only to the specificItblocks.
var _ = Describe("enforcing an allowed set of Service external IPs", Ordered, Label("tenant", "networking", "service", "skip-on-openshift"), func() {
e2e/service_allowed_external_ips_test.go:98
- This
Itpreviously carriedLabel("skip-on-openshift"); after moving the label to the suite, it’s now missing. If the intent is to skip only the CIDR-allow cases on OpenShift (and keep other specs running), restore the label here.
It("should allow the first CIDR block", func() {
e2e/service_allowed_external_ips_test.go:135
- This
Itpreviously carriedLabel("skip-on-openshift"); after moving the label to the suite, it’s now missing. If only the CIDR-allow tests are OpenShift-incompatible, restore the label here rather than skipping the entire suite.
It("should allow the /32 CIDR block", func() {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
e2e/customquota_namespaced_test.go:23
- Adding
skip-on-openshiftat theDescribelevel skips every spec in this suite on OpenShift (this file contains manyIt(...)blocks), which is much broader than “label the 10 failing specs”. Prefer labeling only the specific failingItcases.
var _ = Describe("when CustomQuota uses ledger-backed reconciliation", Ordered, Label("namespaced", "namespacedcustomquota", "customquota", "ledger", "skip-on-openshift"), Ordered, func() {
e2e/customquota_global_test.go:214
- Adding
skip-on-openshiftat theDescribelevel skips the entire GlobalCustomQuota ledger suite on OpenShift (dozens of specs), which appears inconsistent with the PR goal of skipping only the failing specs. Prefer labeling only the specific failingItcases.
var _ = Describe("when GlobalCustomQuota uses ledger-backed reconciliation", Ordered, Label("global", "globalcustomquota", "customquota", "ledger", "skip-on-openshift"), Ordered, func() {
e2e/service_allowed_external_ips_test.go:21
- Moving
skip-on-openshiftto theDescribelabel causes all specs in this suite to be skipped on OpenShift (including ones that were previously not skipped). If only some cases are failing, keep the label on the specificItblocks instead of the whole suite.
var _ = Describe("enforcing an allowed set of Service external IPs", Ordered, Label("tenant", "networking", "service", "skip-on-openshift"), func() {
e2e/service_allowed_external_ips_test.go:98
- This spec was previously labeled
skip-on-openshift, but the label was removed when the suite-level label was added. If the intent is to skip only the failing OpenShift specs (and not the entire suite), the label should remain on thisIt.
It("should allow the first CIDR block", func() {
e2e/service_allowed_external_ips_test.go:135
- This spec was previously labeled
skip-on-openshift, but the label was removed when the suite-level label was added. If the intent is to skip only the failing OpenShift specs (and not the entire suite), the label should remain on thisIt.
It("should allow the /32 CIDR block", func() {
Makefile:245
- The PR description says only E2E test metadata is updated, but this change also modifies Makefile dev-setup dependencies (and changes
make dev-setupbehavior by no longer implicitly runningdev-setup-fluxcd). Either split these Makefile edits into a separate PR or update the PR description/scope accordingly.
dev-setup-cert-manager:
🟢 Get the OpenShift E2E job green
Two changes: fix the OpenShift e2e bootstrap and skip the specs that still fail there.
🛠️ Bootstrap fix (
Makefile)dev-setup-cert-managerdepended ondev-setup-fluxcd(the non‑OpenShift Flux variant), which re‑applied the base manifests over the SCC‑patched ones — restoringfsGroup: 1337onsource-controller.➡️ No OpenShift SCC allows that fsGroup → pod rejected →
source-controllernever starts → cert‑managerHelmChartnever reconciles → the job hangs until the 6h ceiling.Fix: drop that prerequisite (both deps targets already install the right Flux variant first).
Full root-cause trail
FailedCreate: unable to validate against any security context constraintonsource-controller→ HelmRelease stuck atobservedGeneration < 0→wait-for-helmreleasesloops forever. Also restores thedev-setup-capsule-example: dev-setup-fluxcdprerequisite dropped by amainmerge.🚫 Skipped specs
Specs still failing on OpenShift/MicroShift are labeled
skip-on-openshift(excluded bymake e2e-openshift):