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

Refactor kube-proxy service/endpoints update so that can be consumed among different proxiers #52528

Merged
merged 6 commits into from Feb 13, 2018

Conversation

@m1093782566
Member

m1093782566 commented Sep 15, 2017

What this PR does / why we need it:

There are huge duplication among different proxiers. For example, the service/endpoints list/watch part in iptables, ipvs and windows kernel mode(to be get in soon).

I think the more places this is replicated the harder it becomes to keep correct. We may need to refactor it and let different proxiers consume the same code.

Which issue this PR fixes:

fixes #52464

Special notes for your reviewer:

  • This refactor reduces 500 Lines in iptables proxy, so it will reduce 500*N(number of proxiers) lines in total. People no need to care the service/endpoints update logic any more and can be more focus on proxy logic.

  • I would like to do the following things in follow-ups:

  1. rsync it to ipvs proxier

  2. rsync it to winkernel proxier

Release note:

Refactor kube-proxy service/endpoints update so that can be consumed among different proxiers
@m1093782566

This comment has been minimized.

Member

m1093782566 commented Sep 15, 2017

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Sep 15, 2017

@m1093782566: GitHub didn't allow me to request PR reviews from the following users: jiaxuanzhou.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jiaxuanzhou

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.

@@ -155,6 +153,66 @@ type serviceInfo struct {
serviceLBChainName utiliptables.Chain
}
func (info *serviceInfo) ClusterIP() string {

This comment has been minimized.

@jiaxuanzhou

jiaxuanzhou Sep 15, 2017

Contributor

@m1093782566 suggest to add some instructions of the funcs.

This comment has been minimized.

@m1093782566

m1093782566 Sep 15, 2017

Member

Sure, will do that.

localIPs := make(map[types.NamespacedName]sets.String)
for svcPortName := range endpointsMap {
for _, ep := range endpointsMap[svcPortName] {
if ep.IsLocal() {

This comment has been minimized.

@jiaxuanzhou

jiaxuanzhou Sep 15, 2017

Contributor

how about if !ep.IsLocal() {...continue...} ?

@thockin

This comment has been minimized.

Member

thockin commented Sep 15, 2017

There's a lot here, and it is sort of scary. I don't like the giant interface, especially since it is 1:1 (for now). I'd rather move the struct definitions over first, which should be a relatively trivial PR, and THEN consider whether we need an interface or not. Most of those types are just dumb data.

If we move them into a util lib, the comments on the now-public functions have to be REALLY good. These functions were not designed to be generic - they solve a very specific purpose in a very specific context. Making them generic means a brighter spotlight is needed on clarity, comments, and tests.

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Sep 18, 2017

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Sep 18, 2017

@thockin

I apologize that I scared you :)

Let me explain why I introduced the 2 interfaces: ServiceInfo and EndpointsInfo.

See

type proxyServiceMap map[proxy.ServicePortName]*serviceInfo
type proxyEndpointsMap map[proxy.ServicePortName][]*endpointsInfo

Currently both serviceInfo and endpointsInfo are proxy-specific, and if we keep them being proxy-specific, it's difficult to share the duplicated codes among proxiers.

I guess you may don't like defining serviceInfo and endpointsInfo in util and let all proxiers consume the same serviceInfo and endpointsInfo. Because I remember you said "When rebasing the ipvs to be the same structure as iptables, a lot of this will be different." in IPVS PR(xref https://reviewable.kubernetes.io/reviews/kubernetes/kubernetes/46580#-KrcZLOn9a9TJY6Zk1tl). So, I need a public interface.

I agree the 1:1 interface looks a bit large. So, I changed the interface definition now.

type ServiceInfo interface {
	ClusterIP() string
	Port() int
	Protocol() api.Protocol
	HealthCheckNodePort() int
}

type EndpointsInfo interface {
	Endpoint() string
	IsLocal() bool
	IPPart() string
	Equal(EndpointsInfo) bool
}

I think the structs after changing looks more clean now.

If we move them into a util lib, the comments on the now-public functions have to be REALLY good. These functions were not designed to be generic - they solve a very specific purpose in a very specific context. Making them generic means a brighter spotlight is needed on clarity, comments, and tests.

Can't agree more. I would like to add more clarity, comments, and tests when you are green at the basic idea :)

Please check the updates when you have a chance. Looking forwarding to hearing your opinions. Thanks!

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Sep 24, 2017

@wojtek-t What's your opinion?

@thockin

This is better, and overall I get it.

The utter lack of comments makes me unhappy.

The fact that it is a monolithic commit makes me unhappy, too. I feel like we've discussed this a few times.

If I were doing this, I would do something like:

  • 1 commit to define interfaces
  • 1 commit to use the interfaces in-place
  • 1 commit to move only the services code to shared place and update tests
  • 1 commit to move only endpoints code to shared place and update tests

As it is, I won't demand you break it up this way, but PLEASE for the sake of future reviews, think of your poor reviewers?

@@ -42,3 +43,22 @@ type ServicePortName struct {
func (spn ServicePortName) String() string {
return fmt.Sprintf("%s:%s", spn.NamespacedName.String(), spn.Port)
}
type ServiceInfo interface {

This comment has been minimized.

@thockin

thockin Oct 9, 2017

Member

comments comments comments, please. Types and each method.

This comment has been minimized.

@m1093782566

m1093782566 Oct 9, 2017

Member

Soooory for delay, comments are added now.

This comment has been minimized.

@m1093782566

m1093782566 Oct 9, 2017

Member

Please don't be angry about the lack of comments, I was just not sure if you would like this refactor...

This comment has been minimized.

@thockin

thockin Nov 20, 2017

Member

I still hope we can find a better name than "Info" ? Ideas? ServicePort ? ServicePortHandle ?

This comment has been minimized.

@m1093782566

m1093782566 Dec 7, 2017

Member

Yeah, ServicePort seems to be better.

It's fixed now. PTAL.

current ProxyEndpointsMap
}
type EndpointsChangeMap struct {

This comment has been minimized.

@thockin

thockin Oct 9, 2017

Member

comments comments

This comment has been minimized.

@m1093782566

m1093782566 Oct 9, 2017

Member

Sorry, comments are added now.

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels Oct 9, 2017

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Oct 9, 2017

As it is, I won't demand you break it up this way, but PLEASE for the sake of future reviews, think of your poor reviewers?

Got it. I appreciate your help.

@cmluciano

This comment has been minimized.

Member

cmluciano commented Oct 17, 2017

/retest

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 7, 2018

/hold

I am happy that this PR becomes a code review tutorial material, so I will continue to fix review comments util it looks pretty good :)

Comments are ALWAYS welcome!

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 7, 2018

/hold cancel

@thockin

I apologize I didn't let the comments on IsLocal() bool in

// IsLocal returns true if the endpoint is running in same host as kube-proxy, otherwise returns false.

Probably I lost it during rebasing.

All comments are addressed. Please take a look again :)

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 7, 2018

friendly ping @thockin for re-lgtm :)

@prameshj

I just started getting familiar with the ipvs and iptables proxier code. So, apologies if I misunderstood the implementation. I have a few comments/questions.

endpoints := current
if endpoints == nil {
endpoints = previous
}

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

nit: Check if endpoints is still nil and return, in case both previous and current were passed as nil incorrectly?

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

Make sense, previous == nil && current == nil is unexpected, and we should return false directly.

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

It's fixed now. Thanks!

type UpdateEndpointMapResult struct {
// HCEndpointsLocalIPSize maps an endpoints name to the length of its local IPs.
HCEndpointsLocalIPSize map[types.NamespacedName]int
// StaleServiceNames identifies if an endpoints service pair is stale.

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

nit: Comment should read StaleEndpoints instead of StaleServicesName.

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

nice finding, it's fixed now. Thanks!

// StaleServiceNames identifies if an endpoints service pair is stale.
StaleEndpoints map[ServiceEndpoint]bool
// StaleServiceNames identifies if a service is stale.
StaleServiceNames map[ServicePortName]bool

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

Since both StaleEndpoints and StaleServiceNames only store stale values - i.e the map value is always true, do we need them to be maps? Can they instead be arrays?
StaleEndpoints []ServiceEndpoint
StaleServiceNames []ServicePortName

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

Make sense, it's fixed now!

// is passed in to store the stale udp endpoints and `staleServiceNames` argument is passed in to store the stale udp service.
// The changes map is cleared after applying them.
func (endpointsMap EndpointsMap) apply(changes *EndpointChangeTracker, staleEndpoints map[ServiceEndpoint]bool, staleServiceNames map[ServicePortName]bool) {
changes.lock.Lock()

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

nit: Check if 'changes' is nil?

This comment has been minimized.

@m1093782566
// GetLocalEndpointIPs returns endpoints IPs if given endpoints is local - local means the endpoint is running in same host as kube-proxy.
func GetLocalEndpointIPs(endpointsMap EndpointsMap) map[types.NamespacedName]sets.String {
localIPs := make(map[types.NamespacedName]sets.String)
for svcPortName := range endpointsMap {

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

nit: can also do:
for svcPortName, epList := range endpointsMap
and use epList in line 198

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

It's fixed now.

current ServiceMap
}
// ServiceChangeTracker carries state about uncommitted changes to an arbitrary number of

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

Could you explain why this says "uncommitted changes"? I thought serviceChange holds previous and current mapping after changes have been applied. I think I am missing something.

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member
// serviceChange contains all changes to services that happened since proxy rules were synced.  For a single object,
// changes are accumulated, i.e. previous is state from before applying the changes,
// current is state after applying all of the changes.
type serviceChange struct {
	previous ServiceMap
	current  ServiceMap
}

So, I think the word "uncommitted changes" is accurate?

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

ok, I think i get it now.

svc := current
if svc == nil {
svc = previous
}

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

nit: maybe check if svc is still nil, in case previous and current were incorrectly passed in as nil?

This comment has been minimized.

@m1093782566
// - A updated to be {{"ns", "cluster-ip", "http"}: {"172.16.55.10", 1234, "TCP"}}
// - produce string set {"ns/cluster-ip:http"}
func (sm *ServiceMap) merge(other ServiceMap) sets.String {
// existingPorts is going to store all the unique newly merged services' identifier.

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

looks like existingPorts contains identifiers of all services in 'others' map, not just the newly merged ones right? Should we fix the comment?

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

Nice finding, it's fixed now.

}
// filter filters out elements from ServiceMap base on given ports string sets.
func (sm *ServiceMap) filter(ports sets.String) {

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

Any reason we can't pass a ServiceMap to the filter function? Like - func (sm *ServiceMap) filter(other *ServiceMap).
If so, we can use that in line 153 as change.previous.filter(changes.current)
Then we don't need this existingPorts set to be returned by merge().

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

Good point, we can clean up it.

It's fixed now. PTAL. Thanks!

}
delete(*sm, svcPortName)
} else {
glog.Errorf("Service port %q removed, but doesn't exists", svcPortName)

This comment has been minimized.

@prameshj

prameshj Feb 9, 2018

Contributor

Looks like service port is not being removed in this case, maybe change the log to say "Service port %q does not exist"?

This comment has been minimized.

@m1093782566

m1093782566 Feb 9, 2018

Member

make sense, it's fixed now.

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 9, 2018

@prameshj

Thanks for so many good points, it's fixed now. PTAL. Thanks!

@rramkumar1 rramkumar1 referenced this pull request Feb 9, 2018

Closed

IPVS dev work #59662

19 of 19 tasks complete
@prameshj

This comment has been minimized.

Contributor

prameshj commented Feb 9, 2018

Thanks for addressing all the comments!

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 10, 2018

Kindly ping @thockin for re-lgtm.

I am going to be out of office for Chinese Spring Festival. Given the big changes, I think it's better to soak it in test earlier.

@rramkumar1

Would you try lgtm :)

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 13, 2018

I am going to self lgtm to respect code freeze - I will be back to office in 1 or 2 weeks

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

@m1093782566: you cannot LGTM your own PR.

In response to this:

I am going to self lgtm to respect code freeze - I will be back to office in 1 or 2 weeks

/lgtm

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.

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 13, 2018

Well...

@thockin

This comment has been minimized.

Member

thockin commented Feb 13, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 13, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1093782566, thockin

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 OWNERS Files:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 9438e14 into kubernetes:master Feb 13, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation m1093782566 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@prameshj prameshj referenced this pull request Nov 15, 2018

Closed

REQUEST: New membership for prameshj #243

4 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment