GEP-1619: Refactor SessionPersistencePolicy into BackendLBPolicy

[gcs278]

What type of PR is this?
/kind gep

Optionally add one or more of the following kinds if applicable:
N/A

What this PR does / why we need it:

The main goal of this PR is to get GEP-1619 ready for experimental by answering the open questions in Graduation Criteria for Implementable Status:

1. Should we leave room in this policy to add additional concepts in the future such as Session Affinity? If so, how would we adjust the naming and overall scope of this policy?
2. Should we leave room for configuring different forms of Session Persistence? If so, what would that look like?
   It also has variety of other clean up and clarification work.

The PR currently does the following (broken in to commits):

1. Fix links, formatting, metadata.yaml
2. Add new implementations with their session persistence APIs
3. Answer question on forms of Session Persistence
4. Distinguish between session vs. persistent cookies (see GEP 1619: distinguish 'session' cookies. #2747)
5. Update timeouts to use Duration field
6. Adopt RFC8174 language
7. Answer open question about unhealthy backends
8. Refactor the API from SessionPersistencePolicy into BackendLBPolicy and attach only to services, and introduce a new inline route API

Which issue(s) this PR fixes:
#2747
#1778 (Doesn't fix, but is related because LoadBalancerPolicy is the framework for adding algorithm selection)

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[youngnick]

Once #2689 merges, this will need a rebase and update - the GEP files have moved.

[gcs278]

@ginayeh @costinm @robscott Next round of updates (diff):

• Updated title of GEP
• Changed name from BackendLoadBalancePolicy to BackendLBPolicy
• Made the route API common with BackendLBPolicy
• Added a clause to route API to basically say, it's not done, reevaluate and added another Implementable criteria for this
• Changed Timeouts to Extended and use Duration field (added as new commit)

[ginayeh]

Idealy, we should have all backend services enable session persistence in traffic splitting scenarios. However, I want to get some clarity on this traffic splitting scenario of one service (svc1) has persistence enabled while the other (svc2, svc3) does not. In this scenario, if we keep session persistence only applied to svc1, all traffic will eventually goes to svc1 because the persistence should be maintained, resulting in traffic overloading svc1 and may break the system. A more reasonable interpretation of this is that session persistence MUST be applied to all backends (svc1, svc2, svc3) involved in traffic splitting if any of them have session persistence enabled.

[gcs278]

I think you have a good point that it may be much more practical to apply session persistence to all backends given one backend has session persistence configured, but like @costinm mentioned, I think it might be too prescriptive to say it must be done that way.

Do you see any problem with leaving the spec open for implementations to do both?:

1. Maintain only session persistence to svc1 (eventually transitioning all traffic to svc1, possibly overloading it)
2. Apply session persistence to all svc1-3

[ginayeh]

Let's leave the spec open and have implementations to decide which behavior they want since it's an edge case.

[gcs278]

Done in a new commit: 13eaa83

[costinm]

I can confirm that at least in CloudRun session persistence ( which has been around for quite some time), the persistence can be enabled on svc1 while others (svc2, svc3 ) do not have it enabled. I don't think we can impose a MUST on the opposite behavior - probably other implementations follow the same model. Applying persistence on svc2 and svc3 may also break them - because the traffic distribution will be different from what a backend not using persistent sessions expect ( as well as draining, etc). Traffic may move to svc1 - which is intended behavior if user is transitioning to a new version that is using session persistence, and should not cause any problems with auto-scalling, Or if user is moving from persistent session to stateless - it is possible to set svc1 with a weight=0, and no new request will go to it - so use can transition to svc2. I don't mind if we allow some implementations to treat one backend having persistence as all backends having it - migration is a transitory thing and pretty rare. Also that's the current Istio behavior. So far we are not aware of any customers having problems with either approach.

…

On Thu, Mar 7, 2024 at 12:55 PM Gina Yeh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In geps/gep-1619/index.md <#2634 (comment)> : > When using multiple backends in traffic splitting, all backend services should have session persistence enabled. -Nonetheless, implementations should also support traffic splitting scenarios in which one service has persistence +Nonetheless, implementations MUST also support traffic splitting scenarios in which one service has persistence enabled while the other does not. This support is necessary, particularly in scenarios where users are transitioning to or from an implementation version designed with or without persistence. Idealy, we should have all backend services enable session persistence in traffic splitting scenarios. However, I want to get some clarity on this traffic splitting scenario of one service (svc1) has persistence enabled while the other (svc2, svc3) does not. In this scenario, if we keep session persistence only applied to svc1, all traffic will eventually goes to svc1 because the persistence should be maintained, resulting in traffic overloading svc1 and may break the system. A more reasonable interpretation of this is that session persistence MUST be applied to all backends (svc1, svc2, svc3) involved in traffic splitting if any of them have session persistence enabled. — Reply to this email directly, view it on GitHub <#2634 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SONUZA3WP4XL43QF3YXDH4DAVCNFSM6AAAAABADPMBPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRTGUYTMMZVGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[costinm]

The reason svc1 would be overloaded is if it doesn't use auto-scaling. High traffic can overload any service that doesn't correctly auto-scale or shedds load. Load balancers can (should) also use load reporting and track capacity of a backend - regardless of traffic splitting weights a properly configured workload with a good load balancer should never overload - new sessions should not be sent to an endpoint that is at capacity. Without sessions - if you have a 10%->v1 90%->v2 split but v1 is under-sized and can only handle 5% - I would expect a load-aware LB to send 5% and prioritize the health/reliability of the system over the API. At some point we may need to discuss load shedding ( not in this GEP ), which is different from draining and another thing that may make an LB ignore the split settings.

…

On Thu, Mar 7, 2024 at 5:18 PM Grant Spence ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In geps/gep-1619/index.md <#2634 (comment)> : > When using multiple backends in traffic splitting, all backend services should have session persistence enabled. -Nonetheless, implementations should also support traffic splitting scenarios in which one service has persistence +Nonetheless, implementations MUST also support traffic splitting scenarios in which one service has persistence enabled while the other does not. This support is necessary, particularly in scenarios where users are transitioning to or from an implementation version designed with or without persistence. I think you have a good point that it may be much more practical to apply session persistence to all backends given one backend has session persistence configured, but like @costinm <https://github.com/costinm> mentioned, I think it might be too prescriptive to say it must be done that way. Do you see any problem with leaving the spec open for implementations to do both?: 1. Maintain only session persistence to svc1 (eventually transitioning all traffic to svc1, possibly overloading it) 2. Apply session persistence to all svc1-3 — Reply to this email directly, view it on GitHub <#2634 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SYBW4CGHMLA5SMB53YXEGYBAVCNFSM6AAAAABADPMBPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRTHA3DCOBXHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[youngnick]

Updates #1619

[shaneutt]

/retest

[gcs278]

Last update just capitalized a MUST.

[robscott]

Thanks for all the persistence on this @gcs278! This all LGTM. Will defer to @shaneutt or @youngnick for final sign off.

/approve

[shaneutt]

/ok-to-test
/unhold
/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278, robscott, shaneutt

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:

• geps/OWNERS [robscott,shaneutt]

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