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

Remove sessionAffinity ClientIP from services #7748

Merged

Conversation

ejweber
Copy link
Contributor

@ejweber ejweber commented Jan 22, 2024

Which issue(s) this PR fixes:

Issue #7399

What this PR does / why we need it:

Remove sessionAffinity: ClientIP from Longhorn services. See #7399 for justification.

Special notes for your reviewer:

We could optionally introduce a Helm variable that controls the value of sessionAffinity for the Longhorn services that previously used sessionAffinity: ClientIP. If we did this, I would argue for making None the default, with the option to change back to ClientIP if desired. However:

  1. I cannot identify any situation in which we should be using sessionAffinity: ClientIP,
  2. More Helm variables makes Longhorn deployment more difficult to understand.

I'm curious whether reviewers agree.

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM.

Agreed that there is no need for having a variable to customize this property since the session affinity should be determined by the server side instead of the client side. There is no session concept in our API server.

note: The default value of sessionAffinity is None.

@ejweber
Copy link
Contributor Author

ejweber commented Jan 23, 2024

note: The default value of sessionAffinity is None.

Good catch on "" vs None. Updated the description.

Longhorn 7399

Signed-off-by: Eric Weber <eric.weber@suse.com>
@ejweber ejweber force-pushed the 7399-disable-session-affinity-client-ip branch from a136172 to 7dc1255 Compare January 23, 2024 15:19
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

LGTM

Agree that we should remove the setting completely!

Quick question. Assume that we have sessionAffinity: ClientIP set. In the first time the client connects to the backend service it is routed to manager pod with IP-1. Then manager pod with IP-1 crash, restarted, and get a new IP-2. When client connect to backend service for the 2nd time, will Kubernetes network control plane route the client to a new IP different than the IP-1 assume no manager pod has IP-1 anymore?

@ejweber
Copy link
Contributor Author

ejweber commented Jan 24, 2024

Quick question. Assume that we have sessionAffinity: ClientIP set. In the first time the client connects to the backend service it is routed to pod with IP-1. Then pod with IP-1 crash, restarted, and get new IP-2. When client connect to backend service for the 2nd time, will Kubernetes network control plane route the client to a new IP different than the IP-1 assume no manager pod has IP-1 anymore?

I think yes, though I am interested enough to test it.

From #7399 (comment):


The chain that handles load balancing (KUBE-SVC-NQL64TY2T3FWFXFQ) has two kinds of rules:

  • The first set of rules are for packets from source IPs that have already been balanced in the last configured number of seconds (10800). If, for example, the source IP is in the KUBE-SEP-W3Y4QUAIHX5U3K5C list, we jump to the KUBE-SEP-W3Y4QUAIHX5U3K5C chain.
  • The second set of rules are for packets from source IPs that have not already been balanced. Probabilities are used to ensure we have an equal chance to jump to any related chain (e.g. KUBE-SEP-W3Y4QUAIHX5U3K5C).

The event you described should cause kube-proxy to remove the list of source IPs that previously balanced to the old destination IP and the rule that would check that list (e.g. KUBE-SEP-W3Y4QUAIHX5U3K5C). The source IP is obviously not in any of the other, still existing, lists, so we would eventually fall down to the set of rules that does the load balancing and DNAT to a different IP address.

In other words, I don't think sessionAffinity: ClientIP is likely to be the cause of any previously strange connectivity behavior we may have observed.

@innobead innobead merged commit b0568e9 into longhorn:master Jan 24, 2024
4 checks passed
@innobead
Copy link
Member

@mergify backport v1.6.x v1.5.x v1.4.x

Copy link

mergify bot commented Jan 24, 2024

backport v1.6.x v1.5.x v1.4.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants