Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Query NEGs from Svcneg when linking BackendServices. #2615

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Query NEGs from Svcneg when linking BackendServices. #2615

merged 2 commits into from
Sep 17, 2024

Conversation

sawsa307
Copy link
Contributor

  • Query NEGs from Svcneg Status, and only do GCE query as a fallback.

/assign @swetharepakula

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2024
@sawsa307 sawsa307 changed the title Query NEGs from Svcneg. Query NEGs from Svcneg when linking BackendServices. Jul 30, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2024
@sawsa307
Copy link
Contributor Author

/cc @mmamczur we are updating NEG linking logic, fyi

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2024
@sawsa307 sawsa307 force-pushed the link-neg-from-svcneg branch 5 times, most recently from 0dd1f1f to 3c57dea Compare August 6, 2024 00:03
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2024
@sawsa307
Copy link
Contributor Author

/retest

@gauravkghildiyal
Copy link
Member

/assign

@sawsa307
Query NEGs from Svcneg when linking BackendServices
1754ea3 
* Query NEGs from Svcneg Status, and only do GCE query as a fallback.
* If multi-subnet cluster is enabled, we will only link NEGs from
  the default subnet. We will try to add all NEGs once CRD is available.
gauravkghildiyal
gauravkghildiyal reviewed Sep 17, 2024
View reviewed changes
Copy link
Member

@gauravkghildiyal gauravkghildiyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sawsa307. This all looks good to me.

Just one minor suggestion for test.

/lgtm
/hold (If you want approval from Swetha)

pkg/backends/neg_linker.go Show resolved Hide resolved
pkg/backends/neg_linker_test.go
if err := linker.Link(svcPort, shrinkZone); err != nil {
t.Fatalf("Failed to link backend service to NEG for svcPort %v: %v", svcPort, err)
if err := linker.Link(tc.svcPort, shrinkZone); err != nil {
t.Fatalf("Failed to link backend service to NEG for svcPort %v when populateSvcNeg = %v: %v", tc.svcPort, populateSvcNeg, err)
}

validate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this second validate() is trying to ensure that even if the Zones shrink, we don't remove the NEG from the BackendService.

With the new way of using SvcNEG for finding all the NEGs from all the zones, the previous "mimic'ing" part is not sufficiently "mimic'ing" the zone contraction. In addition to passing only zones[0], you may also need to update the existing SvcNEG with only one NegObjectReference, before calling this second validate().

(NOTE: I think it's also true that SvcNEGs would never be updated with such a zone contraction, but it's still nice to verify this behavior independently here)

Copy link
Contributor Author

@sawsa307 sawsa307 Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much Gaurav for pointing this out! It is a very subtle point that I haven't thought about.
We do need to update existing SvcNEG, but instead of remove one of the NegObjectReference, we should instead mark it as Inactive since this will match the expected behavior by NEG controller(Detailed in #2604, my apology that I haven't thought about this dependency).

In short, when NEG controller no longer observes nodes in a specific zone, it will mark the NEG ref in that zone as Inactive. This is to retain the NEG ref so we won't miss any NEGs during GC().

As a side effect, it would also introduce a small behavior change during zone contract. Previously/currently, we won't include NEGs from contracted zone in the Backend Service. With this PR, since we include NEGs in both Active and Inactive state, NEGs from contracted zone will present in the Backend Service. However, there shouldn't be any effect/issue since this situation is simply equivalent to the current behavior when there are nodes but no endpoints/pods in a specific zone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

>Previously/currently, we won't include NEGs from contracted zone in the Backend Service.

Actually I think this is not the case, which means I don't think there's a behavior change -- even better :)

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 17, 2024
@sawsa307
Add SvcNeg to neg_linker test cases.
1f60070 
* Fixed an bug where svcNegLister.Add() return error is not check.
  Malformed SvcNeg struct results in an error during Add(), so there is
  no reference of SvcNeg in cache, and the test case isn't testing the
  expected behavior(querying NEG link from SvcNeg).
* Add SvcNeg CR to all neg_linker test cases.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2024
@gauravkghildiyal
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, sawsa307

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sawsa307
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2024
@sawsa307
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 0a04c98 into kubernetes:master Sep 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea Awaiting requested review from aojea

@gauravkghildiyal gauravkghildiyal Awaiting requested review from gauravkghildiyal

Assignees

@swetharepakula swetharepakula

@gauravkghildiyal gauravkghildiyal

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sawsa307 @gauravkghildiyal @k8s-ci-robot @swetharepakula