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

test: demote service ClientIP affinity timeout tests from conformance #112806

Merged
merged 1 commit into from Oct 11, 2022
Merged

test: demote service ClientIP affinity timeout tests from conformance #112806

merged 1 commit into from Oct 11, 2022
+2 −55

Conversation

dcbw
Copy link
Member

@dcbw dcbw commented Sep 30, 2022 •
edited

During the September 29th, 2022 SIG-Network meeting we decided to demote the two affinity timeout conformance tests. This was because:

  • there is no documented correct behavior for these tests other than "what kube-proxy does"
  • even the kube-proxy behavior differs depending on the backend implementation of iptables, IPVS, or [win]userspace (and winkernel doesn't at all)
  • iptables uses only srcip matching, while userspace and IPVS use srcip+srcport
  • IPVS and iptables have different minimum timeouts and we had to hack up the test itself to make IPVS pass
  • popular 3rd party network plugins also vary in their implementation

Our plan is to deprecate the current affinity options and re-add specific options for various behaviors so it's clear exactly what plugins support and which behavior (if any) we want to require for conformance in the future.

/kind cleanup

Service session affinity timeout tests are no longer required for Kubernetes network plugin conformance due to variations in existing implementations. New conformance tests will be developed to better express conformance in future releases.

@thockin @danwinship @aojea @kubernetes/sig-network-misc

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/network Categorizes an issue or PR as relevant to SIG Network. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 30, 2022
@k8s-ci-robot
Copy link
Contributor

@dcbw: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 30, 2022
@k8s-ci-robot k8s-ci-robot added area/conformance Issues or PRs related to kubernetes conformance tests area/test sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 30, 2022
@dcbw
Copy link
Member Author

dcbw commented Sep 30, 2022

/label area/conformance

@k8s-ci-robot
Copy link
Contributor

@dcbw: The label(s) /label area/conformance cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, official-cve-feed

In response to this:

/label area/conformance

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.

@dcbw
Copy link
Member Author

dcbw commented Sep 30, 2022

/retest

@aojea
Copy link
Member

aojea commented Oct 1, 2022 •
edited

I think that session affinity per client IP is clear, same client IP goes to same backend, if you set a timeout, you recompute and loose the affinity.

ipvs has a limitation and we relaxed the condition for the test

kubernetes/test/e2e/network/service.go

Lines 3403 to 3408 in cf6a086

// session affinity timeout must be greater than 120 in ipvs mode,
// because IPVS module has a hardcoded TIME_WAIT timeout of 120s,
// and that value can't be sysctl'ed now.
// Ref: https://github.com/torvalds/linux/blob/master/net/netfilter/ipvs/ip_vs_proto_tcp.c
// TODO: remove this to speed up testing when IPVS does really respect session affinity timeout
svcSessionAffinityTimeout = int32(125)

However, I understand that it may be hard to implement and Conformance may be a high bar, but affinity was always a common feature on all load balancers

https://www.haproxy.com/blog/load-balancing-affinity-persistence-sticky-sessions-what-you-need-to-know/
https://techdocs.f5.com/en-us/bigip-14-0-0/big-ip-local-traffic-manager-implementations-14-0-0/configuring-http-load-balancing-with-source-address-affinity-persistence.html

@aojea
Copy link
Member

aojea commented Oct 2, 2022

Sorry, I misread it, this is about the timeout option, I fully agree, a conformance test should not be doing assumption on the backend

/lgtm

/assign @johnbelamaric @smarterclayton

For conformance approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2022
@dcbw
Copy link
Member Author

dcbw commented Oct 4, 2022

@johnbelamaric what do you think?

@dcbw
test: demote service ClientIP affinity timeout tests from conformance
1687916 
During the September 29th, 2022 SIG-Network meeting we decided to
demote the two affinity timeout conformance tests. This was because:

(a) there is no documented correct behavior for these tests other than
"what kube-proxy does"
(b) even the kube-proxy behavior differs depending on the backend implementation
of iptables, IPVS, or [win]userspace (and winkernel doesn't at all)
(c) iptables uses only srcip matching, while userspace and IPVS use srcip+srcport
(d) IPVS and iptables have different minimum timeouts and we had
to hack up the test itself to make IPVS pass
(e) popular 3rd party network plugins also vary in their implementation

Our plan is to deprecate the current affinity options and re-add specific
options for various behaviors so it's clear exactly what plugins support
and which behavior (if any) we want to require for conformance in the future.

Signed-off-by: Dan Williams <dcbw@redhat.com>
@dcbw dcbw force-pushed the demote-service-affinity-timeout branch from fd20d89 to 1687916 Compare October 5, 2022 14:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022
@danwinship
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022
@johnbelamaric
Copy link
Member

In general, demoting tests isn't too controversial, at least it won't cause problems for any existing distros. Theoretically it could cause problems for users, but given your statements regarding the level of inconsistency in existing implementations, I think that's not an issue.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, johnbelamaric

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
@dcbw
Copy link
Member Author

dcbw commented Oct 11, 2022

/retest

@k8s-ci-robot k8s-ci-robot merged commit c160266 into kubernetes:master Oct 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 11, 2022
@dcbw dcbw mentioned this pull request Oct 11, 2022
Merged
This was referenced Feb 3, 2023
Closed
Merged
k8s-ci-robot added a commit that referenced this pull request Apr 4, 2023
@k8s-ci-robot
Merge pull request #115510 from abhat/automated-cherry-pick-of-#11280…
cfda19f 
…6-upstream-release-1.24

Automated cherry pick of #112806: test: demote service ClientIP affinity timeout tests from
k8s-ci-robot added a commit that referenced this pull request Apr 4, 2023
@k8s-ci-robot
Merge pull request #112990 from dcbw/automated-cherry-pick-of-#112806…
5da8c2d 
…-upstream-release-1.25

Automated cherry pick of #112806: test: demote service ClientIP affinity timeout tests from
@danwinship danwinship mentioned this pull request Jun 29, 2023
Merged
@aojea aojea mentioned this pull request Dec 22, 2023
Merged
@danwinship danwinship mentioned this pull request Jan 3, 2024
Open
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@danwinship danwinship

@smarterclayton smarterclayton

@aojea aojea

@johnbelamaric johnbelamaric

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dcbw @k8s-ci-robot @aojea @danwinship @johnbelamaric @smarterclayton