Refactor NTO to use controller runtime lib #302

yanirq · 2021-12-15T10:36:44Z

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.

The new controller(s) structure:

2 Controllers under one manager : CVO controller and Tuned controller using controller runtime library.
Metrics server added as runnable to the controller runtime manager.
Tuned daemon controller left untouched.

Current implementation that might be subject to change depending on the operator performance (or reviews):

Tuned controller does not keep internal maps for tracking node names and pods. It watches pod and node label changes and in the reconcile logic will list all nodes before syncing tuned profiles.
Pod labels watch will run always but will never act on changes if no tuned CR has pod type.

Pending tasks checklist:

Ensure metric reporting works as expected (not only on e2e)
e2e and unit test coverage should be added (either in this PR or the following)
Unused methods will be removed once the PR is in an acceptable review state.
Add profile deletion when a node is deleted
Test managed state
Test operator's performance vs previous implementation
Test all status reporting scenarios
Client-go leader election changes applied correctly

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

openshift-ci · 2021-12-15T10:38:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yanirq
To complete the pull request process, please assign jmencak after the PR has been reviewed.
You can assign the PR to them by writing /assign @jmencak in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

yanirq · 2021-12-27T08:54:41Z

/test e2e-aws-operator

yanirq · 2022-01-05T10:59:55Z

/test e2e-aws-operator

jmencak · 2022-01-10T14:53:43Z

Congrats on the e2e-aws-operator tests passing @yanirq , We will start a review this week.
/cc @dagrayvid

yanirq · 2022-01-10T18:54:39Z

Congrats on the e2e-aws-operator tests passing @yanirq , We will start a review this week. /cc @dagrayvid

All the previous PRs can be closed.
There is a checklist still to be completed but reviews should definitely start asap.
You will notice that there are some//TODOsections with alternatives to the current implementation in some sections.

/cc @cynepco3hahue

yanirq · 2022-01-31T11:41:35Z

/retest

yanirq · 2022-02-02T14:02:34Z

/retest

yanirq · 2022-02-03T09:09:47Z

/retest

yanirq · 2022-02-06T09:48:33Z

/retest

dagrayvid · 2022-02-07T16:41:45Z

@yanirq @jmencak,

I did a bit more testing similar to what Jiri already shared. I tested 3 cases for the old code and the new code: 1. fully idle 2. creating ~1000 pods in the background without any custom profile, 3. creating ~1000 pods in the background with a Profile that matches to those pods. Here are the results:

Fully idle:

Old implementation:                 Controller-runtime rewrite:
2022-04-02 17:39:25: 101 18         2022-04-02 16:36:52: 103 20
2022-04-02 17:40:25: 103 19         2022-04-02 16:37:52: 106 22
2022-04-02 17:41:25: 105 20         2022-04-02 16:38:52: 110 24
2022-04-02 17:42:25: 105 20         2022-04-02 16:39:52: 114 25
delta:               4   2                               11  5

Creating pods with no label matching used. Should be pretty idle…

Old implementation:                 Controller-runtime rewrite:
2022-04-02 17:46:33: 112 22         2022-04-02 16:48:01: 153 44
2022-04-02 17:47:33: 113 22         2022-04-02 16:49:01: 226 55
2022-04-02 17:48:33: 115 23         2022-04-02 16:50:01: 299 67
2022-04-02 17:49:33: 117 23         2022-04-02 16:51:01: 365 76
delta:               5   1                               212 32

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

Old implementation:                 Controller-runtime rewrite:
2022-04-02 17:53:18: 169  33        2022-04-02 17:04:48: 104  21
2022-04-02 17:54:18: 539  56        2022-04-02 17:05:48: 1122 85
2022-04-02 17:55:18: 969  72        2022-04-02 17:06:48: 2959 189
2022-04-02 17:56:18: 1488 86        2022-04-02 17:07:48: 5740 299
delta:               1319 53                             5636 278

This confirms that the controller-runtime implementation is doing some work when pods are created even when the pod label matching functionality is unused. It also shows that when the pod label matching functionality is used, the new implementation is using many more CPU cycles than the old implementation.

yanirq · 2022-02-08T19:45:28Z

/retest

openshift-ci · 2022-02-08T21:50:32Z

@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.

yanirq · 2022-02-09T08:34:01Z

/hold - #316 is examined as an alternative for this PR

camilamacedo86 · 2022-02-25T18:37:40Z

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

+			LeaderElection:          true,
+			LeaderElectionID:        config.OperatorLockName,
+			LeaderElectionNamespace: ntoNamespace,
+			LeaseDuration:           &le.LeaseDuration.Duration,


Why are you defining a lease duration? see:

. // LeaseDuration is the duration that non-leader candidates will
// wait to force acquire leadership. This is measured against time of
// last observed ack. Default is 15 seconds.
LeaseDuration *time.Duration

Is not the default time good enough?

this was to keep the original behavior before the refactor

camilamacedo86 · 2022-02-25T18:42:52Z

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

+		restConfig := ctrl.GetConfigOrDie()
+		le := util.GetLeaderElectionConfig(restConfig, enableLeaderElection)
+		mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
+			NewCache:                cache.MultiNamespacedCacheBuilder(namespaces),


Why would you like to use this option?
How many ns would you like to inform here?

See that if you try to cache all -1 or 2 namespaces on the cluster then, you will probably check performance issues.
Please, check the comment: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/cache/multi_namespace_cache.go#L40-L46

the main reason here was to watch cluster resources such as nodes and pods and have a distinct namespace for NTO to be used for filtering

camilamacedo86 · 2022-02-25T18:55:37Z

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

+			LeaseDuration:           &le.LeaseDuration.Duration,
+			RetryPeriod:             &le.RetryPeriod.Duration,
+			RenewDeadline:           &le.RenewDeadline.Duration,
+			Namespace:               ntoNamespace,


You are passing an NS and NewCache: cache.MultiNamespacedCacheBuilder(namespaces)
Note that you need to have permissions ( scope ) to read/update/delete resources and in this way cache them where the operator will be installed. Then, you can:

a) Watching/Catching resources in a set of Namespaces
It is possible to use MultiNamespacedCacheBuilder from Options to watch and manage resources in a set of Namespaces.

OR

b) Watching/Catching resources in a single Namespace (where the. operator will be installed) by using the Namespace option. See:

// Namespace if specified restricts the manager's cache to watch objects in
// the desired namespace Defaults to all namespaces
//
// Note: If a namespace is specified, controllers can still Watch for a
// cluster-scoped resource (e.g Node). For namespaced resources the cache
// will only hold objects from the desired namespace.
Namespace string

Ref: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.1/pkg/manager#Manager

OR

c) Do not add both which means grant cluster-scope permission for the project. Your operator will be watching/catching the whole cluster

Be aware that how much more resources/namespaces do you catching/watching more resources you will consume.

camilamacedo86 · 2022-02-25T18:58:44Z

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

+			RetryPeriod:             &le.RetryPeriod.Duration,
+			RenewDeadline:           &le.RenewDeadline.Duration,
+			Namespace:               ntoNamespace,
+		})


By default SDK/Kubebuilder scaffold the projects as cluster-scope, see: https://github.com/operator-framework/operator-sdk/blob/master/testdata/go/v3/memcached-operator/main.go#L68-L79

But you can change the. scope: https://sdk.operatorframework.io/docs/building-operators/golang/operator-scope/

camilamacedo86 · 2022-02-25T19:03:09Z

manifests/40-rbac.yaml

@@ -34,10 +34,10 @@ rules:
 - apiGroups: ["security.openshift.io"]


by adopting SDK/Kubebuilder you will be able to work with makers, e.g: https://github.com/operator-framework/operator-sdk/blob/master/testdata/go/v3/memcached-operator/controllers/memcached_controller.go#L44-L48

Then, when you run make generate the RBAC will be generated at the config]/rbac/ dir: https://github.com/operator-framework/operator-sdk/tree/master/testdata/go/v3/memcached-operator/config/rbac

Also, you can use make bundle and have the whole OLM bundle generated by you with all your kustomize configs, see: https://github.com/operator-framework/operator-sdk/tree/master/testdata/go/v3/memcached-operator/bundle

That is very helpful to work with the releases and provide the solutions for OLM.
You can also add customizations on in your base CSV: operator-sdk/testdata/go/v3/memcached-operator/config/manifests/bases/

To know more about the default layout see: https://sdk.operatorframework.io/docs/overview/project-layout/

camilamacedo86 · 2022-02-25T19:10:54Z

pkg/operator/mc.go

@@ -1,13 +1,15 @@
 package operator


What means mc.go?
What is its purpose?

camilamacedo86 · 2022-02-25T19:14:15Z

pkg/operator/reconciler.go

+			return e.Object.GetName() == tunedv1.TunedClusterOperatorResourceName
+		},
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			if !validateUpdateEvent(&e) {


Are you doing it to address the scenarios that the reconciliation fails because the resource changed on the cluster? If yes, I'd suggest using the client and fetching the resource that you want to change/update before calling the update. Then, if it fails return err in the reconciliation to ensure that it will be executed again.

camilamacedo86 · 2022-02-25T19:26:32Z

pkg/util/leaderelection.go

@@ -10,22 +10,26 @@ import (
 	"k8s.io/klog"
 )

-func GetLeaderElectionConfig(ctx context.Context, restcfg *rest.Config) configv1.LeaderElection {
+// GetLeaderElectionConfig returns leader election configs defaults based on the cluster topology
+func GetLeaderElectionConfig(restcfg *rest.Config, enabled bool) configv1.LeaderElection {


Why do you need this ultis for leader election?
By default, you have the C+R implementation for leader election : https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/leaderelection/leader_election.go

What is the extra requirement on top of that?

Also, you might give a look in the doc https://sdk.operatorframework.io/docs/building-operators/golang/advanced-topics/#leader-election which has compressive info about it.

camilamacedo86 · 2022-02-25T19:36:42Z

go.mod

@@ -24,10 +24,12 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.30.0
 	k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b
+	sigs.k8s.io/controller-runtime v0.11.0


Also, see; wdyt about check it with the bug fix: https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.11.1

camilamacedo86 · 2022-02-25T19:38:55Z

go.mod

+	sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.11.0
+	sigs.k8s.io/controller-tools => sigs.k8s.io/controller-tools v0.7.0


why do you need these replaces?
Why do you import sigs.k8s.io/controller-tools?

yanirq · 2022-02-27T17:28:08Z

@camilamacedo86 Thank you for the extensive review, it is super informative and helpful (also for deeper understanding of controller runtime "under the hood").
This PR will probably be closed soon since we switched to an alternative solution but it will be kept in case we do choose to use this refactor in the future.
Some of the information is insightful also for the current implementation so thanks again ! I will answer the comments here that could be addressed.

openshift-ci · 2022-02-27T17:28:16Z

@yanirq: PR needs rebase.

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.

camilamacedo86 · 2022-02-28T04:55:14Z

Hi @yanirq,

#302 (comment)
Thank you for your reply. I hope that it can help out.

Feel free to reach out if you need.

yanirq · 2022-03-09T10:12:13Z

This PR will not be the format we will be using.
The solution is to copy over the PAO code and add it to the manager as a runnable instead of refactoring whole of NTO to use controller runtime.
The PR can be reopened or use as a reference if needed.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 15, 2021

openshift-ci bot requested review from ArangoGutierrez and jmencak December 15, 2021 10:38

yanirq force-pushed the refatcor_cr branch 2 times, most recently from 9c57e0c to ffe2c34 Compare December 20, 2021 11:05

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2021

yanirq force-pushed the refatcor_cr branch 4 times, most recently from f8b874c to 98e56a8 Compare December 26, 2021 18:21

yanirq force-pushed the refatcor_cr branch 4 times, most recently from 980b80f to df6183a Compare December 28, 2021 09:54

yanirq force-pushed the refatcor_cr branch 4 times, most recently from cd47e3c to f307fb9 Compare January 4, 2022 18:51

openshift-ci bot requested a review from dagrayvid January 10, 2022 14:53

yanirq force-pushed the refatcor_cr branch from c84b6a5 to b15d7d9 Compare January 10, 2022 18:51

openshift-ci bot requested a review from cynepco3hahue January 10, 2022 18:54

This was referenced Jan 10, 2022

WIP: Refactor controller runtime #289

Closed

WIP: add the performance-addon-operator code #262

Closed

yanirq changed the title ~~WIP: Refactor NTO to use controller runtime lib~~ Refactor NTO to use controller runtime lib Jan 10, 2022

rebase and align to openshift#312

73ad6a5

yanirq force-pushed the refatcor_cr branch from 5d36543 to 73ad6a5 Compare January 30, 2022 16:47

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2022

move status handling under one controller

62e9460

yanirq mentioned this pull request Feb 8, 2022

wrap NTO controller with controller runtime lib #316

Merged

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2022

camilamacedo86 reviewed Feb 25, 2022

View reviewed changes

pkg/operator/mc.go

@@ -1,13 +1,15 @@

package operator

Copy link

camilamacedo86 Feb 25, 2022

Choose a reason for hiding this comment

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

What means mc.go?
What is its purpose?

camilamacedo86 reviewed Feb 25, 2022

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2022

yanirq closed this Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor NTO to use controller runtime lib #302

Refactor NTO to use controller runtime lib #302

yanirq commented Dec 15, 2021 •

edited

Loading

openshift-ci bot commented Dec 15, 2021

yanirq commented Dec 27, 2021

yanirq commented Jan 5, 2022

jmencak commented Jan 10, 2022

yanirq commented Jan 10, 2022 •

edited

Loading

yanirq commented Jan 31, 2022

yanirq commented Feb 2, 2022

yanirq commented Feb 3, 2022

yanirq commented Feb 6, 2022

dagrayvid commented Feb 7, 2022

yanirq commented Feb 8, 2022

openshift-ci bot commented Feb 8, 2022

yanirq commented Feb 9, 2022

camilamacedo86 Feb 25, 2022 •

edited

Loading

yanirq Feb 27, 2022

camilamacedo86 Feb 25, 2022 •

edited

Loading

yanirq Feb 27, 2022

camilamacedo86 Feb 25, 2022

camilamacedo86 Feb 25, 2022

camilamacedo86 Feb 25, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022

camilamacedo86 Feb 25, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022

camilamacedo86 Feb 25, 2022

camilamacedo86 Feb 25, 2022

yanirq commented Feb 27, 2022

openshift-ci bot commented Feb 27, 2022

camilamacedo86 commented Feb 28, 2022

yanirq commented Mar 9, 2022

		@@ -34,10 +34,10 @@ rules:
		- apiGroups: ["security.openshift.io"]

		sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.11.0
		sigs.k8s.io/controller-tools => sigs.k8s.io/controller-tools v0.7.0

Refactor NTO to use controller runtime lib #302

Refactor NTO to use controller runtime lib #302

Conversation

yanirq commented Dec 15, 2021 • edited Loading

openshift-ci bot commented Dec 15, 2021

yanirq commented Dec 27, 2021

yanirq commented Jan 5, 2022

jmencak commented Jan 10, 2022

yanirq commented Jan 10, 2022 • edited Loading

yanirq commented Jan 31, 2022

yanirq commented Feb 2, 2022

yanirq commented Feb 3, 2022

yanirq commented Feb 6, 2022

dagrayvid commented Feb 7, 2022

yanirq commented Feb 8, 2022

openshift-ci bot commented Feb 8, 2022

yanirq commented Feb 9, 2022

camilamacedo86 Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanirq commented Feb 27, 2022

openshift-ci bot commented Feb 27, 2022

camilamacedo86 commented Feb 28, 2022

yanirq commented Mar 9, 2022

yanirq commented Dec 15, 2021 •

edited

Loading

yanirq commented Jan 10, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022 •

edited

Loading

camilamacedo86 Feb 25, 2022 •

edited

Loading