#2110 Review load balancing and independent fetching the list of services in `Kube` provider #2111

antikorol · 2024-06-26T16:58:44Z

Fixes #2110

After Upgrade to 23.3.3 from 23.2.2 about every 20th - 30th request finished with status code 500 #2110

Proposed Changes

Move the creation of the services list from the class field to the method, to prevent list modification from different threads, which caused the partially modified list of services to be returned.

…thod, to prevent modification list from different threads

raman-m

It appears that the significant concurrency issue in the Kube class mentioned in the #2110 (comment) has been resolved, yet a genuine fixing solution for the RoundRobin class is still required.

Please note that our development process necessitates both unit and acceptance testing. It is advisable to replicate the issue with an acceptance test before implementing the bug-fixing solution.
We need to replicate the bug using an acceptance test. I can draft a preliminary version of the test, and you can then refine it to create the final version.

src/Ocelot.Provider.Kubernetes/Kube.cs

antikorol · 2024-06-30T19:05:46Z

It appears that the significant concurrency issue in the Kube class mentioned in the #2110 (comment) has been resolved, yet a genuine fixing solution for the RoundRobin class is still required.

Please note that our development process necessitates both unit and acceptance testing. It is advisable to replicate the issue with an acceptance test before implementing the bug-fixing solution. We need to replicate the bug using an acceptance test. I can draft a preliminary version of the test, and you can then refine it to create the final version.

@raman-m I'm not quite sure which fix you are referring to. In this case, there will no longer be a List of the form [service, null] because the issue with modifying the List of services from different threads has been fixed. Or do you mean adding a check that if the service or the HostAndPort property is null, then log the error and return a 404 status code for that request?

raman-m · 2024-07-09T18:39:16Z

Or do you mean adding a check that if the service or the HostAndPort property is null, then log the error and return a 404 status code for that request?

Yes, I do. Could you include this check within the RoundRobin class plz?
Thank you for adding the unit test. I'm assessing the efficiency of writing an acceptance test, which is expected to be quite complicated. Have you tested the bug fix in your environment?

antikorol · 2024-07-09T19:48:09Z

Or do you mean adding a check that if the service or the HostAndPort property is null, then log the error and return a 404 status code for that request?

Yes, I do. Could you include this check within the RoundRobin class plz? Thank you for adding the unit test. I'm assessing the efficiency of writing an acceptance test, which is expected to be quite complicated. Have you tested the bug fix in your environment?

Added new error to handle this case when service or HostAndPort is null.
The fix has been tested in dev/preprod environments, and the fix was checked with load testing at around 30 rps.

raman-m · 2024-07-10T10:47:41Z

Thank you for conducting the tests!
Is the official hotfix release package, version 23.3.4, needed to update your all environments and conduct the final bugfix test?
Can you update environments with manually built DLLs?

Additionally, I'm going to review the code once more...
Having unit/integration tests is likely sufficient to deem the code acceptable.

raman-m

The primary concern is the redundancy of the ServiceIsInvalidError class.

src/Ocelot/LoadBalancer/LoadBalancers/ServiceIsInvalidError.cs

src/Ocelot/LoadBalancer/LoadBalancers/RoundRobin.cs

test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs

antikorol · 2024-07-10T11:24:50Z

Thank you for conducting the tests! Is the official hotfix release package, version 23.3.4, needed to update your all environments and conduct the final bugfix test? Can you update environments with manually built DLLs?

At your discretion, I can release it with my fix for now and remove it in the future.

raman-m · 2024-07-10T11:54:10Z

Great, Roman!
I'd like to merge it into the develop branch, after which you can create a temporary build from your forked repository and deploy it to the Prod environment, okay?

Could you provide more details about your Production environment?
Please include information on the hosting server (Docker, IIS, self-hosted), the number of Ocelot instances, CPU & memory resources per Ocelot instance, requests per second, and the technical stack (specific Ocelot features utilized), etc.

Is the Ocelot gateway deployed on a public or private network?
Also, could you specify the industry that the Prod product is part of?

…d balancer

antikorol · 2024-07-10T12:54:36Z

Is the Ocelot gateway deployed on a public or private network?

Unfortunately, the game is still in closed beta testing, and the loads on prod are much times lower than what we simulate during tests. Ocelot is deployed as a regular deployment in k8s.
But If I find any issues, I will let you know :)

raman-m · 2024-08-05T19:47:47Z

I've opted for the third option and implemented the Retry pattern. The previously unstable scenario test is now performing much better. I would consider it quite stable. So, I've resolved the issue with the 404 status, as mentioned above. However, sometimes Ocelot returns a 500 status, and the underlying cause is currently unclear to me.

Investigating this issue necessitates further development and execution of tests. However, this error is quite rare, occurring at a rate of 1/100, 1/500, or even 1/1000, which means it demands extensive testing in production environments, as production is certainly not the same as testing.
In conclusion, I believe we can proceed with the release at this hotfix quality. 😎

raman-m · 2024-08-05T21:00:34Z

Development Complete❕

Most important changes are

@ggnaegi commented on Aug 4, 2024

Please review!

src/Ocelot.Provider.Kubernetes/Kube.cs

src/Ocelot.Provider.Kubernetes/KubeServiceCreator.cs

src/Ocelot/Infrastructure/DesignPatterns/Retry.cs

src/Ocelot/LoadBalancer/LoadBalancers/LeastConnection.cs

src/Ocelot/LoadBalancer/LoadBalancers/RoundRobin.cs

src/Ocelot/Values/ServiceHostAndPort.cs

ggnaegi · 2024-08-06T08:44:02Z

src/Ocelot/Values/ServiceHostAndPort.cs

+
+    public bool Equals(ServiceHostAndPort other) => this == other;
+    public override bool Equals(object obj)
+        => obj != null && obj is ServiceHostAndPort o && this == o;


public override bool Equals(object obj) => obj is ServiceHostAndPort o && this == o;

merge into pattern?

Which one? Sorry, I can't get you!

I have attempted to adhere to all documentation and best practices from XML documentation links for the equality operator. Thus, I've combined various practices to write more concise code. The expression this == o signifies that I am utilizing the native class operator ==.

Certainly, all properties comparison predicate can be transferred from operator == to Equals(ServiceHostAndPort). Ultimately, it seems challenging to distill further patterns from this specific version of the code. 😄 Not a fan of the MS docs examples 🤣 so I've extracted patterns from MS docs definitely 🙊

@ggnaegi wrote

merge into pattern?

Swiss humor? Yeah! 🥲 I know you don't like my Retry pattern because you like Polly. 😏
Let's ask another big fan of Polly... not @RaynaldM 🤣 ... and this is Mr. Seth @ks1990cn Hello Tanmay! 😉

raman-m

@ggnaegi I've addressed all issues from your code review. Awaiting your approval or next suggestions...
@RaynaldM Welcome!

raman-m · 2024-08-07T09:34:56Z

@ggnaegi What else?
Did I miss something from your code review?

ggnaegi

Ok, fine for me, thanks!

raman-m · 2024-08-07T18:41:41Z

TODO

React to Adam's feedback on the questions in the tintoy/dotnet-kube-client repo.
Evaluate the initiation of a new project as an Ocelot package for kubernetes-client/csharp, replacing the currently used tintoy/dotnet-kube-client library due to its slow development pace. This would involve transitioning to the industry-standard library for Kubernetes service discovery.
Undertake refactoring of load balancing following the identification of design bottlenecks in the Custom Load Balancers feature. References →
- release 15.0.3
- feature commit ccb5b84
- PR Add ability to register custom load balancers #1155
- Feat Add ability to register custom load balancers #961

src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj

antikorol · 2024-08-19T11:49:48Z

@raman-m commented on Aug 5, 2024

Please share your thoughts❕
P.S. I believe the 1st option is entirely impractical, and I am deliberating between the 2nd and 3rd options, convinced that the Retry pattern must be adopted.

Hello @raman-m Raman.
Sorry. I was on vacation and didn't notice, among the many notifications, that there was a question directed at me. However, I see the solution has already been implemented :)

raman-m · 2024-08-20T12:41:12Z

@antikorol Right! The hotfix has been implemented! However I've asked you to start testing.
Will you wait for official 23.3.4 package release?

antikorol · 2024-08-21T08:26:13Z

@antikorol Right! The hotfix has been implemented! However I've asked you to start testing. Will you wait for official 23.3.4 package release?

@raman-m
According to company policy, we are not allowed to publish packages to Artifactory that are not related to our project, and we can only use specific sources for NuGet packages. As soon as you release a new version, I will update and thoroughly test the fixes.

However, I copied your changes related to fix into my custom Kubernetes provider, which we are currently using as a fix, and so far everything is working well.

Thank you

@raman-m

…Blue Olympic Balumbes release * #2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot` (#2120) * Adding unit test first * Fixing default global config file not being found in folder * Adding PR trait to test * Backing out whitespace changes * Code review by @raman-m * Create Configuration feature folder and move test classes * Adjust namespace and review what we have * Acceptance tests for #2084 user scenario --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Bump Steeltoe.Discovery.Eureka from 3.2.5 to 3.2.8 in /src/Ocelot.Provider.Eureka (#2122) * Bump Steeltoe.Discovery.Eureka in /src/Ocelot.Provider.Eureka Bumps [Steeltoe.Discovery.Eureka](https://github.com/SteeltoeOSS/Steeltoe) from 3.2.5 to 3.2.8. - [Release notes](https://github.com/SteeltoeOSS/Steeltoe/releases) - [Changelog](https://github.com/SteeltoeOSS/Steeltoe/blob/main/Steeltoe.Release.ruleset) - [Commits](SteeltoeOSS/Steeltoe@3.2.5...3.2.8) --- updated-dependencies: - dependency-name: Steeltoe.Discovery.Eureka dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * Bump Steeltoe.Discovery.ClientCore from 3.2.5 to 3.2.8 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2110 Review load balancing and independent fetching the list of services in `Kube` provider (#2111) * Move the creation of the services list from the class field to the method, to prevent modification list from different threads * Early return after data checking * Add unit test for concurrent get list of services * Add logging for invalid service configuration error in RoundRobin load balancer * Code review by @raman-m * Workaround for mistakes made during acceptance testing of load balancing versus service discovery, where tests designed for parallel requests were mistakenly executed sequentially. This resulted in load balancers being loaded by sequential `HttpClient` calls, which was a significant oversight. * Let's DRY StickySessionsTests * Add acceptance tests, but... RoundRobin is not actually RoundRobin 😁 -> 😆 * Independent static indexing iterators per route via service names * Stabilize `CookieStickySessions` load balancer. Review tests after refactoring of `RoundRobin` load balancer * Refactor Lease operation for load balancing. Review LeastConnection load balancer * Leasing mechanism in Round Robin load balancer * Acceptance tests, final version * Apply Retry pattern for K8s endpoint integration * Fix IDE warnings and messages * Follow suggestions and fix issues from code review by @ggnaegi * Bump KubeClient from 2.4.10 to 2.5.8 * Fix warnings * Final version of `Retry` pattern --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Downgrade the Warning to Information on missing `Content-Length` header in `MultiplexingMiddleware` (#2146) * fix: downgrade the warning to information on missing content-length header * chore: add route name to logs * test: fixing multiplexing middleware tests * Code review by @raman-m --------- Co-authored-by: Paul Roy <paul.roy@astriis.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Correct the broken link to the GraphQL sample's `README.md` (#2149) Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars (#2150) * regex escape handling for url templates * refactored regex method to lamda version * Quick code review by @raman-m * added acceptance test for url regex bug * moved acceptance test to routing tests * Convert to theory: define 2 test cases --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2119 Review load balancing (2nd round) and redesign `DefaultConsulServiceBuilder` with `ConsulProviderFactory` refactoring to make it thread safe and friendly (#2151) * Review tests * History of Service Discovery testing: add traits * LoadBalancer traits * #2119 Steps to Reproduce * Reuse service handlers of `ConcurrentSteps` * Reuse service counters of `ConcurrentSteps` * Add LoadBalancer namespace and move classes * Move `Lease` * Move `LeaseEventArgs` * Analyze load balancers aka `ILoadBalancerAnalyzer` interface objects * Prefer using named local methods as delegates over anonymous methods for awesome call stack, ensuring the delegate's typed result matches the typed balancer's creator. Additionally, employ an IServiceProvider workaround. * Review load balancing. Assert service & leasing counters as concurrent step. Final version of acceptance test. * Fixed naming violation for asynchronous methods: `Lease` -> `LeaseAsync` * Fix ugly reflection issue of dymanic detection in favor of static type property * Propagate the `ConsulRegistryConfiguration` object through `HttpContext` in the scoped version of the default service builder, utilizing the injected `IHttpContextAccessor` object. Update `ConsulProviderFactory`. Update docs. Update tests. * Add tests from clean experiment * Final review of the tests * Review `IHttpContextAccessor` logic. Convert anonymous delegates to named ones in placeholders processing * Tried to enhance more, but failed --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Ben Bartholomew <70723971+ben-bartholomew@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Roman <61905975+antikorol@users.noreply.github.com> Co-authored-by: Paul Roy <paul.achess.roy@gmail.com> Co-authored-by: Paul Roy <paul.roy@astriis.com> Co-authored-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Finn <26823828+int0x81@users.noreply.github.com>

raman-m · 2024-10-08T18:14:47Z

@antikorol FYI → Ocelot 23.3.4 Nuget package

Move the creation of the services list from the class field to the me…

360bd61

…thod, to prevent modification list from different threads

raman-m assigned raman-m and antikorol Jun 27, 2024

raman-m added bug Identified as a potential bug Service Discovery Ocelot feature: Service Discovery Load Balancer Ocelot feature: Load Balancer Consul Service discovery by Consul Oct'24 October 2024 release labels Jun 27, 2024

raman-m added this to the Summer'24 milestone Jun 27, 2024

Early return after data checking

d19322d

raman-m requested changes Jun 30, 2024

View reviewed changes

src/Ocelot.Provider.Kubernetes/Kube.cs Show resolved Hide resolved

src/Ocelot.Provider.Kubernetes/Kube.cs Outdated Show resolved Hide resolved

src/Ocelot.Provider.Kubernetes/Kube.cs Show resolved Hide resolved

raman-m changed the title ~~Fix for race condition on fetching the list of services in Kubernetes Service Provider~~ #2110 Fix for race condition on fetching the list of services in Kubernetes service discovery provider Jun 30, 2024

Add unit test for concurrent get list of services

0e9a616

antikorol force-pushed the bugfix/race-condition-on-fetching-services-in-service-discovery-provider branch from 75a3090 to c360228 Compare July 9, 2024 19:45

raman-m requested changes Jul 10, 2024

View reviewed changes

raman-m requested review from RaynaldM and ggnaegi July 10, 2024 11:15

raman-m added hotfix Gitflow: Hotfix issue, PR related to hotfix branch high High priority labels Jul 10, 2024

raman-m removed the hotfix Gitflow: Hotfix issue, PR related to hotfix branch label Jul 10, 2024

Add logging for invalid service configuration error in RoundRobin loa…

bbed7d1

…d balancer

antikorol force-pushed the bugfix/race-condition-on-fetching-services-in-service-discovery-provider branch from c360228 to bbed7d1 Compare July 10, 2024 12:30

raman-m removed this from the Summer'24 milestone Jul 10, 2024

raman-m changed the title ~~#2110 Fix for race condition on fetching the list of services in Kubernetes service discovery provider~~ #2110 Review load balancing and independent fetching the list of services in Kube provider Aug 5, 2024

Apply Retry pattern for K8s endpoint integration

312bc6e

Fix IDE warnings and messages

6367cff

raman-m reviewed Aug 5, 2024

View reviewed changes

src/Ocelot.Provider.Kubernetes/Kube.cs Outdated Show resolved Hide resolved

ggnaegi requested changes Aug 6, 2024

View reviewed changes

Follow suggestions and fix issues from code review by @ggnaegi

c4c7ae0

raman-m approved these changes Aug 6, 2024

View reviewed changes

raman-m added 2 commits August 6, 2024 23:40

Bump KubeClient from 2.4.10 to 2.5.8

c42302c

Fix warnings

e1fe81f

raman-m requested review from raman-m and ggnaegi August 7, 2024 09:35

Final version of Retry pattern

0305278

raman-m approved these changes Aug 7, 2024

View reviewed changes

ggnaegi approved these changes Aug 7, 2024

View reviewed changes

raman-m merged commit 19a8e2f into ThreeMammals:develop Aug 7, 2024
1 check passed

raman-m mentioned this pull request Aug 9, 2024

Improved logic for ObserveLinesWithRetry tintoy/dotnet-kube-client#159

Merged

raman-m reviewed Aug 12, 2024

View reviewed changes

src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj Show resolved Hide resolved

raman-m mentioned this pull request Aug 12, 2024

TODO and other follow-up tasks #2141

Open

raman-m removed the Oct'24 October 2024 release label Aug 12, 2024

raman-m mentioned this pull request Oct 3, 2024

Release 23.3.4 | v23.3 Hotfixes | Blue Olympic Balumbes release #2162

Merged

raman-m mentioned this pull request Nov 9, 2024

Missing scheme on downstream request #1274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2110 Review load balancing and independent fetching the list of services in `Kube` provider #2111

#2110 Review load balancing and independent fetching the list of services in `Kube` provider #2111

antikorol commented Jun 26, 2024 •

edited

Loading

raman-m left a comment

antikorol commented Jun 30, 2024 •

edited

Loading

raman-m commented Jul 9, 2024 •

edited

Loading

antikorol commented Jul 9, 2024

raman-m commented Jul 10, 2024 •

edited

Loading

raman-m left a comment

antikorol commented Jul 10, 2024

raman-m commented Jul 10, 2024

antikorol commented Jul 10, 2024

raman-m commented Aug 5, 2024 •

edited

Loading

raman-m commented Aug 5, 2024 •

edited

Loading

ggnaegi Aug 6, 2024

raman-m Aug 6, 2024

raman-m Aug 6, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Aug 7, 2024

ggnaegi left a comment

raman-m commented Aug 7, 2024 •

edited

Loading

antikorol commented Aug 19, 2024 •

edited by raman-m

Loading

raman-m commented Aug 20, 2024 •

edited

Loading

antikorol commented Aug 21, 2024

raman-m commented Oct 8, 2024

#2110 Review load balancing and independent fetching the list of services in Kube provider #2111

#2110 Review load balancing and independent fetching the list of services in Kube provider #2111

Conversation

antikorol commented Jun 26, 2024 • edited Loading

Fixes #2110

Proposed Changes

raman-m left a comment

Choose a reason for hiding this comment

antikorol commented Jun 30, 2024 • edited Loading

raman-m commented Jul 9, 2024 • edited Loading

antikorol commented Jul 9, 2024

raman-m commented Jul 10, 2024 • edited Loading

raman-m left a comment

Choose a reason for hiding this comment

antikorol commented Jul 10, 2024

raman-m commented Jul 10, 2024

antikorol commented Jul 10, 2024

raman-m commented Aug 5, 2024 • edited Loading

raman-m commented Aug 5, 2024 • edited Loading

Development Complete❕

ggnaegi Aug 6, 2024

Choose a reason for hiding this comment

raman-m Aug 6, 2024

Choose a reason for hiding this comment

raman-m Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

raman-m commented Aug 7, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m commented Aug 7, 2024 • edited Loading

TODO

antikorol commented Aug 19, 2024 • edited by raman-m Loading

raman-m commented Aug 20, 2024 • edited Loading

antikorol commented Aug 21, 2024

raman-m commented Oct 8, 2024

#2110 Review load balancing and independent fetching the list of services in `Kube` provider #2111

#2110 Review load balancing and independent fetching the list of services in `Kube` provider #2111

antikorol commented Jun 26, 2024 •

edited

Loading

antikorol commented Jun 30, 2024 •

edited

Loading

raman-m commented Jul 9, 2024 •

edited

Loading

raman-m commented Jul 10, 2024 •

edited

Loading

raman-m commented Aug 5, 2024 •

edited

Loading

raman-m commented Aug 5, 2024 •

edited

Loading

raman-m Aug 6, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Aug 7, 2024 •

edited

Loading

antikorol commented Aug 19, 2024 •

edited by raman-m

Loading

raman-m commented Aug 20, 2024 •

edited

Loading