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

Modular daemon and operator #25986

Merged
merged 5 commits into from
Jul 6, 2023
Merged

Modular daemon and operator #25986

merged 5 commits into from
Jul 6, 2023

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Jun 7, 2023 •
edited

Current implementation of both the agent and the operator uses global instances of both cobra.Command and hive.
This makes the agent and the operator less flexible, especially when composing the application externally (e.g: for testing purposes, like in the controlplane test framework).

This PR removes the usage of globals and allows building the agent and operator command with constructors, passing a reference to an external hive.

The removal of the global Viper instance has consequences in other part of the codebase that are also addressed in this PR:

  • refactoring of the agent debuginfo API handler that list all daemon flags and their values
  • refactoring of the controlplane test framework to separate agent and operator configuration for the tests
  • refactoring of the provider-specific flags for the operator variants

Notes for the reviewers: review commit by commit might be simpler.

@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 Jun 7, 2023
@pippolo84 pippolo84 added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 7, 2023
@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 Jun 7, 2023
@pippolo84 pippolo84 added area/daemon Impacts operation of the Cilium daemon. area/docker Impacts the integration with Docker. area/operator Impacts the cilium-operator component area/build Anything to do with the build, more general then area/CI dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 7, 2023
@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 Jun 7, 2023
@pippolo84

This comment was marked as outdated.

Sign in to view
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch 5 times, most recently from b4d6eb9 to 9059ead Compare June 13, 2023 08:45
@pippolo84 pippolo84 marked this pull request as ready for review June 13, 2023 08:57
@pippolo84 pippolo84 requested review from a team as code owners June 13, 2023 08:57
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch from 9059ead to 311af99 Compare June 13, 2023 15:58
@pippolo84
Copy link
Member Author

Force-pushed to resolve a conflict with the main branch.

christarazi
christarazi reviewed Jun 13, 2023
View reviewed changes
test/controlplane/suite/testcase.go Outdated Show resolved Hide resolved
test/controlplane/suite/testcase.go Outdated Show resolved Hide resolved
daemon/cmd/debuginfo.go Outdated Show resolved Hide resolved
test/controlplane/suite/testcase.go Outdated Show resolved Hide resolved
daemon/cmd/debuginfo.go Outdated Show resolved Hide resolved
daemon/cmd/debuginfo.go Outdated Show resolved Hide resolved
daemon/cmd/daemon.go Outdated Show resolved Hide resolved
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch 3 times, most recently from 0fcd052 to 9c8ce76 Compare June 14, 2023 18:48
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch from c39b84d to 13b6d91 Compare June 19, 2023 09:11
@pippolo84 pippolo84 added dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Jun 19, 2023
@nathanjsweet nathanjsweet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 20, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch from 13b6d91 to 089bf83 Compare June 21, 2023 08:26
@pippolo84 pippolo84 removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 21, 2023
@pippolo84
Copy link
Member Author

@nathanjsweet rebased, could you PTAL? 🙏 Thanks!

@nathanjsweet nathanjsweet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 26, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch 2 times, most recently from 2ea77cf to d3fef43 Compare June 26, 2023 15:11
@pippolo84 pippolo84 removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 26, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch 2 times, most recently from 7895f60 to 0495633 Compare June 27, 2023 15:03
@joestringer joestringer removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jul 5, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch from 0495633 to 92d4b89 Compare July 6, 2023 11:28
@pippolo84
operator: Refactor cobra cmd and hive to avoid globals
34eeacf 
Refactor the operator cobra command and hive to avoid globals.
This allows an easier use of the operator hive and/or mock operator
command in tests like the controlplane ones.

Since the operator is built in different variants (one for each
supported provider), the commit adds a hook mechanism for the
provider-specific code to add custom flags.

The controlplane tests rely on the global Viper instance to correctly
setup the operator. Thus, controlplane tests are temporarily disable and
will be fixed in a subsequent commit.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
daemon: Refactor cobra cmd and hive to avoid globals
5acc084 
Refactor the agent cobra command and hive to avoid globals.
This allows an easier use of the agent hive and/or mock agent
command in tests like the controlplane ones.

The debuginfo handler of the agent API has been temporarily commented
out since it depends on the removed global instance of Viper.
This will be fixed in a subsequent commit.

The controlplane tests rely on the global Viper instance to correctly
setup the agent. Thus, relevant lines are temporarily commented out
and will be fixed in a subsequent commit.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
daemon: Fix daemon API debuginfo handler
06f8f86 
Among the debug information collected, the debuginfo API handler lists
all the agent flags and values.  Since a global Viper instance is no
more available, the agent hive provides a cell to share a read-only copy
of all the flags.  Doing that it is possible to guarantee the backward
compatibility of the handler output format.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
api: Fix typo in debuginfo description
dc62615 
Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
test/controlplane: Fix tests after removal of global hives
9c00b80 
Refactor controlplane tests to separate agent and operator configuration
for the test environment.  This is needed since there are no more
global Viper instances to configure the agent and the operator.

The current flow to bootstrap a controlplane test is now:

- SetupEnvironment to set the fake datapath and fake k8s clients
- StartAgent to bootstrap the agent with the test specific config
- StartOperator to bootstrap the operator with the test specific config

Each existing controlplane test is updated accordingly.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/modular-daemon-and-operator branch from 92d4b89 to 9c00b80 Compare July 6, 2023 12:01
@pippolo84
Copy link
Member Author

/test

@pippolo84
Copy link
Member Author

pippolo84 commented Jul 6, 2023 •
edited

ci-clustermesh hit a known flake, rerunning

@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 Jul 6, 2023
@michi-covalent michi-covalent merged commit 940b186 into cilium:main Jul 6, 2023
64 of 65 checks passed
@julianwiedmann
Copy link
Member

@pippolo84 heads-up, looks like this might have introduced CI breakage in main: https://github.com/cilium/cilium/actions/runs/5483476340

@julianwiedmann
Copy link
Member

@pippolo84 heads-up, looks like this might have introduced CI breakage in main: https://github.com/cilium/cilium/actions/runs/5483476340

ah nevermind, Daniel already spotted that it's #22587 (presumably not rebased on top of this PR?).

@pippolo84
Copy link
Member Author

ah nevermind, Daniel already spotted that it's #22587 (presumably not rebased on top of this PR?).

I think so, the reason is that this PR changed the controlplane tests API (specifically, the commit here) to account for the removal of the globals.
Let me figure out if I can suggest the fix to the author of #22587

@pippolo84 pippolo84 mentioned this pull request Jul 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
area/build Anything to do with the build, more general then area/CI area/daemon Impacts operation of the Cilium daemon. area/docker Impacts the integration with Docker. area/modularization 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/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pippolo84 @julianwiedmann @nathanjsweet @christarazi @tklauser @joestringer @michi-covalent