Improved logic for ObserveLinesWithRetry

[tintoy]

#157

[raman-m]

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]

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]

(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]

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

• No tests in version rollout -> v2.4.0 #142
• No tests in Added support for previous flag to Pod Logs endpoint #141
• No tests in Fixed fieldSelector for EventClientV1 #140
• But tests exists in KubeClient v2.3.5 #106

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]

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]

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

• What is your plan for addressing and resolving all 39 issues in the backlog over this or the next year?

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]

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]

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

• Are you a downstream consumer of this library?
• Or are you looking for a project to contribute to?
• Or is there some other reason for your interest?

[raman-m]

@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]

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]

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

dotnet-kube-client/test/KubeClient.Extensions.DataProtection.Tests/KeyPersistenceTests.cs

Line 59 in 0896dec

api.HandleResources<SecretV1, SecretListV1>(secrets, secretWatchSubject);

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]

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]

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

[tintoy]

@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):

dotnet-kube-client/src/KubeClient/ResourceClients/HttpExtensions.cs

Line 281 in 0896dec

public static async Task<TObject> ReadContentAsObjectV1Async<TObject>(this Task<HttpResponseMessage> response, string operationDescription, params HttpStatusCode[] successStatusCodes)

or, for more-dynamic response handling:

dotnet-kube-client/src/KubeClient/ResourceClients/HttpExtensions.cs

Line 210 in 0896dec

public static async Task<KubeResourceResultV1<KubeResourceV1>> ReadContentAsResourceOrStatusV1(this Task<HttpResponseMessage> response, Type modelType, string operationDescription, params HttpStatusCode[] successStatusCodes)

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:

dotnet-kube-client/src/KubeClient/ResourceClients/PodClientV1.cs

Line 249 in 0896dec

.ReadContentAsObjectV1Async<PodV1>(

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