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.

Sign up for GitHub

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

Jump to bottom

k8s: Split SharedResources into binary specific cells #25757

Merged
merged 4 commits into from Jun 1, 2023
Merged

k8s: Split SharedResources into binary specific cells #25757

merged 4 commits into from Jun 1, 2023

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented May 30, 2023 •
edited

Split the SharedResources cell into multiple cells, one for each binary (agent, operator and clustermesh-apiserver).
This improves the dependencies management, since each binary will depend only on the k8s resources that it actually uses.

Also, the functional options pattern is introduced to give more flexibility when customizing a resource events stream.

Notes for the reviewers: review commit by commit might be simpler. The last commit is the one responsible for the high number of files modified, but it is just a rename (sharedResources -> resources) after the removal of the single SharedResources cell.

Fixes #24070

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 30, 2023
@pippolo84 pippolo84 added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/operator Impacts the cilium-operator component area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related. labels May 30, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 30, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/split-shared-resources branch from 56663a9 to 9484f01 Compare May 30, 2023 13:15
joamaki
joamaki reviewed May 30, 2023
View reviewed changes
pkg/k8s/resource_ctors.go Outdated
return resource.New[*cilium_api_v2.CiliumIdentity](lc, lw), nil
}

func CiliumNetworkPolicy(lc hive.Lifecycle, cs client.Clientset, opts ...func(*metav1.ListOptions)) (resource.Resource[*cilium_api_v2.CiliumNetworkPolicy], error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it'd make sense to namespace these by package, e.g. pkg/k8s/resources and then we'd have resources.CiliumNetworkPolicy, or by the suffix (NamespaceResource). I think either is probably fine and I do like having k8s in the package name. If you go the suffix route, then let's add it to these Cilium* functions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the Resource suffix where missing.

@pippolo84 pippolo84 force-pushed the pr/pippolo84/split-shared-resources branch 2 times, most recently from 81ce31d to d5fa5d5 Compare May 30, 2023 16:46
@pippolo84 pippolo84 changed the title k8s: Split SharedResources into binary specific structs k8s: Split SharedResources into binary specific cells May 30, 2023
@pippolo84 pippolo84 marked this pull request as ready for review May 30, 2023 17:01
@pippolo84 pippolo84 requested review from a team as code owners May 30, 2023 17:01
gandro
gandro approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

My codeowners look good to me.

It might be a naive question, but why do we even need the per-binary packages? Can't there be something like cell.LazyProvide that only executes the constructor if it's actually requested by a dependency?

@pippolo84
Copy link
Member Author

It might be a naive question, but why do we even need the per-binary packages? Can't there be something like cell.LazyProvide that only executes the constructor if it's actually requested by a dependency?

I think it'd be a reasonable alternative for the current usage of the resources, even if keeping a single cell would not allow to customize each resource events stream when consumed by multiple binaries.
As an example, we might want to use different selectors (or different TransformFunc when we'll add support for that in resource.Resource[T]).

YutaroHayakawa
YutaroHayakawa approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Looks good to me from ClusterMesh's perspective.

clustermesh-apiserver/k8s/resources.go
"Clustermesh-apiserver Kubernetes resources",

cell.Provide(
k8s.ServiceResource,
Copy link
Member

Choose a reason for hiding this comment

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

I think we need CiliumNode, Endpoints, and CiliumEndpoint resources, but they are currently using the legacy clientset. Approving with the understanding that they will be followed up at some point.

Copy link
Member Author

@pippolo84 pippolo84 Jun 1, 2023 •
edited

Choose a reason for hiding this comment

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

Yep, there is already work in progress to port other resources to the new resource.Resource[T] framework.

@pippolo84
Copy link
Member Author

pippolo84 commented Jun 1, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, DSR with Geneve and Maglev

Failure Output

FAIL: Can not connect to service "http://[fd04::11]:30775" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/356/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@pippolo84
k8s: Add Resource suffix to k8s resources constructors
021c5a4 
Add the "Resource" suffix to the CNP, CCNP and CCG resources
constructors to uniform the methods naming.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: Use functional options for k8s resources ctors
aa592ba 
Use functional options pattern to customize k8s resources constructors
with option modifiers.  This pattern allows to expose a base constructor
that is easily customizable with modifiers like labels-based objects
filtering.

This is helpful to customize the events stream for a given resource that
is used by multiple binaries, like the agent and the operator.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: Split SharedResources into binary specific cells
45da4cd 
Split SharedResources cell into multiple cells, one for each binary
relying on k8s resources (agent, operator and clustermesh-apiserver).
Each binary depends only on the specific resources that it actually
uses, and it customizes each resource events stream based on its own
specific needs.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: Rename daemon SharedResources in Resources
303787d 
The SharedResources cell has been split into multiple binary-specific
cells, so change the name of the daemon params and k8s watchers fields
accordingly.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/split-shared-resources branch from d5fa5d5 to 303787d Compare June 1, 2023 10:27
@pippolo84
Copy link
Member Author

Force-pushed to solve a conflict in clustermesh-apiserver/main.go

@pippolo84
Copy link
Member Author

/test

ldelossa
ldelossa approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 1, 2023
@julianwiedmann julianwiedmann merged commit fcc7532 into cilium:main Jun 1, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki left review comments

@gandro gandro gandro approved these changes

@lmb lmb lmb approved these changes

@nebril nebril nebril approved these changes

@christarazi christarazi christarazi approved these changes

@ldelossa ldelossa ldelossa approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/operator Impacts the cilium-operator component ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modularization: split SharedResources into OperatorResources and DaemonResources
9 participants
@pippolo84 @gandro @lmb @joamaki @nebril @christarazi @ldelossa @YutaroHayakawa @julianwiedmann