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

#2168 WatchKube discovery provider #2174

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

kick2nick
Copy link
Contributor

Closes #2168

Proposed Changes

  • WatchKube service discovery provider that utilizes k8s watch api requests for efficient detection of changes.

It's proof of concept that needs improvements (see comments) and tests.
Tested locally, it works as expected, but with older versions of KubeClient which can be used with newer one after fix tintoy/dotnet-kube-client#163.

p.s. sorry for mess with #2173
Had no intention to force downgrade of KubeClient, will resume after fix)

@kick2nick kick2nick changed the title Added 'WatchKube' discovery provider #2168 WatchKube discovery provider Oct 14, 2024
@kick2nick kick2nick marked this pull request as draft October 14, 2024 18:17
@raman-m raman-m added proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery Kubernetes Service discovery by Kubernetes labels Oct 15, 2024
@raman-m
Copy link
Member

raman-m commented Oct 16, 2024

I've rebased the branch. Don't push please❗
But if you want to commit more, then you have to delete branch, fetch all, checkout the branch locally 👉

  • git switch develop
  • git branch -D 2168-watchkube
  • git fetch --all
  • git pull
  • git checkout 2168-watchkube

And you will be able to contribute more 😉

@kick2nick
Copy link
Contributor Author

Hi @raman-m , from my point of view all required stuff is done for moving to code review. Or am I missing something?

@raman-m
Copy link
Member

raman-m commented Oct 24, 2024

The issue found at tintoy/dotnet-kube-client#163 was resolved by tintoy/dotnet-kube-client#164 and included in the release v2.5.12. However, this release has not been marked as the latest on the repository's releases page, where v2.5.10 is still listed as the most recent version. Could you please explain why v2.5.10 is considered the latest release?

@raman-m
Copy link
Member

raman-m commented Oct 24, 2024

from my point of view all required stuff is done for moving to code review. Or am I missing something?

If you're confident and have completed the main development phase, can't you press the "Ready for Review" button?

@kick2nick
Copy link
Contributor Author

Could you please explain why v2.5.10 is considered the latest release?

Have no idea, let's wait for the author's answer to your question.

@kick2nick kick2nick marked this pull request as ready for review October 24, 2024 08:42
@raman-m
Copy link
Member

raman-m commented Oct 24, 2024

Hello, Nikolay!

While it's true that the PR seems partially ready for review following the closure of tintoy/dotnet-kube-client#163 and its subsequent release of v2.5.12, I am concerned that reviewing this PR may be premature due to the following dependencies:

  1. This PR relies on #977 Enable validation of DI scopes in Kube and PollKube discovery providers #2180, which needs to be released beforehand as a part of the next 23.3.6 patch.
  2. My plan is to include it in the November'24 milestone, which will prioritize the .NET 9 upgrade and migration to the net9.0 framework, with a lower priority assigned to introducing new features.

@raman-m raman-m added the Autumn'24 Autumn 2024 release label Oct 24, 2024
@raman-m raman-m added this to the November'24 milestone Oct 24, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Testing the new WatchKube type in ShouldReturnServicesFromK8s is necessary, but it is not sufficient for completing acceptance testing.

While it is well-tested by unit tests, there are no new acceptance tests for the proposed feature. Please review the scenarios of the current Kube and PollKube providers for inspiration. If Not, I will share my ideas later, in 1-2 weeks, after the release of the v23.3.6 patch.

@kick2nick
Copy link
Contributor Author

This PR relies on #2180, which needs to be released beforehand as a part of the next 23.3.6 patch.

Not quite "relies", but conflicts on rebase will apear, I'll resolve them after #2180 merging.

My plan is to include it in the November'24 milestone, which will prioritize the .NET 9 upgrade and migration to the net9.0 framework, with a lower priority assigned to introducing new features.

No problem, I'll be in touch when you get back to this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Autumn'24 Autumn 2024 release Kubernetes Service discovery by Kubernetes proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watch request fails with InvalidOperationException
2 participants