Skip to content

Conversation

@vitus133
Copy link
Contributor

Draft for Tuned performance rendering

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023
@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 Oct 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 16, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 16, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vitus133
Once this PR has been reviewed and has the lgtm label, please assign jmencak for approval. For more information see the Kubernetes Code Review Process.

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

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2023

// If the user supplies extra machine pools, we ingest them here
for _, pool := range mcPools {
if err := genBootstrapWorkloadPinningManifests(partitioningMode, outputDir, pool.Name); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

not really sure why this is needed.
Would not be enough to handle kernel arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, other mcps are not needed. Remained from another try...
Will remove

continue
}

profileName := mainSection.Key("include").String()
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 need to read included tuned profiles? I thought tuned would do that.

Copy link
Contributor Author

@vitus133 vitus133 Nov 6, 2023

Choose a reason for hiding this comment

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

The idea is to render performanceProfile and Tuned patch (both part of the DU profile). For example:

apiVersion: tuned.openshift.io/v1
kind: Tuned
metadata:
  name: performance-patch
  namespace: openshift-cluster-node-tuning-operator
spec:
  profile:
  - data: |
      [main]
      summary=Configuration changes profile inherited from performance created tuned
      include=openshift-node-performance-openshift-node-performance-profile
      [bootloader]
      cmdline_crash=nohz_full=4-47
      [sysctl]
      kernel.timer_migration=1
      [scheduler]
      group.ice-ptp=0:f:10:*:ice-ptp.*
      [service]
      service.stalld=start,enable
      service.chronyd=stop,disable
    name: performance-patch
  recommend:
  - machineConfigLabels:
      machineconfiguration.openshift.io/role: master
    priority: 19
    profile: performance-patch
---
apiVersion: performance.openshift.io/v2
kind: PerformanceProfile
metadata:
  name: openshift-node-performance-profile
spec:
  additionalKernelArgs:
  - rcupdate.rcu_normal_after_boot=0
  - efi=runtime
  - vfio_pci.enable_sriov=1
  - vfio_pci.disable_idle_d3=1
  - module_blacklist=irdma
  cpu:
    isolated: 4-47
    reserved: 0-3
  hugepages:
    defaultHugepagesSize: 1G
    pages:
    - count: 32
      size: 1G
  machineConfigPoolSelector:
    pools.operator.machineconfiguration.openshift.io/master: ""
  nodeSelector:
    node-role.kubernetes.io/master: ""
  numa:
    topologyPolicy: restricted
  realTimeKernel:
    enabled: true
  workloadHints:
    highPowerConsumption: false
    perPodPowerManagement: false
    realTime: true

I'm not sure how to do that with Tuned - do we need to extract this patch to some file before invoking the Tuned?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding the target is to create a MachineConfig to apply kernerl boot arguments.
To get those arguments we need to run tuned with the tuned.conf file generated by the PerformanceProfile and then read the /etc/tuned/bootcmdline file.

So, as far as I see it, we could use PerformanceProfile as "main" input and use NewNodePerformance function to get the tuned from there, as the PerformanceProfile controller does.

Other inputs should be the MachineConfigPools for the SNO ( we already have them cause they are used in the previous step of the installer, the one for the PerformanceProfile render command here

All those inputs will be in one folder (INPUT_DIR)

With all that in place tuned will be run and will generate the /etc/tuned/bootcmdline file that could be read using getBootCmdline function
Here the challenge is to mount all the system folders tuned needs to work properly from the host machine but without letting tuned to modify them. I am working with overlayfs and podman-run to get that.

With all that in place we just need to create the MachineConfig object and write the manifest to a folder which should be mounted from the host machine so it can be read after the container finish its execution.

This is just my understanding of the task, so maybe @MarSik or @yanirq can correct me.

A (WIP) implementation of this is #844

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need to read all extra Tuned objects too. Customers can use those to override pieces of our default one (https://access.redhat.com/solutions/5532341).

But the rest sounds right to me:

Prepare the (RHEL level) tuned profiles, give them to tuned, run the oneshot mode (in isolation somehow), collect the boot args, generate MC compatible with what NTO would do at Day 2.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 18, 2023

@vitus133: 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-aws-ovn-techpreview 5808351 link true /test e2e-aws-ovn-techpreview

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-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2023
@openshift-merge-robot
Copy link
Contributor

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.

@vitus133
Copy link
Contributor Author

not relevant anymore

@vitus133 vitus133 closed this Dec 18, 2023
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. 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.

5 participants