search

tintoy / dotnet-kube-client

A Kubernetes API client for .NET Standard / .NET Core
MIT License
192 stars 33 forks source link

Improved logic for ObserveLinesWithRetry #159

Closed tintoy closed 4 months ago

tintoy commented 4 months ago

tintoy/dotnet-kube-client#157

raman-m commented 3 months ago

Hello Adam! Fantastic library! However, why not link the PR to the issue in the Development right-side panel and close the issues in your backlog?

tintoy commented 3 months ago

Thanks! Sorry, I hadn’t published a new version on NuGet, yet because I was waiting to see if the changes I made resolved the original reporter’s issue (for issues I can’t reproduce myself, I usually publish a version on MyGet so we can see if they solve the problem). If I don’t get a reply in a day or so then I’ll just publish it anyway 🙂

tintoy commented 3 months ago

(I usually don't use the issue auto-close feature because I only consider an issue closed once the changes have been published to NuGet and the original reporter confirms that their issue has been resolved)

raman-m commented 3 months ago

If I don’t get a reply in a day or so then I’ll just publish it anyway 🙂

It wasn't just a day ago; the issue was fixed and merged a month ago. Didn't you receive any feedback from the author over that month? What are you waiting for? It seems you should be confident that the fix is correct. It's quite a risky workflow to await feedback from the majority of authors who are typically silent.

Regarding another matter, I reviewed the PR and noticed there were no tests included. Is this a common or an unusual occurrence? Another PRs

It's advisable to cover the code with tests or replicate the user's scenario to confirm the issue is resolved, and then promptly close the issue without awaiting a response from the author if they remain silent.

P.S. Waiting for feedback from issue raisers, who are mostly silent, can cause my project to stall. Typically, if an issue raiser is unresponsive, I proceed by covering the issue with acceptance tests and always try to include unit tests. If the issue raiser engages, I invite them to the testing phase; however, if acceptance tests have been developed, the related issue is automatically closed upon merging the PR. I find this workflow to be more efficient and quicker for real-world open-source contributions.

tintoy commented 3 months ago

If you’re volunteering to contribute I’d certainly appreciate the help; work hasn’t left much time to work on this for a while now 🙂

raman-m commented 3 months ago

Adam, may I inquire sincerely and receive a truthful response, please?

I've observed that most issues originate from 2018/19/20 and remain open. Is it a matter of time or development capacity? It appears the project was on hiatus after 2020, and currently, you merge pull requests sporadically, about one or two per month.

tintoy commented 3 months ago

Yes, it is a capacity issue; I have many other things going on in my life and while I do my best to keep supporting this project I don’t have as much free time as I used to. The last couple of roles I’ve worked in took up almost all of my capacity/enthusiasm for coding (leaving almost none for my own projects) - my most recent role is at least k8s-related which is why I’ve started working on this in my spare time again, but I still have other things that are more important to me at the moment. I’m always happy to work with anyone who wants to contribute to any of my projects (such as the MSBuild language service) but otherwise they get worked on when I have the time and enthusiasm.

tintoy commented 3 months ago

Can I, in turn, ask you where these questions are coming from?

raman-m commented 3 months ago

@tintoy commented If you’re volunteering to contribute I’d certainly appreciate the help; work hasn’t left much time to work on this for a while now 🙂

Fair enough! 😄 I could say the same about my Ocelot project, for which I am grateful for contributions. 🆗 If I find something or identify an issue, I will definitely report it and try to accompany it with a PR.

Are you a downstream consumer of this library?

The Ocelot team continues to utilize your library for Kubernetes integration, so here are the references 👉

https://github.com/ThreeMammals/Ocelot/blob/19a8e2f8b3e773fbe962a56fc2e7067b6b19132b/src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj#L32-L33

We have special Kube provider based on your KubeClient package for the Kubernetes service discovery feature within our project. If you don't mind I will contact you occasionally.

tintoy commented 3 months ago

If you don't mind I will contact you occasionally

Please do! I always find it easier to prioritise fixes when I have a sense of their impact (and downstream projects like yours are always going to be higher priority since they may impact more people).

I do still care about this project (we use it internally at my current employer) but it’s not always easy to find the time outside of work to work on this stuff.

Anytime you find an issue that is impacting you or your users let me know and I’ll do my best to get it onto the top of the pilez

PS. I’m a fan of Ocelot - you folks do good work 🙂

tintoy commented 3 months ago

As for testing, I’ve recently put a significant amount of effort into test infrastructure so future testing efforts may be a bit easier:

https://github.com/tintoy/dotnet-kube-client/blob/0896dec539857a4d586d5e341412a6631d9852bb/test/KubeClient.Extensions.DataProtection.Tests/KeyPersistenceTests.cs#L59 https://github.com/tintoy/dotnet-kube-client/blob/develop/test/KubeClient.Extensions.DataProtection.Tests/Mocks/MockKubeApiExtensions.cs (needs to be moved to shared project still)

raman-m commented 3 months ago

I do still care about this project (we use it internally at my current employer) but it’s not always easy to find the time outside of work to work on this stuff. we use it internally at my current employer

Fair enough! 😄 Do you know that YARP project was developed by Microsoft for Microsoft's internal needs. 🤣 It's my developer's joke! 😉 YARP has become a source of frustration due to the decision by Microsoft managers to "kill" Ocelot. 😠

Anytime you find an issue that is impacting you or your users let me know and I’ll do my best to get it onto the top of the pilez

It may not be a KubeClient issue, but we have observed unusual behavior with the IKubeApiClient when subjected to high parallel request loads in our testing environment. Further details can be found in #2111. Occasionally, the client yields a null EndpointsV1 object from our EndPointClientV1 class, as seen here: https://github.com/ThreeMammals/Ocelot/blob/19a8e2f8b3e773fbe962a56fc2e7067b6b19132b/src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs#L8 The issue appears to originate from lines L33-L35: https://github.com/ThreeMammals/Ocelot/blob/19a8e2f8b3e773fbe962a56fc2e7067b6b19132b/src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs#L33-L35 At times, the response is unsuccessful, resulting in a null return. Could you examine this class in our codebase and offer any recommendations? Is it possible to use ReadContentAsAsync to ensure a non-null EndpointsV1 object is returned, possibly with an empty list of subsets?

tintoy commented 3 months ago

Sure - I’ll take a look at this first thing tomorrow :)

tintoy commented 3 months ago

@raman-m OK, I think I can see a quick win there:

The way KubeClient tends to handle polymorphic response types that don't have a useful common ancestor (something the K8s API does all the time unfortunately) is by segregating response types by status code (usually success vs failure). Pattern is usually something derived from KubeResourceV1/KubeObjectV1 when successful, and StatusV1 when unsuccessful. There are some notable exceptions, such as K8s resource operations like Delete, that will return the existing resource state only if you request immediate deletion and otherwise return a StatusV1 😬.

There are extension methods in the KubeClient assembly for handling that type of scenario (something derived from KubeObjectV1 for success status codes, StatusV1 for failure status codes): https://github.com/tintoy/dotnet-kube-client/blob/0896dec539857a4d586d5e341412a6631d9852bb/src/KubeClient/ResourceClients/HttpExtensions.cs#L281 or, for more-dynamic response handling: https://github.com/tintoy/dotnet-kube-client/blob/0896dec539857a4d586d5e341412a6631d9852bb/src/KubeClient/ResourceClients/HttpExtensions.cs#L210 Or if you want to do all the handling yourself, there's a more low-level version: https://github.com/tintoy/HTTPlease/blob/0e872957866fedea194c0f0b4d36d71d1cb85835/src/HTTPlease.Formatters/FormatterResponseExtensions.cs#L287

I think you probably want the second option; the result object it returns will have Status and Resource properties (but will implicitly cast to the resource type if there's no status; i.e. operation was successful) and you can check if Status property is non-null and decide what to do (maybe log error code from K8s API, not sure what works best for your scenario there but that's usually all I can do there, I find).

Or if you use the first option, you can catch a HttpRequestException<StatusV1> and choose what to log/return there instead. If you provide an operation description, it will use that to provide more-useful exception messages.

Example usage: https://github.com/tintoy/dotnet-kube-client/blob/0896dec539857a4d586d5e341412a6631d9852bb/src/KubeClient/ResourceClients/PodClientV1.cs#L249

If that doesn't do the trick, let me know and I can open an issue for it.