USHIFT-3463: Add microshift-tuned daemon for unattended TuneD profile activation #3656
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-3463 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.17.0" version, but no target version was set. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
approved
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
from
689e9e4
to
6bc6446
Compare
openshift-merge-robot
added
the
needs-rebase
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
from
6bc6446
to
3267a0a
Compare
openshift-merge-robot
removed
the
needs-rebase
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
from
3267a0a
to
c45d7ff
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
3 times, most recently
from
4422b03
to
921fcb0
Compare
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
from
921fcb0
to
70f8737
Compare
| Requires=tuned.service | ||
|
|
||
| [Service] | ||
| ExecStart=/usr/bin/microshift-tuned |
There was a problem hiding this comment.
This script may reboot host. Should we provide a mandatory command line for the script to actually do something? Otherwise, if someone runs it, they can end up rebooting accidentally.
There was a problem hiding this comment.
You mean that dry run is default and not dry run is protected with --something?
There was a problem hiding this comment.
There's a variable in the config which controls if reboot is allowed. Wouldn't it be slightly confusing if it was defined False but our service always had --reboot-if-allowed?
There was a problem hiding this comment.
We could call it --live-run or --execute.
| # Later, when microshift-tuned.service is merged, we'll re-enable | ||
| # it to test the whole R4E flow. | ||
| enabled = ["microshift-test-agent", "tuned"] | ||
| enabled = ["microshift-test-agent", "tuned", "microshift", "microshift-tuned"] |
There was a problem hiding this comment.
Do we also need tuned explcitly enabled? Can we do it like with MicroShift and CRI-O services, so that enabling / starting microshift-tuned would also start tuned?
| @@ -15,5 +15,14 @@ scenario_remove_vms() { | |||
| } | |||
|
|
|||
| scenario_run_tests() { | |||
| run_tests host1 suites/tuned/ | |||
| # Should not be ran immediately after creating VM because of | |||
| # microshift-tuned rebooting the node to activate the profile. | |||
There was a problem hiding this comment.
How do we time this after creating the VM, then?
There was a problem hiding this comment.
Should we add some kind of a loop checking that the profile is active?
There was a problem hiding this comment.
Added a check to see if we have >= 2 boots. > so we can run the script repeatedly in local env.
| Current Profile Should Be Hashed | ||
| [Documentation] We expect that microshift-tuned keeps a hash of the profile and variables. | ||
|
|
||
| SSHLibrary.File Should Exist /var/lib/microshift-tuned.yaml |
There was a problem hiding this comment.
I'm not sure how the name / docs reflect the contents of the test. Could you explain?
| [Documentation] If profile is already active, but cache file is missing, | ||
| ... we expect microshift-tuned to reactivate it, reboot, and store the hashes. | ||
|
|
||
| Command Should Work rm /var/lib/microshift-tuned.yaml |
There was a problem hiding this comment.
| Command Should Work rm /var/lib/microshift-tuned.yaml | |
| Command Should Work rm -f /var/lib/microshift-tuned.yaml |
There was a problem hiding this comment.
Can you add a rationale for this change? I'm not opposing, just want to know full story
There was a problem hiding this comment.
Sometime rm is aliased in the root account as rm -i
# alias rm
alias rm='rm -i'
|
|
||
| MicroShift-Tuned Requires Config To Function | ||
| [Documentation] Verify that missing configuration will be fatal. | ||
| Command Should Work rm /etc/microshift/tuned.yaml |
There was a problem hiding this comment.
| Command Should Work rm /etc/microshift/tuned.yaml | |
| Command Should Work rm -f /etc/microshift/tuned.yaml |
| [Documentation] Verify that missing configuration will be fatal. | ||
| Command Should Work rm /etc/microshift/tuned.yaml | ||
| Command Should Work systemctl restart microshift-tuned.service | ||
| Sleep 5s |
There was a problem hiding this comment.
This is potentially a race condition because we're checking a concrete state.
Can we add a loop here with a few retries?
| [Documentation] TODO | ||
| ${bootid}= Get Current Boot Id | ||
| Command Should Work systemctl restart microshift-tuned.service | ||
| Sleep 60s |
There was a problem hiding this comment.
Could we think of making this check a bit faster? Is there any other way we can get an indication that the microshift-tuned script "decided" not to reboot?
There was a problem hiding this comment.
I changed it but I'm not sure about robustness, we'll see
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
2 times, most recently
from
4272664
to
9ea0735
Compare
pmtk
force-pushed
the
low-latency/microshift-tuned
branch
from
9ea0735
to
796c754
Compare
|
@pmtk: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
| description=f"""Daemon for unattended TuneD profile activation. | ||
|
|
||
| When program starts, it compares configuration and system state. | ||
| If the requested profile is not activate, it will be activated, |
There was a problem hiding this comment.
| If the requested profile is not activate, it will be activated, | |
| If the requested profile is not active, it will be activated, |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, pmtk 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:
Approvers can indicate their approval by writing |
No description provided.