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

plugin_scheduler: Adjust error logging in _set_affinity #659

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

zacikpa
Copy link
Contributor

@zacikpa zacikpa commented Jul 12, 2024

Failing to change the affinity of a defunct process should not cause any issues, even if the process is not bound to specific CPUs. We should thus not log this failure as an error. This PR introduces a change that causes TuneD to log such failures as debug messages only, along with some refactoring/simplification of the relevant functions.

Resolves: RHEL-46560

except (AttributeError, KeyError) as e:
log.error("Failed to get task info for PID %d: %s"
% (pid, e))
return -2
finally:
return False

Check warning

Code scanning / CodeQL

'break' or 'return' statement in finally Warning

'return' in a finally block will swallow any exceptions raised.
Copy link
Contributor

Choose a reason for hiding this comment

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

The CodeQL is probably right here, this could hide other unhandled exceptions happening inside the try: block.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the CodeQL didn't trigger the rescan.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's no more issue with the recent force push.

@zacikpa zacikpa force-pushed the defunct-affinity branch 3 times, most recently from 8a2c4d1 to c55f4f5 Compare July 12, 2024 09:21
Do not report an error if a process is defunct even if it
is not bound to specific CPUs: failing to change its affinity
does not have any negative effects.

Refactor the helper function _affinity_changeable to
_ignore_set_affinity_error and make it return True/False
depending on whether the failed affinity change can be
ignored (i.e., not reported as an error).

Do not check for vanished processes twice, the later
check is sufficient.

Resolves: RHEL-46560
@zacikpa zacikpa force-pushed the defunct-affinity branch from c55f4f5 to 30908c3 Compare July 12, 2024 09:23
@zacikpa zacikpa requested a review from yarda July 23, 2024 13:03
@yarda
Copy link
Contributor

yarda commented Jul 24, 2024

/packit build

Copy link
Contributor

@yarda yarda left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

CentOS 7 failure is unrelated and being worked on by TFT team.

@yarda yarda merged commit 1c4cb62 into redhat-performance:master Jul 24, 2024
13 of 14 checks passed
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