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

hive: Rebase on cilium/hive #32020

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Apr 17, 2024

The general idea is to have pkg/hive contain the cilium-specific adaptations of cilium/hive. Specifically, that's providing certain cells like statedb and jobs directly, and having module decorators creating the right health and logging components.

It's one big commit as it has proven difficult to get intermediate commits to compile (which our CI mandates). The health part needs a bit of love after this, we can probably move healthv2 into hive/health as it's strongly tied to cilium's hive.

The bulk of the changes are mechanical. The categories are:

  1. import path change from cilium/cilium/pkg/hive/{cell,job} to cilium/hive/{cell,job}
  2. giving hive.{Start,Stop,Run} an slog.Logger to provide to the hive,
  3. changes to the health interfaces - Scope and HealthReporter are unified under just Health
  4. modules are provided with a job group scoped to their module id directly, instead of having to create a group from a registry

@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 Apr 17, 2024
@bimmlerd bimmlerd added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related. area/modularization labels Apr 17, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 17, 2024
@bimmlerd bimmlerd force-pushed the pr/joamaki/relocate-hive-2 branch 14 times, most recently from 7d9a222 to 40bf4d9 Compare April 18, 2024 15:10
pkg/hive/hive.go Outdated Show resolved Hide resolved
pkg/hive/hive.go Show resolved Hide resolved
@bimmlerd bimmlerd force-pushed the pr/joamaki/relocate-hive-2 branch 4 times, most recently from 5954750 to 11fbb74 Compare April 24, 2024 13:45
@bimmlerd
Copy link
Member Author

/test

@bimmlerd bimmlerd force-pushed the pr/joamaki/relocate-hive-2 branch 2 times, most recently from ad98280 to 8276d67 Compare April 25, 2024 08:53
@bimmlerd
Copy link
Member Author

/test

@bimmlerd bimmlerd requested a review from a team as a code owner April 25, 2024 14:40
@bimmlerd bimmlerd requested review from glibsm and gandro April 25, 2024 14:41
@bimmlerd bimmlerd force-pushed the pr/joamaki/relocate-hive-2 branch 2 times, most recently from 8241410 to c5b99f9 Compare April 25, 2024 14:46
@bimmlerd
Copy link
Member Author

/test

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.

Thanks so much for cleaning up the imports! I did find some instances in my CODEOWNERs where the imports cleanup went slightly wrong (I have not checked files not owned by me)

pkg/fqdn/dns/dns.go Outdated Show resolved Hide resolved
pkg/fqdn/proxy/ipfamily/ipfamily.go Outdated Show resolved Hide resolved
pkg/ipam/node_test.go Outdated Show resolved Hide resolved
pkg/ipam/node_test.go Outdated Show resolved Hide resolved
pkg/nodediscovery/cell.go Outdated Show resolved Hide resolved
@bimmlerd bimmlerd force-pushed the pr/joamaki/relocate-hive-2 branch 2 times, most recently from 7bb0566 to 7c4cc0b Compare April 25, 2024 15:26
@bimmlerd bimmlerd requested a review from gandro April 25, 2024 15:27
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.

Awesome, this looks good to me now 🚀

@bimmlerd
Copy link
Member Author

/test

Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Hubble related changes lgtm.

Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Woohoo! 🥳. Quick clarifying question for you, but otherwise LGTM for API changes.

daemon/restapi/api_limits_test.go Show resolved Hide resolved
@learnitall
Copy link
Contributor

I did a quick sanity check and I think there are still some places where the old hive package is referenced. Were these left in on purpose? It looks like there are some leftover references in Documentation and comments, as well as some leftover imports to github.com/cilium/cilium/pkg/hive:

$ grep 'pkg/hive' -R . --include \*.go --include \*.rst | wc -l
89

Examples:

./Documentation/contributing/development/hive.rst:`hive.New <https://pkg.go.dev/github.com/cilium/cilium/pkg/hive#New>`_ constructor. 
./vendor/github.com/cilium/hive/hive.go:// See pkg/hive/example for a runnable example application.
./pkg/proxy/logger/logger_test.go:	"github.com/cilium/cilium/pkg/hive"

@bimmlerd
Copy link
Member Author

I did a quick sanity check and I think there are still some places where the old hive package is referenced. Were these left in on purpose? It looks like there are some leftover references in Documentation and comments, as well as some leftover imports to github.com/cilium/cilium/pkg/hive:

$ grep 'pkg/hive' -R . --include \*.go --include \*.rst | wc -l
89

Examples:

./Documentation/contributing/development/hive.rst:`hive.New <https://pkg.go.dev/github.com/cilium/cilium/pkg/hive#New>`_ constructor. 
./vendor/github.com/cilium/hive/hive.go:// See pkg/hive/example for a runnable example application.
./pkg/proxy/logger/logger_test.go:	"github.com/cilium/cilium/pkg/hive"

These are intentional. The PR's purpose is not to remove pkg/hive, but to use the implementation of hive in github.com/cilium/hive. cilium/hive is meant to be usable for projects unrelated to cilium, whereas github.com/cilium/cilium/pkg/hive is the adaptation of hive for the purposes of the cilium agent (see PR description/commit message). pkg/hive/job and pkg/hive/cell are removed from cilium/cilium, hence these import paths had to be rewritten, but not pkg/hive, as, for example, pkg/hive.New is how we inject our default cells into the hive.

@bimmlerd
Copy link
Member Author

/test

Copy link
Contributor

@learnitall learnitall 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, thank you! My apologies for the oversite there 🤦

The general idea is to have `pkg/hive` contain the cilium-specific
adaptations of `cilium/hive`. Specifically, that's providing certain
cells like statedb and jobs directly, and having module decorators
creating the right health and logging components.

It's one big commit as it has proven difficult to get intermediate
commits to compile (which our CI mandates). The health part needs a bit
of love after this, we can probably move `healthv2` into `hive/health`
as it's strongly tied to cilium's hive.

The bulk of the changes are mechanical. The categories are:

1. import path change from cilium/cilium/pkg/hive/{cell,job} to
cilium/hive/{cell,job}
2. giving hive.{Start,Stop,Run} an slog.Logger to provide to the hive,
3. changes to the health interfaces - Scope and HealthReporter are
unified under just Health
4. modules are provided with a job group scoped to their module id
directly, instead of having to create a group from a registry

In addition, a broken test is fixed in api_limits_test. The test was
inadvertently using global state of pflag, and reading the config did
_not_ actually work, but just inherited the existing state from a
previous test.

Finally, in pkg endpoint, due to the removal of cell.GetSubScope, a bit
of health logic is exposed - cell.GetSubScope used to allocate a new
shim scope if the passed parent is `nil`, which it is for EP tests.
Instead of fixing up all the tests which don't pass a health, put that
logic into pkg/endpoint where it's at least visible, and can be cleaned
up if so desired.

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

/test

@aanm aanm merged commit 2311f3d into cilium:main Apr 29, 2024
64 checks passed
@michi-covalent michi-covalent added the hubble-cli PRs or GitHub issues related with hubble-cli label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization hubble-cli PRs or GitHub issues related with hubble-cli kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet