OCPBUGS-114882: drop GCD health-check ranges in openshift-gcp-routes - #6501
patrickdillon 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 |
openshift-ci
Bot
added
the
approved
|
For some reason the payload job failed to load credentials. I'm not sure if there is an issue running payload jobs with gcd? will check it out, instead we should be able to test like this: /testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview openshift/installer#10871 |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview openshift/installer#10871 |
Ei.. looks like a widespread issue when running periodic job against PR. I see the same issue in openshift/installer#10872 (comment) too. |
|
Let's use an installer PR against /testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview openshift/installer#10872 |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview openshift/installer#10872 |
|
/test unit verify-deps |
Thanks! I have PRs open against 4.22 & main. Lost track of which I was working on 😅 |
|
/test unit perscale failure due to infra, git clone issue |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Looking at gather-extra machineconfigs.json, we can see the content of /opt/libexec/openshift-gcp-routes.sh contains health check IPs for u-germany-northeast1:
add rule ip ${TABLE_NAME} forward ip saddr 34.3.144.0/23 drop
add rule ip ${TABLE_NAME} forward ip saddr 34.3.151.0/26 drop
add rule ip ${TABLE_NAME} forward ip saddr 34.3.151.64/26 drop
add rule ip ${TABLE_NAME} forward ip saddr 136.124.104.0/22 drop
add rule ip ${TABLE_NAME} forward ip saddr 136.124.108.0/22 drop
I also cross-ref with installer-code and the following docs:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, tthvo, 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 |
|
/verified by e2e testwith shows install complete and |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by 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. |
|
/test unit |
|
/test unit |
1 similar comment
|
/test unit |
|
@patrickdillon: 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. |
|
@yuqi-zhang wdyt about the frequently failing unit tests? I had chai bot look into it and it came up with: AFAICT it is unrelated to my pr... |
|
/override ci/prow/unit It does seem unrelated and hopefully we are good to merge here. |
|
@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/unit 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. |
|
/skip |
|
/refresh |
|
/test e2e-aws-ovn e2e-aws-ovn-upgrade Would this work to get them out of "stuck" state 👀? |
4 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