Service Topology implementation for Kubernetes - #72046
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
kind/api-change
m1093782566
changed the title
k8s-ci-robot
added
release-note
m1093782566
changed the title
k8s-ci-robot
added
size/M
|
/assign @thockin @johnbelamaric @wojtek-t |
k8s-ci-robot
added
the
kind/feature
m1093782566
changed the title
k8s-ci-robot
added
size/S
afce5bf to
13c76b5
Compare
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
I don't think this needs "get" any more
There was a problem hiding this comment.
I'm not sure this is the case anymore but I was under the impression that we generally only make additive changes to bootstrap policies since removing rules can complicate downgrade.
There was a problem hiding this comment.
I don't think it has needed "get" for a long time :)
There was a problem hiding this comment.
These changes did not get copied to the v1 types.go, which is where the actually matter.
There was a problem hiding this comment.
Nice catch. Not very familiar with the mechanism before, now I am familiar with it.
There was a problem hiding this comment.
I agree that only one is needed, and since topology keys is alpha, that's the one we should yell about. But I won't block the PR just on that.
|
All fixes have pushed and squashed, PTAL. @thockin |
thockin
left a comment
thockin
left a comment
There was a problem hiding this comment.
I'm going to approve this but I need a followup PR to do the sync trigger (or explain why not) and to fix validation.
Glad to get this in. Looking forward to the discussion of traffic policy :)
/lgtm
/approve
There was a problem hiding this comment.
I think we need to explicitly trigger a sync (in all 3 handlers), don't we? Otherwise this will be deferred until the next service or endpoint change. Or am I missing some connection? E.g.
lock
old = proxier.nodeLabels
proxier.nodeLabels = new
unlock
if old != new {
proxier.syncProxyRules()
}
There was a problem hiding this comment.
Yes, sorry It's my mistake. It seems we still need DeepEqual to compare the old and new labels, but we can compare length at first to avoid traversing when labels been added or removed.
There was a problem hiding this comment.
Same comment from iptables re triggering a sync
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Looks like there's a conflict already :( |
|
/milestone v1.17 |
|
I'll do the rebase to resolve conflicts. |
|
/retest |
There was a problem hiding this comment.
btw, this is still unresolved, can we fix in follow-up PR?
There was a problem hiding this comment.
shouldn't use new, since it's a go keyword, can we rename to newLabels
There was a problem hiding this comment.
length is already considered from reflecy.DeepEqual?
There was a problem hiding this comment.
Yes, gonna remove length comparation.
* Implement Service Topology for ipvs and iptables proxier * Add test files * API validation
|
/milestone v1.18 (code freeze v1.17) |
Can we request an exception to fit this into v1.17? @thockin @andrewsykim @johnbelamaric |
|
/milestone v1.17 I can lgtm by EOD PST if comments are addressed, otherwise we'll file an exception (we agreed to do this in today's SIG meeting) |
|
I see comments are addressed now, thanks @imroc /lgtm |
|
/retest |
|
@imroc looks like we need to update swagger docs and protobuf |
Yes, now it's updated and pushed. |
|
/lgtm |
|
/retest help wanted please. |
|
/test pull-kubernetes-e2e-gce-100-performance |
|
/retest |
|
@m1093782566: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
16 participants
What type of PR is this?
/kind api-change
/kind feature
What this PR does / why we need it:
API&Implement for feature: topology-aware service routing
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Design document of this feature is kubernetes/enhancements#640
Does this PR introduce a user-facing change?: