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

Break import cycles and move the datapath cell to datapath/cell.go #24337

Merged
merged 2 commits into from Mar 16, 2023

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Mar 13, 2023

In preparation of moving the datapath module into datapath/cells.go, we need to break import cycles by moving the datapath interface definitions into their own package. Use the existing datapath/types package for this, and adjust code and tests everywhere accordingly. Then move the datapath module and associated initialization code to pkg/datapath.

The sysctl stuff should most definitely not live top level in pkg/datapath, but given this touches a million things, I'd like to keep the functional diff minimal and just move code around.

@bimmlerd bimmlerd added release-note/misc This PR makes changes that have no direct user impact. area/modularization labels Mar 13, 2023
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/move-datapath-types branch 4 times, most recently from 4cde4c2 to 11afe7f Compare March 13, 2023 14:45
@bimmlerd bimmlerd requested a review from joamaki March 14, 2023 10:00
@bimmlerd bimmlerd marked this pull request as ready for review March 14, 2023 10:00
@bimmlerd bimmlerd requested review from a team as code owners March 14, 2023 10:00
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Good change overall, but we should minimize churn wherever we can to reduce backport pain.

pkg/datapath/linux/config/config.go Outdated Show resolved Hide resolved
pkg/endpoint/redirect_test.go Outdated Show resolved Hide resolved
pkg/endpointmanager/manager_test.go Outdated Show resolved Hide resolved
test/controlplane/suite/agent.go Outdated Show resolved Hide resolved
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/move-datapath-types branch from 11afe7f to 187724e Compare March 14, 2023 15:50
@bimmlerd bimmlerd requested a review from ti-mo March 14, 2023 15:52
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/move-datapath-types branch from 187724e to 96f0af1 Compare March 14, 2023 16:27
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

ci-structure got assigned due to trivial change in test, acking just to get it green.

@bimmlerd
Copy link
Member Author

/test

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Hubble changes LGTM, thanks @bimmlerd!

In preparation of moving the datapath module into datapath/cells.go, we
need to break import cycles by moving the interface definitions into
their own package. Reuse the existing datapath/types package for this,
and adjust code and tests everywhere accordingly.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Now that there are no more circular imports, we can move the datapath
cell and associated initialization code to pkg/datapath.

The sysctl stuff should most definitely not live toplevel here, but one
step at a time.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd
Copy link
Member Author

Rebased on master to get the cluster-mesh tests working, was missing the kind setup introduced in #23496.

@bimmlerd
Copy link
Member Author

/test

@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 Mar 16, 2023
@aditighag
Copy link
Member

Strangely, the build-push-prs image job timed out even though there are just two commits in the PR. 😕

@aditighag aditighag merged commit 6d482d3 into cilium:master Mar 16, 2023
39 of 41 checks passed
@bimmlerd bimmlerd deleted the pr/bimmlerd/move-datapath-types branch April 19, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants