Skip to content

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

Closed
cynepco3hahue wants to merge 4 commits intoopenshift:masterfrom
cynepco3hahue:pao
Closed

WIP: add the performance-addon-operator code#262
cynepco3hahue wants to merge 4 commits intoopenshift:masterfrom
cynepco3hahue:pao

Conversation

@cynepco3hahue
Copy link

@cynepco3hahue cynepco3hahue commented Aug 18, 2021

TODO:

  • e2e tests, not all of them but at least part of them.
  • add owners file for the PAO component
  • disabling the PAO operator based on a ManagementState
  • reporting PAO operator status via co/node-tuning

@openshift-ci 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 Aug 18, 2021
@openshift-ci openshift-ci bot requested review from dagrayvid and jmencak August 18, 2021 13:04
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cynepco3hahue
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.

Details Needs approval from an approver in each of these files:

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

Copy link
Contributor

@jmencak jmencak left a comment

Choose a reason for hiding this comment

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

Thanks you for the PR, Artyom, it is a good start. In order to pass the verification, you need to fix your formatting. See "make verify" and it will tell you which files to fix.

I haven't gone through all of the code, just the interface and added a few comments. PAO commpiled fine. I'd really like to get rid of the dependency on sigs.k8s.io/controller-runtime if possible.

k8s.io/metrics => k8s.io/metrics v0.21.1
k8s.io/mount-utils => k8s.io/mount-utils v0.21.2
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.21.1
sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.9.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need sigs.k8s.io/controller-runtime?

Copy link
Author

Choose a reason for hiding this comment

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

We are using it a lot under the PAO controller. In general, it makes it much easier to write a controller with it, without the need to specify separate clients for each API type, informers, and listers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It took me quite a bit of effort to get away from operator-sdk and controller-runtime in the past. Perhaps things are better now, but it used to be quite hard (nearly impossible) to keep other k8s dependencies up-to-date when I was using those two dependencies in the past. How easy/difficult is it now? I.e. use the current 1.22 k8s deps?

items:
description: ProfileStatusCondition represents a partial state of
the per-node Profile application.
- additionalPrinterColumns:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this file changed?

Copy link
Author

Choose a reason for hiding this comment

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

I just ran the make crd-schema-gen.

rm -rf $(TUNED_DIR)/.git)

build: $(BINDATA) pkg/generated
build: $(BINDATA) generate-deepcopy pkg/generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps at some point add performance-addon-operator-bin target?

Copy link
Author

Choose a reason for hiding this comment

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

For sure, I will do it once will add PAO dockerfiles.

@jmencak
Copy link
Contributor

jmencak commented Aug 18, 2021

$ make verify
verify-gofmt: ERROR: gofmt failed on the following files:
./pkg/apis/performanceprofile/v2/register.go ./pkg/performance-addon-controller/components/manifestset/manifestset.go ./pkg/performance-addon-controller/components/tuned/tuned.go ./pkg/performance-addon-controller/performanceprofile_controller_suite_test.go ./pkg/performance-addon-controller/performanceprofile_controller_test.go ./pkg/performance-addon-controller/status.go

For details, run: gofmt -d -s ./pkg/apis/performanceprofile/v2/register.go ./pkg/performance-addon-controller/components/manifestset/manifestset.go ./pkg/performance-addon-controller/components/tuned/tuned.go ./pkg/performance-addon-controller/performanceprofile_controller_suite_test.go ./pkg/performance-addon-controller/performanceprofile_controller_test.go ./pkg/performance-addon-controller/status.go

make: *** [Makefile:116: verify-gofmt] Error 1

@jmencak
Copy link
Contributor

jmencak commented Aug 18, 2021

Another thing we should consider is modifying the OWNERS file to add approvers from the PAO team.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2021
@jmencak
Copy link
Contributor

jmencak commented Aug 19, 2021

Another two issues to solve that currently come to mind:

@cynepco3hahue
Copy link
Author

Thanks you for the PR, Artyom, it is a good start. In order to pass the verification, you need to fix your formatting. See "make verify" and it will tell you which files to fix.

I haven't gone through all of the code, just the interface and added a few comments. PAO commpiled fine. I'd really like to get rid of the dependency on sigs.k8s.io/controller-runtime if possible.

A lot of thanks for the initial review, we are using a lot sigs.k8s.io/controller-runtime under the PAO controller, it makes it much easier to configure the controller and I can see it pretty popular among OpenShift repositories.

Artyom Lukianov added 4 commits August 19, 2021 11:49
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 19, 2021

@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 Rerun command
ci/prow/e2e-upgrade bd2d691 link /test e2e-upgrade

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
Copy link
Contributor

openshift-ci bot commented Aug 25, 2021

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

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2021
@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 23, 2021
@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 23, 2021
@yanirq
Copy link
Contributor

yanirq commented Jan 10, 2022

@cynepco3hahue you can close this PR in favor of #302 or keep it for additional references in the near future (maybe even as closed if we still have the code)

@cynepco3hahue
Copy link
Author

close in favor of #302.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments