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 down rabbitmqcluster_controller.go #418

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Oct 27, 2020

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

This is a refactor PR which separates logics of controllers/rabbitmqcluster_controller.go. I extracted helper functions according to different concepts/objects that the Reconcile() is operating on.

  • reconcile_finalizer.go
  • reconcile_queue_rebalance.go
  • reconcile_rabbitmq_configurations.go
  • reconcile_status_.go
  • reconcile_tls.go

Each of the file has a corresponding reconcile_*_test.go which contains its integration tests. I add an additional utils.go which contained general helper functions that were used in multiple of the above files. PodExecutor interface is extracted to its own file as well. rabbitmqcluster_controller.go still containers the Reconcile(), SetupWithManager(), and helper function addResourceToIndex, getRabbitmqCluster, getChildResources and logAndRecordOperationResult.

Pros and Cons

I think the controller logic became easier to navigate with this refactor. When we are modifying rabbitmqcluster_controller.go, most of the time we are changing the logic of how the Reconcile() deals with one object/aspects. With this refactor, it should be easier to see where to make changes. Each of these files can grow or shrink themselves without impacting the main controller logic.

The obvious cons will be losing history behind the helper function. However, I would argue that we will eventually do a refactor and the earlier we do it, the more history we can preserve.

Next Step

This is "shift and life" PR that does not change any helper function or any test. I think the logical step next will be to refactor all helper reconcile to have the same contract, and to further move certain logic from Reconcile() to the helpers. Before I head there, I would like to check in with the team first.

Local Testing

Have run unit and integration tests.

- separate according to different k8s/rmq concepts that
the Reconcile function is handling
- all broken down files have a corresponding  *_test.go
file which contains its integration tests
@MirahImage
Copy link
Member

I'm really happy we're doing this, the controller codebase was getting difficult to navigate.

Copy link
Collaborator

@mkuratczyk mkuratczyk left a comment

Choose a reason for hiding this comment

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

Great stuff. I feel like this queue rebalancing could be done in a more generic way (it doesn't really reconciles anything and it's not clear how another post-deploy command would fit into this). But I'm totally happy to take this PR as-is and worry about that when working on #414, which is another post deploy command (but only executed on fresh deployments).

Copy link
Contributor

@ferozjilla ferozjilla left a comment

Choose a reason for hiding this comment

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

LGTM

@ablease
Copy link
Contributor

ablease commented Oct 28, 2020

Thanks for this Chunyi. This solves some of the navigation issues I had with the controller also.

Copy link
Contributor

@coro coro left a comment

Choose a reason for hiding this comment

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

Thanks for this refactor!

@ChunyiLyu ChunyiLyu merged commit 7fdb23c into main Oct 28, 2020
@ChunyiLyu ChunyiLyu deleted the controller-refactors branch October 29, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants