openshift / cluster-node-tuning-operator

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

CNF-13014: Add a specific error message for when the TuneD submodule folder is not initialized #1141

Closed fontivan closed 1 week ago

fontivan commented 2 months ago
openshift-ci-robot commented 2 months ago

@fontivan: This pull request references CNF-13014 which is a valid jira issue.

In response to [this](https://github.com/openshift/cluster-node-tuning-operator/pull/1141): >- If you did not checkout the git repo using `--recursive` then the tuned submodule folder will be empty >- Without a specific error message it would instead return a vague error that a patch failed to be applied >- Instead we will check explicitly if the folder is empty and return a more helpful error message in these cases > 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 2 months ago

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[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

Old error:

+ cd /root/assets/tuned/tuned
+ LC_COLLATE=C
+ cat ../patches/030-no-bootloader-errors.diff
+ patch -Np1
can't find file to patch at input line 10
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|NTO currently supports the bootloader plugin only on RHCOS via the integration
|with MCO.  Disable patching of BLS entries and grub configuration files so
|that error messages from the tuned daemon are eliminated and not reported
|further on in k8s objects.
|
|diff --git a/assets/tuned/daemon/tuned/plugins/plugin_bootloader.py b/assets/tuned/daemon/tuned/plugins/plugin_bootloader.py
|index 416df7d6..b0a61905 100644
|--- a/tuned/plugins/plugin_bootloader.py
|+++ b/tuned/plugins/plugin_bootloader.py
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
3 out of 3 hunks ignored
Error: building at STEP "RUN /bin/bash /tmp/dockerfile_install_support.sh": while running runtime: exit status 1
make: *** [Makefile:175: local-image] Error 1

New error:

++ ls /root/assets/tuned/tuned
+ [[ -z '' ]]
+ echo 'TuneD submodule is an empty folder. Consider initializing the module: '\''git submodule update --init --recursive'\'''
TuneD submodule is an empty folder. Consider initializing the module: 'git submodule update --init --recursive'
+ exit 1
Error: building at STEP "RUN /bin/bash /tmp/dockerfile_install_support.sh": while running runtime: exit status 1
make: *** [Makefile:175: local-image] Error 1
fontivan commented 2 months ago

/retest

yanirq commented 2 months ago

/retest

rbaturov commented 2 months ago

@fontivan take a look at this fix https://github.com/openshift/cluster-node-tuning-operator/pull/1159.

fontivan commented 2 months ago

@rbaturov I like #1159 but I also think we could also do error message addition too since it is a minor error message improvement if you end up in a weird submodule state

fontivan commented 2 months ago

/retest

fontivan commented 1 week ago

@MarSik @jmencak @rbaturov do we still want this or is this unnecessary with #1159?

fontivan commented 1 week ago

/cc @MarSik @jmencak

jmencak commented 1 week ago

@MarSik @jmencak @rbaturov do we still want this or is this unnecessary with #1159?

Can you provide steps to show us the issue you're seeing?

fontivan commented 1 week ago

I think this is just no longer required with #1159, I don't see the original error any more on a fresh checkout