Performance profile render command integration by MarSik · Pull Request #324 · openshift/cluster-node-tuning-operator

MarSik · 2022-03-24T15:38:12Z

The original PAO code supports an offline render mode where it reads the PerformanceProfile manifest from a file and generates all the outputs as files. This is useful for Day 0 and GitOps based installations.

This PR is work in progress and depends on #322

MarSik · 2022-03-24T15:51:31Z

@jmencak Hi, this PR introduces cobra to the main.go file. PAO has the dependency anyway, but I changed the way flags are parsed. Would this approach be acceptable?

The render command is then executed as

podman run -it cluster-node-tuning-operator:4.11 render --help

When no subcommand is used, NTO starts as usual.

jmencak

Thank you for the PR. I think this approach is acceptable even though I'm far from thrilled by the cobra usage. But as you say, we already have the dependency. The PR needs cleaning though:

Can we have back the help for both cluster-node-tuning-operator and openshift-tuned that would show it can be used with the -v options?
-v options should work!
Also, some comments about the usage for openshift-tuned would be welcome as you already added them for cluster-node-tuning-operator

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

MarSik · 2022-03-31T09:35:41Z

/retest

MarSik · 2022-04-07T16:34:07Z

/retest

jmencak

Thank you for the PR and the changes. While the e2e tests pass, the -v=<number> option for the operator itself doesn't work. This needs to be fixed prior the merge.

$ ./cluster-node-tuning-operator -v=2
Error: invalid argument "2" for "-v, --version" flag: strconv.ParseBool: parsing "2": invalid syntax
Usage:
  cluster-node-tuning-operator [flags]
  cluster-node-tuning-operator [command]

Available Commands:
  completion  generate the autocompletion script for the specified shell
  help        Help about any command
  render      Render performance-addon-operator manifests

Flags:
      --enable-leader-election   Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager. (default true)
  -h, --help                     help for cluster-node-tuning-operator
  -v, --version                  Show program version and exit.

Use "cluster-node-tuning-operator [command] --help" for more information about a command.

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

jmencak · 2022-04-08T09:12:31Z

test/e2e/performanceprofile/testdata/render-expected-output/manual_tuned.yaml

    operand:
-      debug: false
+      tunedConfig:
+        reapply_sysctl: null


reapply_sysctl is a Boolean, why does it have null value?

You tell me :) This is the value the test is seeing, I only updated the expected value for it to pass as PAO does not care about this part.

I might tell you, but I have no idea about the test you're talking about. :) I guess it is null because tunedConfig was not specified at all? Good to know it was manually updated by you.

This is the expected value of Tuned when PerfProfile is processed by the render stage. If you check couple of lines above this, we mostly care about the generated tuned profile. The rest is just what NTO structures serialize to by default.

yanirq · 2022-04-10T08:24:32Z

Makefile

 	done

+.PHONY: test-e2e-local
+test-e2e-local:


Should we make the make target more specific as in

cluster-node-tuning-operator/Makefile

Line 202 in 1316041

performance-profile-creator-tests: build-performance-profile-creator

?

I was wondering if we can have more tests that are e2e in nature, but do not require a running cluster.. but the name is not important obviously.

if we are planing to add a render lane in openshift/release then it should correspond to the make target

@MarSik we currently have https://github.com/openshift/cluster-node-tuning-operator/blob/master/hack/run-render-command-functests.sh under this repo. If this target comes to replace the invocation of this script like we did in PAO then we can remove the script (although the script contains some additional decorations)

yanirq · 2022-04-11T07:59:56Z

@MarSik please make sure to also update this section:
https://github.com/openshift/cluster-node-tuning-operator/blob/master/docs/performanceprofile/performance_addon.md#render-mode

openshift-ci · 2022-04-11T08:58:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [MarSik]

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

yanirq · 2022-04-11T10:16:31Z

docs/performanceprofile/performance_addon.md

 Build and invoke the binary
 ```
-build/_output/bin/performance-addon-operators render (FIXME)
+_output/cluster-node-tuning-operator render (FIXME)


we can remove the (FIXME) part, I placed it specifically for this PR

jmencak

The -v options seem to work now. Once the nits:

(FIXME)
s/tuned/TuneD/

are addressed and the commits squashed, this is good to go from my side.

This integrates the render mode for offline processing of the PerformanceProfile. Part of this change is the move of command line processing code to the cobra library.

MarSik · 2022-04-12T15:42:51Z

/retest

openshift-ci · 2022-04-12T17:52:56Z

@MarSik: all tests passed!

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.

MarSik · 2022-04-13T06:54:04Z

@yanirq @jmencak It seems all is ready for the final check now.

yanirq · 2022-04-13T07:33:31Z

looks good to me.
I will add a CI lane for "e2e-no-cluster" under openshift/release.
@jmencak feel free to give the final ack

jmencak · 2022-04-13T07:37:11Z

/lgtm

This integrates the render mode for offline processing of the PerformanceProfile. Part of this change is the move of command line processing code to the cobra library.

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 Mar 24, 2022

openshift-ci bot requested review from ArangoGutierrez and kpouget March 24, 2022 15:40

jlojosnegros mentioned this pull request Mar 25, 2022

PerformanceProfileCreator build and functional test execution adaptation. #325

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

MarSik force-pushed the pao-render branch from 0ec6b78 to d2e302e Compare March 28, 2022 07:22

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

jmencak reviewed Mar 28, 2022

View reviewed changes

cmd/cluster-node-tuning-operator/main.go Outdated Show resolved Hide resolved

cmd/cluster-node-tuning-operator/main.go Outdated Show resolved Hide resolved

jlojosnegros mentioned this pull request Mar 28, 2022

Add ci-ppc test to NTO as part of PAO migration openshift/release#27342

Merged

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

MarSik force-pushed the pao-render branch from 2879338 to 5a4f6dc Compare April 7, 2022 09:52

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

MarSik force-pushed the pao-render branch from 5a4f6dc to 2879338 Compare April 7, 2022 10:28

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

MarSik force-pushed the pao-render branch from 2879338 to c9c0edd Compare April 7, 2022 10:45

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

MarSik changed the title ~~WIP Performance profile render command integration~~ Performance profile render command integration Apr 7, 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 Apr 7, 2022

jmencak suggested changes Apr 8, 2022

View reviewed changes

openshift-ci bot assigned jmencak Apr 8, 2022

yanirq reviewed Apr 10, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2022

yanirq reviewed Apr 11, 2022

View reviewed changes

jmencak reviewed Apr 11, 2022

View reviewed changes

Add performance profile render command line sub-command

9f00192

This integrates the render mode for offline processing of the PerformanceProfile. Part of this change is the move of command line processing code to the cobra library.

MarSik force-pushed the pao-render branch from f8ad5ab to 9f00192 Compare April 12, 2022 12:21

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022

openshift-merge-robot merged commit 0352d62 into openshift:master Apr 13, 2022

Comments

Conversation

MarSik commented Mar 24, 2022

Uh oh!

MarSik commented Mar 24, 2022

Uh oh!

jmencak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarSik commented Mar 31, 2022

Uh oh!

MarSik commented Apr 7, 2022

Uh oh!

jmencak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 Apr 11, 2022

Uh oh!

openshift-ci bot commented Apr 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmencak left a comment

Choose a reason for hiding this comment

Uh oh!

MarSik commented Apr 12, 2022

Uh oh!

openshift-ci bot commented Apr 12, 2022

Uh oh!

MarSik commented Apr 13, 2022

Uh oh!

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

Uh oh!

Uh oh!

jmencak commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yanirq commented Apr 13, 2022 •

edited

Loading