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

Add test for omrsysinfo_is_running_in_container #6567

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

EricYangIBM
Copy link
Contributor

Add test for omrsysinfo_is_running_in_container and fix cgroup tests when
running in a cgroup v1 container.

Issue: #1281
Signed-off-by: Eric Yang [email protected]

@EricYangIBM
Copy link
Contributor Author

@babsingh

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

To run PR builds in Docker, we will need #6525 to be merged. If it takes time for it be merged, then I will cherry-pick your commit in #6525, run PR builds in #6525, and post the results here.

Comment on lines 2399 to 2402
char *omr_running_in_docker = getenv("OMR_RUNNING_IN_DOCKER");
if ((NULL != omr_running_in_docker) && (0 == strcmp(omr_running_in_docker, "1"))) {
isRunningInContainer = true;
}
Copy link
Contributor

@babsingh babsingh Jun 14, 2022

Choose a reason for hiding this comment

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

Camel case syntax is required: https://github.com/eclipse/omr/blob/master/doc/CodingStandard.md#variables.

Suggested change
char *omr_running_in_docker = getenv("OMR_RUNNING_IN_DOCKER");
if ((NULL != omr_running_in_docker) && (0 == strcmp(omr_running_in_docker, "1"))) {
isRunningInContainer = true;
}
char *omrRunningInDocker = getenv("OMR_RUNNING_IN_DOCKER");
if ((NULL != omrRunningInDocker) && (0 == strcmp(omrRunningInDocker, "1"))) {
isRunningInContainer = true;
}

@@ -2376,6 +2376,7 @@ TEST(PortSysinfoTest, sysinfo_test_os_kernel_info)
class CgroupTest : public ::testing::Test {
protected:
#if defined(LINUX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the cgroup API implementation, it uses defined(LINUX) && !defined(OMRZTPF). We will have to use this expression in the tests. We will also need to account for this in #6559 since it is unmerged.

#if defined(LINUX)
ASSERT_EQ(isRunningInContainer, CgroupTest::isRunningInContainer);
#else /* defined(LINUX) */
if (FALSE != runningInContainer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Already a boolean.

Suggested change
if (FALSE != runningInContainer) {
if (runningInContainer) {

BOOLEAN runningInContainer = omrsysinfo_is_running_in_container();

#if defined(LINUX)
ASSERT_EQ(runningInContainer, CgroupTest::isRunningInContainer);
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment should be added: If this test fails, then it should be verified that OMR_RUNNING_IN_DOCKER (env var) is correctly set. For a containerized run, OMR_RUNNING_IN_DOCKER is set to 1. For a non-containerized run, OMR_RUNNING_IN_DOCKER is either unset or set to 0.

Add test for omrsysinfo_is_running_in_container and fix cgroup tests when
running in a cgroup v1 container.

Issue: eclipse#1281
Signed-off-by: Eric Yang <[email protected]>
@EricYangIBM
Copy link
Contributor Author

Done

@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

All are known failures unrelated to this PR.

https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/3664/consoleFull

Compiler issue. Tracked in #6556. Unrelated to this PR.

https://dev.azure.com/eclipse-omr/ea4519db-b27e-4d19-a971-f01491f803e3/_apis/build/builds/5854/logs/44

Network issue. Tracked in #6516. Unrelated to this PR.

@babsingh
Copy link
Contributor

All PR builds inside a Docker container have passed: #6570 (comment). Thus, approving the PR.

@0xdaryl 0xdaryl self-assigned this Jun 15, 2022
@0xdaryl 0xdaryl merged commit c5700ab into eclipse:master Jun 15, 2022
@EricYangIBM EricYangIBM deleted the test_running_in_container branch June 15, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants