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.

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

Added portlibrary global to make isRunningInContainer called at startup #3420

Merged
merged 1 commit into from
Jan 23, 2019
Merged

Added portlibrary global to make isRunningInContainer called at startup #3420

merged 1 commit into from
Jan 23, 2019

Conversation

bharathappali
Copy link
Contributor

OMR PortLibrary Api omrsysinfo_is_running_in_container is required by every runtime built on top of OMR and also might be having many places where this would be called, As this api internally calls other function, its get invoked everytime we make a call to the api so i thought it would be good to have a PortLibrary global variable PPG_isRunningInContainer for is running in container check and also that function to be called at the portLibrary startup which assigns the value for the portlibrary global PPG_isRunningInContainer and would decrease the calls for isRunningInContainer function.

Signed-off-by: bharathappali bharath.appali@gmail.com

@bharathappali bharathappali mentioned this pull request Jan 7, 2019
Closed
@bharathappali bharathappali changed the title WIP: Added portlibrary global to make isRunningInContainer called at startup Added portlibrary global to make isRunningInContainer called at startup Jan 7, 2019
ashu-mehra
ashu-mehra reviewed Jan 7, 2019
View reviewed changes
port/unix/omrsysinfo.c Outdated
{
BOOLEAN inContainer = FALSE;
#if defined(LINUX) && !defined(OMRZTPF)
*errorCode = isRunningInContainer(portLibrary, &inContainer);
inContainer = PPG_isRunningInContainer;
Copy link
Contributor

Choose a reason for hiding this comment

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

This function can just return PPG_isRunningInContainer. I don't think there is any need for inContainer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but PPG_isRunningInContainer is only in LINUX and on other platforms it needs to return FALSE so to avoid two returns i created a variable inContainer and returning it.

Please correct me if i'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I didn't realize PPG_isRunningInContainer is only in LINUX.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have the global on all platforms and always just return the global? Platforms that do not support containers can just always set the global to false. This way in the future if other platforms do support containers there would be less code to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charliegracie That sounds good to me, Will be making the changes. Thanks !

@ashu-mehra
Copy link
Contributor

Can you please add a comment as to why the errorCode argument is dropped from omrsysinfo_is_running_in_container api.

@bharathappali
Copy link
Contributor Author

As the current implementation is making the result of isRunningInContainer global and returned in omrsysinfo_is_running_in_container, to have a error code sent back it again needs a global variable to record it at startup which is not that required. So to make the functionality simple we could just return either TRUE or FALSE for omrsysinfo_is_running_in_container, if there was any error in the call for isRunningInContainer it makes the BOOLEAN var passed to it as FALSE. So returning TRUE or FALSE could make it simple and easy to use.

Please provide your views on it @ashu-mehra . Thanks!

DanHeidinga
DanHeidinga reviewed Jan 7, 2019
View reviewed changes
port/common/omrsysinfo.c
@@ -960,7 +960,7 @@ omrsysinfo_get_cgroup_subsystem_list(struct OMRPortLibrary *portLibrary)
* @return TRUE if Runtime is running in a container and FALSE if not or if an error occurs
Copy link
Contributor

Choose a reason for hiding this comment

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

 * @param[in] errorCode int32_t pointer to state error code from internal calls

Needs to be removed as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed description for errorCode param.

@bharathappali bharathappali force-pushed the edit-running-in-container branch 5 times, most recently from 3580185 to 8e47fdd Compare January 8, 2019 10:21
charliegracie
charliegracie reviewed Jan 8, 2019
View reviewed changes
port/unix/omrsysinfo.c
@@ -2058,6 +2059,7 @@ omrsysinfo_startup(struct OMRPortLibrary *portLibrary)
}
}
attachedPortLibraries += 1;
isRunningInContainer(portLibrary, &PPG_isRunningInContainer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense for this code to look like this:
PPG_isRunningInContainer = isRunningInContainer(portLibrary);

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update on this question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for delayed reply @charliegracie I have missed to reply to this question earlier.

isRunningInContainer is an internal method which is used by many functions and some of them (now or in future) might need the return code of it. So I'm thinking it would be better to have the current implementation to have the error code handy. Please let me know your views on it. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem :). Ah I missed the other callers. Since the other callers are using the return value I think it makes sense to leave the code as is. Thanks!

@bharathappali
Added portlibrary global to make isRunningInContainer called at portl…
2c23672 
…ibrary startup

OMR PortLibrary api `omrsysinfo_is_running_in_container` is required by
every runtime built on top of OMR and also might be having many places
where this would be called, As this api internally calls other function
`isRunningInContainer`, its get invoked everytime we make a call to the
api so it would be good to have a PortLibrary global variable
`PPG_isRunningInContainer` for is running in container check and also
that function to be called at the portLibrary startup which assigns the
value for the portlibrary global PPG_isRunningInContainer and would
decrease the calls for `isRunningInContainer` function.

Signed-off-by: bharathappali <bharath.appali@gmail.com>
@bharathappali bharathappali force-pushed the edit-running-in-container branch from 8e47fdd to 2c23672 Compare January 16, 2019 07:01
@charliegracie
Copy link
Contributor

@genie-omr build all

@bharathappali
Copy link
Contributor Author

@charliegracie continuous-integration is failing to fetch the origin. Will triggering the build again solves the problem ?

@charliegracie
Copy link
Contributor

@genie-omr build plinux

@charliegracie charliegracie merged commit cf3e1b6 into eclipse-omr:master Jan 23, 2019
@DanHeidinga DanHeidinga mentioned this pull request Jan 23, 2019
Merged
DanHeidinga added a commit to DanHeidinga/openj9 that referenced this pull request Jan 24, 2019
@DanHeidinga
Update omrsysinfo_is_running_in_container() signature
cd91fc4 
OMR updated omrsysinfo_is_running_in_container() to remove the
`errorCode` parameter in eclipse-omr/omr#3420

This change helps the openj9-omr to promote

Signed-off-by: Dan Heidinga <daniel_heidinga@ca.ibm.com>
@bharathappali bharathappali deleted the edit-running-in-container branch January 24, 2019 06:12
Yuehan-Lin pushed a commit to Yuehan-Lin/openj9 that referenced this pull request Feb 5, 2019
@DanHeidinga @Yuehan-Lin
Update omrsysinfo_is_running_in_container() signature
4e68c73 
OMR updated omrsysinfo_is_running_in_container() to remove the
`errorCode` parameter in eclipse-omr/omr#3420

This change helps the openj9-omr to promote

Signed-off-by: Dan Heidinga <daniel_heidinga@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga left review comments

@ashu-mehra ashu-mehra ashu-mehra left review comments

@charliegracie charliegracie charliegracie left review comments

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@youngar youngar Awaiting requested review from youngar

Assignees
No one assigned
Labels
comp:port
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bharathappali @ashu-mehra @charliegracie @DanHeidinga @Leonardo2718