openshift / cluster-node-tuning-operator

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

PSAP-1354: Replaced tuned hard-included repo with a git submodule #1029

Closed rbaturov closed 7 months ago

rbaturov commented 7 months ago

Until now tuned repo was hard included in the NTO repo. This design could lead to developers mistakenly contribute to assets/tuned. Instead of https://github.com/redhat-performance/tuned, as it is hard to realize it is an external repo. Therefore, we chose to change this design by including the tuned repo as a git submodule, preventing mistakes as I described and better informing tuned is an external repo.

rbaturov commented 7 months ago

/unhold

jmencak commented 7 months ago

/assign jmencak

jmencak commented 7 months ago

Thank you for the PR, Ronny. I think this needs a bit more work.

This is what I've done:

$ git clone https://github.com/openshift/cluster-node-tuning-operator
$ git fetch origin pull/1029/head:1029
$ git checkout 1029
$ make update-tuned-submodule
(cd assets/tuned/tuned && \
  git pull origin master && \
  cd tuned && git checkout 954bc4624db8fb345e9fb264ee2ac09d1736105a)
From https://github.com/openshift/cluster-node-tuning-operator
 * branch              master     -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint: 
hint:   git config pull.rebase false  # merge
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.
make: *** [Makefile:58: update-tuned-submodule] Error 128

This also suggests that TuneD would be in assets/tuned/tuned/tuned ? Is this intended?

rbaturov commented 7 months ago

Thank you for the PR, Ronny. I think this needs a bit more work.

This is what I've done:

$ git clone https://github.com/openshift/cluster-node-tuning-operator
$ git fetch origin pull/1029/head:1029
$ git checkout 1029
$ make update-tuned-submodule
(cd assets/tuned/tuned && \
  git pull origin master && \
  cd tuned && git checkout 954bc4624db8fb345e9fb264ee2ac09d1736105a)
From https://github.com/openshift/cluster-node-tuning-operator
 * branch              master     -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint: 
hint:   git config pull.rebase false  # merge
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.
make: *** [Makefile:58: update-tuned-submodule] Error 128

This also suggests that TuneD would be in assets/tuned/tuned/tuned ? Is this intended?

Yes. Before, the assets/tuned/daemon dir contained the hard-included tuned repo. After I have changed this design to include a git submodule I removed "daemon" and included the git "tuned" submodule. I could name the submodule "daemon" to preserve the naming structure, but it would be misleading as in github it will be displayed like this: image

rbaturov commented 7 months ago

Thank you for the changes, looking better now. Let's address a few nits and also set TUNED_COMMIT:=HEAD

Thank you!

Sorry, missed you asked for that in the previous message. I have pushed this change now. This will guarantee tuned repo will always be updated with the latest commit, that's your intention, right?

jmencak commented 7 months ago

Thank you for the changes. /approve /lgtm

Giving other reviewers a chance to chime in. /hold

jmencak commented 7 months ago

/cc @bartwensley

rbaturov commented 7 months ago

/retest-required

jmencak commented 7 months ago

@rbaturov , please add a valid Jira reference, thank you!

openshift-ci-robot commented 7 months ago

@rbaturov: This pull request references PSAP-1354 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 the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1029): >Until now tuned repo was hard included in the NTO repo. This design could lead to developers mistakenly contribute to assets/tuned. Instead of https://github.com/redhat-performance/tuned, as it is hard to realize it is an external repo. Therefore, we chose to change this design by including the tuned repo as a git submodule, preventing mistakes as I described and better informing tuned is an external repo. 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.
jmencak commented 7 months ago

@rbaturov feel free to unhold if you don't see any other feedback from reviewers in some time.

openshift-ci[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartwensley, jmencak, rbaturov

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)~~ [jmencak] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
rbaturov commented 7 months ago

/unhold

openshift-ci-robot commented 7 months ago

/retest-required

Remaining retests: 0 against base HEAD 0d90b86f112f34e562a3a7c5ee53813051d0ac9b and 2 for PR HEAD 371534cadc4f931ca50ec519c9ba3f03a3fb5c34 in total

openshift-ci[bot] commented 7 months ago

@rbaturov: 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-202404162215.p0.g0751f57.assembly.stream.el9 for distgit cluster-node-tuning-operator. All builds following this will include this PR.