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

Controller clean up #25579

Merged
merged 7 commits into from Jun 6, 2023
Merged

Controller clean up #25579

merged 7 commits into from Jun 6, 2023

Conversation

jrajahalme
Copy link
Member

Clean up internals of pkg/controller by:

  • Simplify by using an infinite interval
  • Organize fields by access type
  • Use params.Context
  • Pass parameter updates in the update channel
  • Introduce 'managedController' to simplify field access contract
  • Move TerminationChannel to test
  • Un-export Controller type

@jrajahalme jrajahalme added the kind/cleanup This includes no functional changes. label May 22, 2023
@jrajahalme jrajahalme requested a review from a team as a code owner May 22, 2023 10:24
@jrajahalme jrajahalme requested a review from pippolo84 May 22, 2023 10:24
@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 22, 2023
@jrajahalme jrajahalme marked this pull request as draft May 22, 2023 10:24
@jrajahalme
Copy link
Member Author

/test

@joamaki
Copy link
Contributor

joamaki commented May 22, 2023

Hey, are you aware of pkg/hive/job which is aiming to replace pkg/controller in the long run?
Docs for it in https://docs.cilium.io/en/latest/contributing/development/hive/#job-groups. What's missing vs. pkg/controller is the ability to inspect the jobs and metrics for them. Nothing against cleaning up pkg/controller, but just wanted to make sure you were aware of the job package.

@jrajahalme jrajahalme added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 23, 2023
@jrajahalme
Copy link
Member Author

@joamaki Was not aware of pkg/hive/job, but no harm in that :-)

@jrajahalme jrajahalme marked this pull request as ready for review May 23, 2023 08:24
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Just a nit left inline. Nice cleanup, thanks! 💯

)

var (
// globalStatus is the global status of all controllers
globalStatus = NewManager()
)

type controllerMap map[string]*Controller
type managedController struct {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think it's better to move unexported types to the bottom, after the exported ones, in order to improve readability for the package users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@pippolo84
Copy link
Member

Current CI failures should be solved simply rebasing.

Simplify controller goroutine implementation by using an infinite
interval instead of a series of 10 minute intervals when the controller
function does not need to run.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Make the contract between the manager and a controller more explicit by
documenting it in controller struct definition.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
ControllerParams is passed by value, so it is possible to add cancel to
the given context, to define it if nil.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Pass ControllerParameter updates in the update channel to simplify
code. We already have the channel for update notifications, migth as well
use it to pass the updated parameters to simplify synchronization
between the manager and the controller.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
…ntract

Move controller fields only accessed by the manager to a new
'managedController' type. This hides these fields from the 'controller'
type making documentation easier.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
TerminationChannel function was only used for testing, move it to test file.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

info: patch template saved to `-`
Controller type need not be exported.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

Rebased for CI fixes.

@jrajahalme
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 Jun 6, 2023
@jrajahalme jrajahalme merged commit 59483dd into cilium:main Jun 6, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. 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

3 participants