wrap NTO controller with controller runtime lib

[yanirq]

Refactor cluster node tuning operator to use controller runtime library (release 0.11).

The functionality is internal only and replaces the direct application of a controller with the controller runtime scheme.
This is a refactor to the main invocation of cluster node tuning controller.
The controller and the metrics server are wrapped and started by the controller runtime library.

This is also a preliminary work to set up the stage for moving Performance addons operator under NTO as documented here: openshift/enhancements#867

[yanirq]

@jmencak @dagrayvid
This PR is an alternative to replace a full refactor of NTO's controller presented here: #302
It keeps most of the original code and will save a lot of regressions if proven as a valid solution.
The performance should be tested as well but since the main functionality is kept then the results should not differ from the current implementation of NTO.

[dagrayvid]

As expected this PR seems to perform about the same as the current implementation:

1. Fully idle:

Old implementation:                 Controller-runtime wrapper:
2022-04-02 17:39:25: 101 18         2022-08-02 16:52:44: 54 12
2022-04-02 17:40:25: 103 19         2022-08-02 16:53:44: 56 13
2022-04-02 17:41:25: 105 20         2022-08-02 16:54:44: 57 13
2022-04-02 17:42:25: 105 20         2022-08-02 16:55:44: 60 15
delta:               4   2                               6  3

2. Creating pods with no label matching used.

Old implementation:                 Controller-runtime wrapper:
2022-04-02 17:46:33: 112 22         2022-08-02 17:00:22: 69 18
2022-04-02 17:47:33: 113 22         2022-08-02 17:01:22: 71 18
2022-04-02 17:48:33: 115 23         2022-08-02 17:02:22: 73 19
2022-04-02 17:49:33: 117 23         2022-08-02 17:03:22: 75 19
delta:               5   1                               6 1

3. Creating pods with a profile that matches to the pods (3 master 3 worker cluster)

Old implementation:                 Controller-runtime wrapper:
2022-04-02 17:53:18: 169  33        2022-08-02 17:09:37: 129 27
2022-04-02 17:54:18: 539  56        2022-08-02 17:10:37: 587 48
2022-04-02 17:55:18: 969  72        2022-08-02 17:11:37: 1101 65
2022-04-02 17:56:18: 1488 86        2022-08-02 17:12:37: 1696 80
delta:               1319 53                             1567 53

[yanirq]

@jmencak In case the move to the controller runtime wrapper does not handle this GC, will we still need to keep this cleanup for legacy NTO lock ?
What is the upgrade path for NTO ? can we skip major versions and miss this cleanup ?

[jmencak]

So this cleanup code was added in 4.8. In my opinion we no longer need to keep it since any of the 4.8, 4.9, 4.10 versions will take care of this cleanup. In other words, we don's support direct upgrades from say 4.7->4.11.

[jmencak]

Thank you for the PR, @yanirq ! I had a quick look at this and it looks mostly good to me. Only nits so far, but I will take another look. Also, @dagrayvid , can you take a look?

[jmencak]

So this cleanup code was added in 4.8. In my opinion we no longer need to keep it since any of the 4.8, 4.9, 4.10 versions will take care of this cleanup. In other words, we don's support direct upgrades from say 4.7->4.11.

[dagrayvid]

The changes look good to me after my first look through.

@yanirq @jmencak is the long term plan to maintain the NTO controller as a client-go based controller like this? Or do we intend to rewrite it using the controller-runtime eventually, assuming we can find a way to do so without a significant performance regression?

[yanirq]

The changes look good to me after my first look through.

@yanirq @jmencak is the long term plan to maintain the NTO controller as a client-go based controller like this? Or do we intend to rewrite it using the controller-runtime eventually, assuming we can find a way to do so without a significant performance regression?

The main goal here is to have NTO wrapped by controller runtime in order for us to add PAO, which is already written completely with controller-runtime, under this repository (see openshift/enhancements#867).
When approaching this at first we thought about having a main reconcile loop (controller runtime implemented) that would take care of both NTO and PAO.
In retrospective #302 showed that a full conversion of NTO controller to use controller runtime will need a deeper design change so it would be efficient and and written according to best practices.

With the changes introduced in this PR, the next step would be to add PAO controller with controller runtime manager:

cluster-node-tuning-operator/cmd/cluster-node-tuning-operator/main.go

Line 85 in 86248b9

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{

[jmencak]

So now that we have two options, I wonder how they'd compare performance-wise with PAO already merged in as a controller. Have you tried that, @yanirq? What's the expectation here? At the moment, this PR is the "winner", but will it still be a "winner" when running idle with PAO merged in?

[yanirq]

So now that we have two options, I wonder how they'd compare performance-wise with PAO already merged in as a controller. Have you tried that, @yanirq? What's the expectation here? At the moment, this PR is the "winner", but will it still be a "winner" when running idle with PAO merged in?

The approach of adding PAO would be similar in both PRs. We will add it as a separate controller (with its reconciler) under the manger.
The reconciler ideally should handle only related CRs (Performance Profiles in our case and watch for its created artifacts)

[dagrayvid]

So now that we have two options, I wonder how they'd compare performance-wise with PAO already merged in as a controller. Have you tried that, @yanirq? What's the expectation here? At the moment, this PR is the "winner", but will it still be a "winner" when running idle with PAO merged in?

I had the same thought. I can probably test this using #314.

[yanirq]

So now that we have two options, I wonder how they'd compare performance-wise with PAO already merged in as a controller. Have you tried that, @yanirq? What's the expectation here? At the moment, this PR is the "winner", but will it still be a "winner" when running idle with PAO merged in?

I had the same thought. I can probably test this using #314.

@cynepco3hahue should have a more updated PR for that (at least a WIP one)

[yanirq]

/retest

[dagrayvid]

I had the same thought. I can probably test this using #314.

@cynepco3hahue should have a more updated PR for that (at least a WIP one)

I will wait for an update to PR#314 before testing. Although I don't expect any surprises, I would like to do this test before merging this PR.

[yanirq]

/retest

[openshift-ci]

@yanirq: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[jmencak]

Thank you for the changes, @yanirq . This looks good to me now. Can we squash the commits? @dagrayvid , unless you have objections from your side, I think this is ready to be merged.

[yanirq]

Thank you for the changes, @yanirq . This looks good to me now. Can we squash the commits? @dagrayvid , unless you have objections from your side, I think this is ready to be merged.

@jmencak commits are already squashed

[jmencak]

/lgtm
/approve
/hold
David, please cancel the hold if you have no objections.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmencak, yanirq

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jmencak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dagrayvid]

/unhold