Skip to content

linux: ns: get_process_nspid() support old kernels #20

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Jan 19, 2022
Merged

linux: ns: get_process_nspid() support old kernels #20

merged 19 commits into from
Jan 19, 2022

Conversation

@adi-benz
Copy link
Contributor

@adi-benz adi-benz commented Jan 16, 2022 •
edited
Loading

Make get_process_nspid() work with old kernels

Tested on Ubuntu 14.04 3.13.0-170-generic

@adi-benz adi-benz added the enhancement New feature or request label Jan 16, 2022
@adi-benz adi-benz requested a review from Jongy January 16, 2022 14:26
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
@adi-benz adi-benz mentioned this pull request Jan 16, 2022
Merged
3 tasks
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 16, 2022
View reviewed changes
Adi Benziony added 4 commits January 16, 2022 17:03
Reduce file read exception scope
cfc102d
Rename procfs var
bc6149e
Some refactoring
3a0e6bc
Trying to read the sched file might yield ProcessLookupError
25adb3c 
This happend when the process dies between us opening the sched file
and us reading it. Appearently procfs returns ESRCH when trying to read
a procfs file of a dead process, and Python converts it to
a ProcessLookupError
@adi-benz adi-benz requested a review from Jongy January 16, 2022 15:47
@adi-benz
Copy link
Contributor Author

adi-benz commented Jan 16, 2022

Following your suggestions to use asserts (in intel/gprofiler#269), now that get_process_nspid() fails only if the process isn't running, should we make the function raise instead of returning None?

@Jongy
Copy link
Contributor

Jongy commented Jan 16, 2022

Following your suggestions to use asserts (in Granulate/gprofiler#269), now that get_process_nspid() fails only if the process isn't running, should we make the function raise instead of returning None?

Yes. Let's, instead of returning None, ensure that the PID now really doesn't run - and then raise psutil.NoSuchProcess. If it does run and we haven't found, raise another exception (that's probably a bug in our code)

Adi Benziony added 3 commits January 16, 2022 20:05
get_process_nspid() returns int or raises
85a3e8a 
Also, make sure we raise NoSuchProcess in all places instead of weird,
un-indicative errors
Fix formatting
c28f0da
Make enable_hook.sh work in submodules as well
00bf366
Jongy
Jongy reviewed Jan 17, 2022
View reviewed changes
@adi-benz adi-benz requested a review from Jongy January 18, 2022 07:45
Jongy
Jongy reviewed Jan 18, 2022
View reviewed changes
granulate_utils/linux/ns.py Outdated

is_the_same_ns = get_process_ns_inode(process, nstype) == get_process_ns_inode(process2, nstype)

# If one of the processes isn't running, we checked the wrong one
Copy link
Contributor

@Jongy Jongy Jan 18, 2022

Choose a reason for hiding this comment

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

Why do this check again, if already done in get_process_ns_inode?

Copy link
Contributor Author

@adi-benz adi-benz Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

get_process_ns_inode() doesn't make sure this is the same process.
get_process_ns_inode() might return True, but actually, it was for the wrong process.

But now I'm moving this check into get_process_ns_inode() 🙃

Jongy
Jongy reviewed Jan 18, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 18, 2022
View reviewed changes
@adi-benz adi-benz requested a review from Jongy January 19, 2022 10:23
Jongy
Jongy reviewed Jan 19, 2022
View reviewed changes
Jongy
Jongy reviewed Jan 19, 2022
View reviewed changes
Jongy
Jongy suggested changes Jan 19, 2022
View reviewed changes
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.

.

adi-benz and others added 2 commits January 19, 2022 15:53
@adi-benz @Jongy
Typo in doc
7f8fb31 
Co-authored-by: Yonatan Goldschmidt <yonatan.goldschmidt@granulate.io>
@adi-benz @Jongy
Use repr to print string
a865137 
Co-authored-by: Yonatan Goldschmidt <yonatan.goldschmidt@granulate.io>
@adi-benz adi-benz requested a review from Jongy January 19, 2022 13:54
@adi-benz adi-benz merged commit 83dd35e into master Jan 19, 2022
@adi-benz adi-benz deleted the get-process-nspid-support-old-kernels branch January 19, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Jongy Jongy Jongy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adi-benz @Jongy