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

Remove dependency on k8s.io code #4

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Apr 1, 2024

Firstly, this project is really interesting, thank you, and I believe that statedb will be extremely useful as the core data store for many applications outside Cilium and Kubernetes.

However, the current dependency graph is huge and includes k8s.io/api, k8s.io/apimachinery, and k8s.io/client-go. This makes the project significantly less appealing as few non-Kubernetes projects want to pull in large sections of the huge Kubernetes codebase as a dependency, even if most of it will be eventually pruned by the linker.

This is a draft PR, and probably should not be merged, to indicate the effort required to make this code more generic:

  • The Kubernetes reflector is simply removed, as this is application-specific code and not relevant to statedb's core functionality. It can be moved to the application using statedb.
  • The single type (a generic set) from k8s.io/apimachinery/pkg/util/sets is vendored, removing the dependency on k8s.io/apimachinery.

This is a draft PR designed to encourage discussion, not to be merged.

@joamaki
Copy link
Contributor

joamaki commented Apr 3, 2024

Thanks! Agree on removing the k8s specific code so let's go for that. I'll move it into cilium/cilium or into a statedb utils repo.

I would not vendor sets.Set since we can instead just use map[T]bool or map[T]struct{} internally where needed. Also CollectSet now returns the wrong type and it's used in cilium/cilium in places that expect the sets.Set type. I think we can just drop CollectSet for now.

Could you refactor the remaining sets.Set uses to just use map[T]bool? It's mostly just test+example and NewTable so should be straightforward.

@twpayne
Copy link
Contributor Author

twpayne commented Apr 3, 2024

Could you refactor the remaining sets.Set uses to just use map[T]bool? It's mostly just test+example and NewTable so should be straightforward.

Done.

@twpayne twpayne marked this pull request as ready for review April 3, 2024 17:53
@twpayne
Copy link
Contributor Author

twpayne commented Apr 3, 2024

I couldn't resist adding an extra commit to use map[T]struct{} instead of map[T]bool for internal set types. As you know, this reduces memory usage at the cost of slightly different ergonomics. However, memory usage is unlikely to be a significant factor here (all sets are currently temporary, either within a function or within a test).

@joamaki joamaki merged commit a249f7a into cilium:main Apr 4, 2024
1 check passed
@twpayne twpayne deleted the no-k8s.io branch April 4, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants