Vendor and run performance-addon-operator as part of the controller-runtime NTO manager by cynepco3hahue · Pull Request #314 · openshift/cluster-node-tuning-operator

cynepco3hahue · 2022-02-06T13:06:17Z

No description provided.

openshift-ci · 2022-02-06T13:06:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

jmencak · 2022-02-08T13:29:23Z

/cc @jmencak

jmencak · 2022-02-08T13:29:49Z

/uncc @ArangoGutierrez

cynepco3hahue · 2022-02-14T09:00:41Z

/retest

cynepco3hahue · 2022-02-14T10:58:18Z

/retest

yanirq · 2022-02-14T12:23:04Z

@dagrayvid we can start testing the performance of this combination of PAO and NTO with this PR (based on #316)
The manifests in this folder should be applied manually (including the new additions).

dagrayvid · 2022-02-14T15:48:14Z

manifests/40-webhook-configuration.yaml

@@ -0,0 +1,55 @@
+---


We should probably increase the prefix number for this and the following files by 10. Or perhaps use 45-webhook-configuration.yaml if changing the name of the other manifests could cause problems?

yee, I wanted to avoid the renaming, 45 will work for me

dagrayvid · 2022-02-14T16:56:16Z

This PR performs very similarly to the current implementation of NTO. As we expected, the addition of the PAO control loop has no impact when there are no PerformanceProfiles to reconcile.

Fully idle:

Old implementation:                 pr-314:
2022-04-02 17:39:25: 101 18         2022-14-02 11:27:42: 45 9
2022-04-02 17:40:25: 103 19         2022-14-02 11:28:42: 46 10
2022-04-02 17:41:25: 105 20         2022-14-02 11:29:42: 48 10
2022-04-02 17:42:25: 105 20         2022-14-02 11:30:42: 49 12
delta:               4   2                               4  3

Creating pods with no label matching used.

Old implementation:                 pr-314:
2022-04-02 17:46:33: 112 22         2022-14-02 11:32:54: 52 13
2022-04-02 17:47:33: 113 22         2022-14-02 11:33:54: 53 13
2022-04-02 17:48:33: 115 23         2022-14-02 11:34:54: 54 14
2022-04-02 17:49:33: 117 23         2022-14-02 11:35:54: 56 14
delta:               5   1                               4 1

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

Old implementation:                 pr-314:
2022-04-02 17:53:18: 169  33        2022-14-02 11:43:02: 130 28
2022-04-02 17:54:18: 539  56        2022-14-02 11:44:02: 501 51
2022-04-02 17:55:18: 969  72        2022-14-02 11:45:02: 981 71
2022-04-02 17:56:18: 1488 86        2022-14-02 11:46:02: 1547 86
delta:               1319 53                             1417 58

With this, I am comfortable merging #316. @jmencak, WDYT?

jmencak · 2022-02-14T17:19:52Z

With this, I am comfortable merging #316. @jmencak, WDYT?

This looks good. Do we have any data wrt. memory usage?

dagrayvid · 2022-02-14T17:37:41Z

This looks good. Do we have any data wrt. memory usage?

I haven't looked at memory usage yet. I can probably gather some data on this today if needed. Is memory usage as much a concern as CPU usage for core OCP operators?

jmencak · 2022-02-14T18:11:19Z

This looks good. Do we have any data wrt. memory usage?

I haven't looked at memory usage yet. I can probably gather some data on this today if needed. Is memory usage as much a concern as CPU usage for core OCP operators?

Not as much as CPU usage, but it would be nice to know.

dagrayvid · 2022-02-14T23:17:28Z

I ran the first and third test cases to get an idea of the memory usage before / after these changes.

It's probably easiest to just share it with a graph of memory usage over time. There is a line for fully idle, and then a line for memory over time while creating 1000 pods using pod-label matching, and then deleting them to see if the usage goes back down to where it was before creating any pods.

One note: In case you wonder why the idle memory usage is lower than the memory usage at the start of the pod creation case. I created the custom profile before starting the second test case, without creating any matching pods. This jump was seen in both the new and old implementations (corrected from previous comment)

cynepco3hahue · 2022-02-15T13:13:22Z

/retest

cynepco3hahue · 2022-02-15T14:38:20Z

/retest

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

jmencak

Thank you Artyom! Untested (yet), but the changes look good to me. Only nits right now.

jmencak · 2022-02-23T09:59:57Z

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

+			Scheme:   mgr.GetScheme(),
+			Recorder: mgr.GetEventRecorderFor("performance-profile-controller"),
+		}).SetupWithManager(mgr); err != nil {
+			klog.Exitf("unable to create PerformanceProfile controller : %v", err)


Consistency. Not sure about PAO, but in NTO, we don't have spaces after text in text: %v. Can we keep it that way? Also, I notice you don't have a space in a similar message above.

typo, done :)

jmencak · 2022-02-23T10:01:32Z

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

+		webHookServer.KeyName = webhookKeyName
+
+		if err = (&performancev2.PerformanceProfile{}).SetupWebhookWithManager(mgr); err != nil {
+			klog.Exitf("unable to create PerformanceProfile v2 webhook : %v", err)


Nit, as noted above.

jmencak · 2022-02-23T10:08:14Z

manifests/40-rbac.yaml

 - apiGroups: ["coordination.k8s.io"]
  resources: ["leases"]
  verbs: ["create","get","update","patch"]
+- apiGroups: ["node.k8s.io"]


In the past, I was asked by ProdSec to review RBAC. After the review, I added a few comments here. Can we also do this for PAO? Especially here for runtimeclasses. performance.openshift.io should be clear to all.

jmencak · 2022-02-23T10:13:13Z

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

 		controller, err := operator.NewController()
 		if err != nil {
-			klog.Fatal(err)
+			klog.Fatalf("failed to create new controller %v", err)


Consistency. s/controller %v/controller: %v/ ?

jmencak · 2022-02-23T10:15:51Z

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

+			klog.Fatalf("failed to create new controller %v", err)
+		}
+
+		if err := mgr.Add(controller); err != nil {


Thank you for doing this, we missed this in the earlier review. :/

yanirq · 2022-02-24T08:32:12Z

@cynepco3hahue in the current vendoring PAO still uses openshift-performance-addon-operator namespace:

https://github.com/openshift-kni/performance-addon-operators/blob/c61353605e60f48ac75432a37489d278d32eb4ce/controllers/performanceprofile_controller.go#L227

If we will keep it that way we also need to add the namespace to the manifests.

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

cynepco3hahue · 2022-02-28T13:38:08Z

@cynepco3hahue in the current vendoring PAO still uses openshift-performance-addon-operator namespace:

https://github.com/openshift-kni/performance-addon-operators/blob/c61353605e60f48ac75432a37489d278d32eb4ce/controllers/performanceprofile_controller.go#L227

If we will keep it that way we also need to add the namespace to the manifests.

It should not affect us, it is used only to generate correct manifests automatically. We are adding manifests manually so all kube builder thing is not relevant for us.

cynepco3hahue · 2022-02-28T13:38:47Z

/hold waiting for upgrade handling under the PAO.

openshift-ci · 2022-03-28T04:35:11Z

@cynepco3hahue: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-pao	`d4fd838`	link	true	`/test e2e-gcp-pao`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci · 2022-03-28T04:35:18Z

@cynepco3hahue: PR needs rebase.

Details

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.

cynepco3hahue · 2022-03-28T10:28:04Z

not relevant anymore

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2022

openshift-ci bot requested review from ArangoGutierrez and dagrayvid February 6, 2022 13:06

openshift-ci bot requested review from jmencak and removed request for ArangoGutierrez February 8, 2022 13:29

cynepco3hahue force-pushed the pao_integration branch from cb945d0 to 8d15fc6 Compare February 8, 2022 18:30

dagrayvid mentioned this pull request Feb 10, 2022

wrap NTO controller with controller runtime lib #316

Merged

cynepco3hahue force-pushed the pao_integration branch 2 times, most recently from dee875d to de53168 Compare February 13, 2022 15:22

cynepco3hahue force-pushed the pao_integration branch from de53168 to d78df03 Compare February 14, 2022 12:40

dagrayvid reviewed Feb 14, 2022

View reviewed changes

cynepco3hahue force-pushed the pao_integration branch from d78df03 to f9b3284 Compare February 15, 2022 11:12

Vendor performance-addon-operator

6b8c893

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

cynepco3hahue force-pushed the pao_integration branch from f9b3284 to 59ad9ae Compare February 15, 2022 15:00

jmencak reviewed Feb 23, 2022

View reviewed changes

Add performance-addon-operator as part of the nto manager

e715a8f

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

manifests: provide entities required by the performance-addon-operator

d4fd838

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

cynepco3hahue force-pushed the pao_integration branch from 59ad9ae to d4fd838 Compare February 28, 2022 13:36

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 28, 2022

cynepco3hahue changed the title ~~WIP: vendor and run performance-addon-operator as part of the controller-runtime NTO manager~~ Vendor and run performance-addon-operator as part of the controller-runtime NTO manager Feb 28, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2022

yanirq mentioned this pull request Feb 28, 2022

WIP: Remove OLM operator on startup openshift-kni/performance-addon-operators#857

Closed

yanirq mentioned this pull request Mar 15, 2022

Performance addon operator code base move to NTO #322

Merged

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

cynepco3hahue closed this Mar 28, 2022

Conversation

cynepco3hahue commented Feb 6, 2022

Uh oh!

openshift-ci bot commented Feb 6, 2022

Uh oh!

jmencak commented Feb 8, 2022

Uh oh!

jmencak commented Feb 8, 2022

Uh oh!

cynepco3hahue commented Feb 14, 2022

Uh oh!

cynepco3hahue commented Feb 14, 2022

Uh oh!

yanirq commented Feb 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cynepco3hahue Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagrayvid commented Feb 14, 2022

Uh oh!

jmencak commented Feb 14, 2022

Uh oh!

dagrayvid commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmencak commented Feb 14, 2022

Uh oh!

dagrayvid commented Feb 14, 2022

Uh oh!

cynepco3hahue commented Feb 15, 2022

Uh oh!

cynepco3hahue commented Feb 15, 2022

Uh oh!

jmencak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanirq commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cynepco3hahue commented Feb 28, 2022

Uh oh!

cynepco3hahue commented Feb 28, 2022

Uh oh!

openshift-ci bot commented Mar 28, 2022

Uh oh!

openshift-ci bot commented Mar 28, 2022

Uh oh!

cynepco3hahue commented Mar 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

cynepco3hahue Feb 14, 2022 •

edited

Loading

dagrayvid commented Feb 14, 2022 •

edited

Loading

yanirq commented Feb 24, 2022 •

edited

Loading