Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spec: used macro for profiles path and other fixes #665

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

yarda
Copy link
Contributor

@yarda yarda commented Jul 26, 2024

  • improved conditionals for better readability
  • fixed tuned-ppd not to run install twice
  • used macro for profiles path

@yarda yarda requested a review from zacikpa July 26, 2024 20:08
@yarda yarda force-pushed the spec-profile-paths-macro branch 2 times, most recently from 2652a28 to 05bf7bb Compare July 27, 2024 10:07
@zacikpa
Copy link
Contributor

zacikpa commented Aug 6, 2024

The changes look fine to me and the RPM build/installation works well. The failed CI tests do not seem related to the changes, though I'm not sure what's causing them. Approving.

@yarda
Copy link
Contributor Author

yarda commented Oct 14, 2024

/packit build

@yarda
Copy link
Contributor Author

yarda commented Oct 14, 2024

Rebased and hopefully fixed not to break the CI now :)

@yarda yarda force-pushed the spec-profile-paths-macro branch 6 times, most recently from 196635e to efd84e4 Compare October 14, 2024 19:21
@yarda
Copy link
Contributor Author

yarda commented Oct 14, 2024

It was a bit more tricky than I initially thought :). It seems I will also have to fix tests.

@yarda
Copy link
Contributor Author

yarda commented Oct 15, 2024

It also requires: beakerlib/tuned#10

@yarda yarda force-pushed the spec-profile-paths-macro branch 2 times, most recently from 788be03 to 30c6f94 Compare October 15, 2024 14:19
@yarda
Copy link
Contributor Author

yarda commented Oct 15, 2024

@zacikpa please re-review.

Copy link
Contributor

@zacikpa zacikpa left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good, I only have a couple of comments.

Comment on lines +37 to +41
rlServiceStop "tuned"
sleep 1
rlFileBackup "/var/log/tuned/tuned.log"
rlRun "rm -rf /var/log/tuned/tuned.log"
rlFileBackup "/var/log/tuned/tuned.log"
rlRun "rm -rf /var/log/tuned/tuned.log"
rlServiceStart "tuned"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why separate the service stop? I'd back up the logs first and then run rlServiceStart (which restarts TuneD if it's already running).

Copy link
Contributor Author

@yarda yarda Oct 16, 2024

Choose a reason for hiding this comment

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

It's taken from the RHEL dist-git upstream Beaker test. I wanted to sync to their version. IMHO it's trying to avoid possible race if the daemon is simultaneously logging while the file is removed/backed-up. Without it IMHO the worst case that could happen is that for multiline logs there could be some lines missing in the log (IMHO it uses line buffering). But the calls in the test could be reordered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's keep the tests in sync.

Makefile Outdated Show resolved Hide resolved
- improved conditionals for better readability
- fixed tuned-ppd not to run install twice
- used macro for profiles path
- consolidated names of related variables
- updated Makefile to update the current profile paths during
  installation
- fixed tests to use correct profile directory on RHEL/CentOS
- consolidated whitespaces and variable names in fixed tests

Signed-off-by: Jaroslav Škarvada <jskarvad@redhat.com>
@yarda yarda merged commit 35eed3c into redhat-performance:master Oct 16, 2024
16 checks passed
@yarda yarda deleted the spec-profile-paths-macro branch October 16, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants