OCPBUGS-114882: drop GCD health-check ranges in openshift-gcp-routes - #6501
OCPBUGS-114882: drop GCD health-check ranges in openshift-gcp-routes#6501patrickdillon wants to merge 3 commits into
Conversation
openshift-gcp-routes drops load balancer health-check probes that would be forwarded off-node, to avoid polluting conntrack (BZ 1925698, 1930457). The dropped source ranges were hardcoded to the public GCP prober ranges (35.191.0.0/16, 130.211.0.0/22). GCD (sovereign) regions probe from different ranges which need to be added to avoid the issue seen on public GCP. Select the dropped ranges by region via a new gcpHealthCheckSourceRanges template function: GCD regions drop their own region's prober ranges, all other regions keep the public GCP behavior. Only ranges reserved for health-check probers in the target environment are dropped -- a range that is reserved prober infrastructure in one environment is ordinary routable space in another, so the ranges must not be dropped unconditionally.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-114882, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (55)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe change adds region-specific GCP health-check source ranges, exposes the selector to templates, and renders nftables rules for each selected range. ChangesGCP health-check range handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is mergeable with normal checks; no actionable regression is identified. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-114882, which is valid. 3 validation(s) were run on this bug
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. |
|
/payload-job periodic-ci-openshift-release-main-ci-5.1-e2e-gcd-ovn |
|
@patrickdillon: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6f8dfd30-a85c-11f1-813b-44d8bf3c86c6-0 |
|
/test unit unrelated failure looks like a flake |
|
/payload-job periodic-ci-openshift-release-main-ci-5.1-e2e-gcd-ovn |
|
@patrickdillon: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/188a0340-a928-11f1-9980-69e9442188d8-0 |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Logic itself seems fine to me, a couple of questions inline on the details
Both test runs failed with
ERROR: gcloud crashed (ConnectionError): HTTPSConnectionPool(host='compute.apis-berlin-build0.goog', port=443): Max retries exceeded with url: /compute/v1/projects/eu0%3Aopenshift/regions/u-germany-northeast1?alt=json (Caused by NameResolutionError("HTTPSConnection(host='compute.apis-berlin-build0.goog', port=443): Failed to resolve 'compute.apis-berlin-build0.goog' ([Errno -2] Name or service not known)"))
it looks like. Is that a test configuration issue?
| "34.3.144.0/23", | ||
| "34.3.151.0/26", | ||
| "34.3.151.64/26", | ||
| "136.124.104.0/22", |
There was a problem hiding this comment.
This and the following entry are listed for both germany and france, are they actually regional? Or global? (Doesn't affect the implementation I think but just curious)
There was a problem hiding this comment.
Hm yes apparently both of these ranges are included in both S3NS france docs and the berlin region, the docs for which are not publicy available.
These are both for external passthrough load balancers so, yes they are global, although distinct from the gcp ranges.
|
|
||
| // gcpHealthCheckSourceRanges returns the health-check prober source ranges to | ||
| // drop for the cluster's region. Only GCD regions have specific ranges. | ||
| func gcpHealthCheckSourceRanges(cfg RenderConfig) []string { |
There was a problem hiding this comment.
Does https://github.com/openshift/api/blob/6733660e6ece5593234b36fd21327c4c8ec96db2/config/v1/types_infrastructure.go#L854 UniverseDomain intersect with this at all? Also I noticed that in the MCO vendored code, the featuregate for soverign cloud on GCP https://github.com/openshift/machine-config-operator/blob/main/vendor/github.com/openshift/api/features/features.go#L880 is still dev preview. Do you need a vendor bump?
There was a problem hiding this comment.
Does https://github.com/openshift/api/blob/6733660e6ece5593234b36fd21327c4c8ec96db2/config/v1/types_infrastructure.go#L854 UniverseDomain intersect with this at all?
Yes, it does intersect, in the sense that these GCD sovereign cloud regions have alternate universe domains. Most of the implementation for this feature has been updating clients to use an alternate universe domain when talking to GCD.
I'm not sure how strict the coupling is between GCD and alternate universe domains, it seems possible there will be uses of alternate universe domains separate from GCD.
There was a problem hiding this comment.
Also I noticed that in the MCO vendored code, the featuregate for soverign cloud on GCP https://github.com/openshift/machine-config-operator/blob/main/vendor/github.com/openshift/api/features/features.go#L880 is still dev preview. Do you need a vendor bump?
Maybe? I have not used feature gates in MCO before, and I wasn't aware it needed to be explicitly vendored in to be updated (the installer is like that too, but most components AFAIK do not need vendoring...)
On the other hand, nothing in the code is currently gated and the gated API field is not being used in this case, so it doesn't seem strictly necessary. That said, it seems like a good idea to have it up to date, so I can include a bump here.
There was a problem hiding this comment.
Bumped openshift/api for good measure
|
/payload-job periodic-ci-openshift-release-main-ci-5.1-e2e-gcd-ovn |
|
@patrickdillon: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cf9eedf0-adf8-11f1-8e40-8f7809ae87d5-0 |
|
oops I screwed up the vendoring. I mistakenly pulled in release-5.0 when we need 5.1/main |
go get github.com/openshift/api@master && go mod tidy
go mod vendor To pull in GCD feature gate in default feature set.
3dc97ea to
5acd9d6
Compare
|
ok vendoring is fixed up now. |
I'm rerunning the test, I think it's along those lines, but the issue is more on the build cluster side. It looks related to the build04 cluster issues we're seeing in this bug/incident: https://redhat.atlassian.net/browse/OCPBUGS-105502 |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
/lgtm
Leaving an approval from the MCO side, thanks for addressing the questions. Would like to see a green run and/or some other way to validate the actual implementation, but logically should be safe to merge
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@patrickdillon: The following tests 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. |
3 participants
- What I did
Updated the pattern introduced by the bug fixes for https://bugzilla.redhat.com/show_bug.cgi?id=1925698 and https://bugzilla.redhat.com/show_bug.cgi?id=1930457 to take into account the health probe ranges for GCD sovereign regions.
- How to verify it
We have gcd e2e jobs, and I will kick one off, but I'm not certain the file in question would be gathered in CI artifacts. If not, I will manually verify (and update here of course).
- Description for the changelog
Include GCD health-check probe ranges in openshift-gcp-routes.
Summary by CodeRabbit
New Features
Bug Fixes