search

openshift / cluster-node-tuning-operator

Manage node-level tuning by orchestrating the tuned daemon.
Apache License 2.0
102 stars 105 forks source link

CNF-6517: [Part 1/3] Hypershift PAO adoption #1032

Closed Tal-or closed 7 months ago

Tal-or commented 7 months ago

Before we start with the implementation itself we should do some preliminary work in order to generalize some of the functions and flows so they could fit for the controller when it runs on hypershift platform.

The main work done here, is extracting bits which are specific for non-hypershift platform and replacing methods with functions.

For example we want to pass the client from the caller because on hypershift the client is different than on non-hypershift platform.

NOTE: whether we decide to go with unified controller for both hypershift and none-hypertshift or a separate controller for each, this PR is good improvement in general and will allow us to reuse more code. This is the main reason for dedicating a separate PR for this code changes.

Part 2/3: https://github.com/openshift/cluster-node-tuning-operator/pull/1037 Part 3/3: WIP

openshift-ci-robot commented 7 months ago

@Tal-or: This pull request references CNF-6517 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.15" instead.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1032): >Before we start with the implementation itself we should do some preliminary work in order to generalize some of the function and flows so they could fit for the controller when it runs on hypershift platform. > >The main work done is here, is extracting bits which are specific for non-hypershift platform and replacing methods with functions. > >For example we want to pass the `client` from the caller because on hypershift the client is different than on non-hypershift platform. > >NOTE: whether we decide to go with unified controller for both hypershift and none-hypertshift or a separate controller for each, this PR is good improvement in general and will allow us to reuse more code. >This is the main reason for issuing it on a separate PR. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 7 months ago

@Tal-or: This pull request references CNF-6517 which is a valid jira issue.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1032): >Before we start with the implementation itself we should do some preliminary work in order to generalize some of the functions and flows so they could fit for the controller when it runs on hypershift platform. > >The main work done is here, is extracting bits which are specific for non-hypershift platform and replacing methods with functions. > >For example we want to pass the `client` from the caller because on hypershift the client is different than on non-hypershift platform. > >NOTE: whether we decide to go with unified controller for both hypershift and none-hypertshift or a separate controller for each, this PR is good improvement in general and will allow us to reuse more code. >This is the main reason for issuing it on a separate PR. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 7 months ago

@Tal-or: This pull request references CNF-6517 which is a valid jira issue.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1032): >Before we start with the implementation itself we should do some preliminary work in order to generalize some of the functions and flows so they could fit for the controller when it runs on hypershift platform. > >The main work done is here, is extracting bits which are specific for non-hypershift platform and replacing methods with functions. > >For example we want to pass the `client` from the caller because on hypershift the client is different than on non-hypershift platform. > >NOTE: whether we decide to go with unified controller for both hypershift and none-hypertshift or a separate controller for each, this PR is good improvement in general and will allow us to reuse more code. >This is the main reason for dedicating a separate PR for this code changes. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
Tal-or commented 7 months ago

/cc @yanirq @ffromani PTAL

openshift-ci[bot] commented 7 months ago

@Tal-or: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1032#issuecomment-2050591217): >/cc @yanirq @ffromani PTAL Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Tal-or commented 7 months ago

/retest

yanirq commented 7 months ago

overall Looks good. general questions are in comments.

Tal-or commented 7 months ago

/retest

Tal-or commented 7 months ago

/retest

yanirq commented 7 months ago

/approve

openshift-ci-robot commented 7 months ago

@Tal-or: This pull request references CNF-6517 which is a valid jira issue.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1032): >Before we start with the implementation itself we should do some preliminary work in order to generalize some of the functions and flows so they could fit for the controller when it runs on hypershift platform. > >The main work done here, is extracting bits which are specific for non-hypershift platform and replacing methods with functions. > >For example we want to pass the `client` from the caller because on hypershift the client is different than on non-hypershift platform. > >NOTE: whether we decide to go with unified controller for both hypershift and none-hypertshift or a separate controller for each, this PR is good improvement in general and will allow us to reuse more code. >This is the main reason for dedicating a separate PR for this code changes. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik, Tal-or, yanirq

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/cluster-node-tuning-operator/blob/master/OWNERS)~~ [MarSik,yanirq] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 7 months ago

@Tal-or: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 7 months ago

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-node-tuning-operator-container-v4.16.0-202404191609.p0.g8a57e13.assembly.stream.el9 for distgit cluster-node-tuning-operator. All builds following this will include this PR.