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

Refactor client/pool into a single struct #281

Closed
hausdorff opened this issue Nov 16, 2018 · 2 comments
Closed

Refactor client/pool into a single struct #281

hausdorff opened this issue Nov 16, 2018 · 2 comments
Assignees
Labels
help-wanted We'd love your contributions on this issue kind/engineering Work that is not visible to an external user
Milestone

Comments

@hausdorff
Copy link
Contributor

In the provider and the await package we pass around a discovery client and a client pool so that we can perform and await operations, and get things like Event to produce diagnostic information on the error path.

This has become cumbersome, resulting in a lot of duplicated logic, and I propose to merge these into a single struct that makes these common cases simple. The main benefit is that we write tests that make sure we don't regress the very subtle error handling behavior in the create, delete, and update paths.


When we began our journey, the use of kubernetes/client-go was fairly simple. With the introduction of robust CRD support and client-level retry create operations, it has become considerably more complex -- we need to take the disparate error types exposed by the client, and map them into sensible await logic in the await package. For example, in the delete path, we have to:

  1. Get a G/V/K-specific client from the discovery client and the client pool.
  2. Call .Delete.
  3. Call .Watch and wait for a 404 error.

If any of these intermediate steps results in an IsNotFound error, we need to terminate with a successful delete.

To make matters worse, the create and update path have very similar logic.

In the long run this is not maintainable because we have no way to convincingly test that our logic is correct. My proposal is to merge the client pool and the discovery client into a single struct that collapses these intermediate steps into a single .Get, .Watch, etc. Because the client pool and the discovery client are both interfaces, this will make it much easier to encode the desired state machine in tests, making this logic much more verbose.

@hausdorff
Copy link
Contributor Author

As @lblackstone points out, this may involve upgrading client-go: kubernetes/kubernetes#62913

@hausdorff hausdorff modified the milestones: 0.19, 0.20 Nov 16, 2018
@hausdorff hausdorff removed this from the 0.20 milestone Dec 18, 2018
@hausdorff hausdorff added the help-wanted We'd love your contributions on this issue label Dec 18, 2018
@hausdorff
Copy link
Contributor Author

@lblackstone I'm assuming this will happen alongside #344? (I'll proactively put in M20 just ot make sure we don't lose it.)

@hausdorff hausdorff added this to the 0.20 milestone Jan 9, 2019
@infin8x infin8x added the kind/engineering Work that is not visible to an external user label Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted We'd love your contributions on this issue kind/engineering Work that is not visible to an external user
Projects
None yet
Development

No branches or pull requests

3 participants