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.

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

[WIP do not merge] Kube-Proxy Library: Breakout a KPNG-like interface "kube proxy lib" from #2104 #3649

Closed
wants to merge 19 commits into from

Conversation

jayunit100
Copy link
Member

Break out KEP from #2104

mcluseau and others added 8 commits September 24, 2022 00:36
Adding details about how proxies work w/ and w/o the fullstate

initial wave of fixes from dan's review

updaedd diffstore explanation and typo fix, moved it

updates to userstories and fixes to DiffState bundling logic

removed exsiting Run() logic,

Fix some spelling and typos

Add section on the different types
of client sinks.

Signed-off-by: astoycos <astoycos@redhat.com>

Add note about separate node kpng-brain and its fragility as a real feature

Update README.md

Refactoring Kube-Proxy, mega-commit
Co-authored-by: mcluseau <mikael.cluseau@gmail.com>
Co-authored-by: jayunit100 <jvyas@vmware.com>
Co-authored-by: kbabben <knabben@vmware.com>
Co-authored-by: rajas kakodkar <rkakodkar@vmware.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. labels Nov 8, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jayunit100
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval by writing /assign @thockin in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2022
@jayunit100 jayunit100 changed the title Kube-Proxy Library: Breakout a KPNG-like interface "kube proxy lib" from #2104 [WIP do not merge] Kube-Proxy Library: Breakout a KPNG-like interface "kube proxy lib" from #2104 Nov 8, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2022
@astoycos
Copy link

/cc

should be approved by the remaining approvers and/or the owning SIG (or
SIG Architecture for cross-cutting KEPs).
-->
# KEP-2104: kube-proxy library (KEP-2104, breakout)
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll probably want a new KEP number for this

Copy link

Choose a reason for hiding this comment

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

Fixing....

Copy link

Choose a reason for hiding this comment

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

I guess we'd need a new issue for this right @jayunit100 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

ya, i guess for the last thing we do we can clean that ... lets get consensus on the impl first tho i think b4 we gift wrap that part?

keps/sig-network/2104-kube-proxy-lib/README.md Outdated Show resolved Hide resolved
keps/sig-network/2104-kube-proxy-lib/README.md Outdated Show resolved Hide resolved
keps/sig-network/2104-kube-proxy-lib/README.md Outdated Show resolved Hide resolved
keps/sig-network/2104-kube-proxy-lib/README.md Outdated Show resolved Hide resolved

#### Story 2

As a Kubernetes maintainer, I don't want to have to understand the internals of a networking backend in order to simulate or write core updates to the logic of the kube-proxy locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fairly small use case. In fact, it's pretty much limited to topology-related features, where all you care about is restricting the endpoints that get used on a given node. For just about anything else, any change to the core is going to need to be matched with new code in the backends as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, updated as a svc proxy maint , pushing shortly

- Exemplify the use of such a repository in a mock "backend" which uses the repository to process and respond to changes in the Kubernetes networking state.
- Define a policy around versioning and releasing of "kube-proxy-lib".
- Create a CI system that runs in test-grid which tests kube-proxy-lib compatibility with the latest Kubernetes releases continuously, and which can be used to inform API changes for regressions that break "kube-proxy-lib".
- Enable the eventual *replacement* of the k/k/pkg/proxy serviceChangeTracker and endpointsChangeTracker related caching structures inside of in-tree kube proxy with this generic library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe point out that the existing code has hard-to-fix bugs. Eg, kubernetes/kubernetes#112604

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 198 to 200
- sending the full-state could be resource consuming on big clusters, but it should still be O(1) to
the actual kernel definitions (the complexity of what the node has to handle cannot be reduced
without losing functionality or correctness).
Copy link
Contributor

Choose a reason for hiding this comment

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

It is completely unclear what this means, in the context of this KEP.

Copy link
Member Author

Choose a reason for hiding this comment

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

deleting


No risks, because we arent removing the in tree proxy as part of this repo, but rather, proposing a library for kube proxy extenders
to use optionally. There may be risks, eventually, if we write another KEP to *replatform* sig-networks k/k/pkg/proxy implementations
on top of this library, but that would be in a separate KEP.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes sense for that to be a separate KEP. IMO this project is only interesting for SIG Network if it is going to replace our existing shared proxy code. (Yes, the code would be useful for third parties even if SIG Network wasn't using it in the k8s core, but it wouldn't make sense for SIG Network to own the code in that case; it should just be a third-party project.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point but, in the interest of "building something that is uncontraversial as a first step", How strongly do you feel about this ? If i made this argument:

  • "sig network needs b"
  • "b needs a"
  • thus were going to make "sig-network/a" as a stepping stone to us being able to build "sig-network/b"
  • by the way "sig-network/b" is somethin lots of people want, so even if we get hit by a bus after making "b", and we never get around to "a", we still have a useful intermediate deliverable....

Is that digestible ? or does it sit funny w/ you ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is that I don't think the library can be definitely "uncontroversial" and "a useful intermediate deliverable" unless we know that it is suited to replacing pkg/proxy.

I'm not saying we have to deliver the library and the updated iptables proxy in the same release, I'm saying that rather than doing:

  1. come up with a plan for the library
  2. implement the library
  3. come up with a plan to replace pkg/proxy with the new library
  4. replace pkg/proxy with the new library

we need to do

  1. come up with a plan for the library
  2. come up with a plan to replace pkg/proxy with the new library
  3. implement the library
  4. replace pkg/proxy with the new library

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, ok updated as a risk (and updated the goals to reflect)


###### Will enabling / using this feature result in introducing new API types?

Yes. There will be an in-memory API that is supported by this library, that is incremented overr time
Copy link
Contributor

Choose a reason for hiding this comment

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

No. The question is about kubernetes API types, not about golang APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, added "No" + clarified

@astoycos
Copy link

astoycos commented Dec 2, 2022

/assign

jayunit100 and others added 3 commits December 6, 2022 10:17
Co-authored-by: Dan Winship <danwinship@redhat.com>
Co-authored-by: Dan Winship <danwinship@redhat.com>
Co-authored-by: Dan Winship <danwinship@redhat.com>
jayunit100 and others added 3 commits December 6, 2022 10:18
@aojea
Copy link
Member

aojea commented Dec 8, 2022

/cc

@jayunit100
Copy link
Member Author

@astoycos I'd say get folks to agree on the content then fix the details like issue and kep number after... your call tho

@astoycos
Copy link

astoycos commented Dec 8, 2022

+1 @jayunit100 I'm running through Dan's comments that are still open and fixing some small stuff I should have a commit up by tonight then I'll keep pushing others on if the context makes sense

@jayunit100
Copy link
Member Author

feel free to YOLO push @astoycos thanks !

This update provides some fixes and review
comment resolutions from Dan.  It also
clarifies where and what the proposed library
would look like and the role it would initially
serve for existing proxies.

Also fix verification tests :)

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos
Copy link

astoycos commented Dec 9, 2022

/assign

@astoycos
Copy link

astoycos commented Dec 9, 2022

Pushed some updates based on @danwinship comments and the discussion from the kpng meeting this morning

@astoycos
Copy link

/close

Closed in favor of #3788

@k8s-ci-robot
Copy link
Contributor

@astoycos: Closed this PR.

In response to this:

/close

Closed in favor of #3788

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants