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

granulate-utils: Fix resolve_proc_root_links #221

Merged
merged 5 commits into from
Dec 28, 2023

Conversation

netaneld122
Copy link
Contributor

Fix resolve_proc_root_links handling of links that resolve to sub-path links.

Example:

{tmpdir}/link
    link -> {tmpdir}/a/c
    a -> {tmpdir}/b
Eventually we expect {tmpdir}/link to resolve to {tmpdir}/b/c

@netaneld122 netaneld122 added the bug Something isn't working label Dec 26, 2023
@netaneld122 netaneld122 force-pushed the bugfix/resolve-proc-root-links-alternating-links branch from 2a1b64c to c816d39 Compare December 26, 2023 20:25
Copy link
Contributor

@Jongy Jongy left a comment

Choose a reason for hiding this comment

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

Didn't review yet, but just one note - there are several tests in gprofiler which test this function's behavior. Please create a PR in gprofiler which points to this commit so the tests run agains this commit. (Plus, post merging we'll want to update gprofiler as well).

granulate_utils/linux/ns.py Outdated Show resolved Hide resolved
Jongy
Jongy previously approved these changes Dec 27, 2023
Copy link
Contributor

@Jongy Jongy left a comment

Choose a reason for hiding this comment

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

The core change is clear and looks good to me. I gave 2 nitpick comments. @michelhe as this is a very core part, please review as well.

Copy link
Contributor

@michelhe michelhe left a comment

Choose a reason for hiding this comment

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

Looks good, 1 small comment and a question -
Wouldn't nsentring to the process mount namespace and then simply readlink() inside the namespace will serve the same purpose, but we'll have the OS do the resolving for us?

granulate_utils/linux/ns.py Show resolved Hide resolved
@netaneld122 netaneld122 merged commit 77609c2 into master Dec 28, 2023
7 checks passed
@netaneld122 netaneld122 deleted the bugfix/resolve-proc-root-links-alternating-links branch December 28, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants