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

Exclude OperatingSystemMXBean PhysicalMemory test in Docker #12420

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

pshipton
Copy link
Member

@pshipton pshipton commented Apr 12, 2021

Part of tests testOSMXBeanLocal and testOSMXBeanRemote, which run
org.openj9.test.management.TestOperatingSystemMXBean.runTestOSMXBean()

Issue #12038

@pshipton
Copy link
Member Author

I'll add plinux.

@pshipton
Copy link
Member Author

Even better would be to detect the VM is running in docker, not sure how to do that.

@pshipton pshipton force-pushed the excludeosbean branch 4 times, most recently from 9c2f4ff to 18501cb Compare April 13, 2021 17:23
@pshipton pshipton changed the title Exclude OperatingSystemMXBean PhysicalMemory test on xLinux and aarch64 Exclude OperatingSystemMXBean PhysicalMemory test in Docker Apr 13, 2021
@pshipton
Copy link
Member Author

Updated to detect when running in a Docker container on Linux rather than checking for hard coded platforms.

Tested via grinders, which don't detect a container as expected on the OpenJ9 machines.
zlinux: https://ci.eclipse.org/openj9/view/Test/job/Grinder/1673
plinux: https://ci.eclipse.org/openj9/view/Test/job/Grinder/1674
xlinux: https://ci.eclipse.org/openj9/view/Test/job/Grinder/1675
aarch64: https://ci.eclipse.org/openj9/view/Test/job/Grinder/1676
osx: https://ci.eclipse.org/openj9/view/Test/job/Grinder/1677

Added an enum in the code, run one more grinder to validate.
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1678

@pshipton pshipton marked this pull request as ready for review April 13, 2021 17:50
Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

Have you tested this in other container environments (e.g. podman)?

Comment on lines 86 to 91
try (FileInputStream env = new FileInputStream("/.dockerenv")) {
logger.info("/.dockerenv found");
inContainer = ContainerStatus.FOUND;
return true;
} catch (IOException e) {
// no "/.dockerenv" file, fall through
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to if (new File("/.dockerenv").exists()) { ...}.

String line;
while ((line = reader.readLine()) != null) {
if (line.contains("docker")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested simplification:

		if (reader.lines().anyMatch(line -> line.contains("docker"))) {
			logger.info("Container found: " + line);
			inContainer = ContainerStatus.FOUND;
			return true;
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't work because line is used afterwards to log how the container was found, for easier debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that aspect. It could probably be reworked, but perhaps it's not worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it to use your pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

The lambda need not change if you use findFirst():

	Optional<String> docker = reader.lines().filter(line -> line.contains("docker")).findFirst();
	if (docker.isPresent()) {
		logger.info("Container found: " + docker.get());
		inContainer = ContainerStatus.FOUND;
		return true;
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@pshipton pshipton force-pushed the excludeosbean branch 3 times, most recently from be3f2e1 to 2fbbaad Compare April 13, 2021 22:20
@pshipton
Copy link
Member Author

pshipton commented Apr 13, 2021

Updated to address comments. New grinder https://ci.eclipse.org/openj9/view/Test/job/Grinder/1682 - passed.

@pshipton pshipton force-pushed the excludeosbean branch 2 times, most recently from 09b1718 to 59067d5 Compare April 14, 2021 00:24
@andrew-m-leonard
Copy link
Contributor

@pshipton just tried fix out at Adopt, and works greate on xLinux, pLinux and aarch64 docker nodes:
https://ci.adoptopenjdk.net/job/Test_openjdk16_j9_extended.functional_x86-64_linux/62/
https://ci.adoptopenjdk.net/job/Test_openjdk8_j9_extended.functional_ppc64le_linux/120/
https://ci.adoptopenjdk.net/view/Test_functional/job/Test_openjdk16_j9_extended.functional_aarch64_linux/61/

It would be useful to have this in the release branch if possible for next week so we avoid unnecessary test triage noise...

@pshipton pshipton force-pushed the excludeosbean branch 2 times, most recently from 2030679 to ca1f687 Compare April 14, 2021 14:51
Part of tests testOSMXBeanLocal and testOSMXBeanRemote, which run
org.openj9.test.management.TestOperatingSystemMXBean.runTestOSMXBean()

Issue eclipse-openj9#12038

Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
@pshipton
Copy link
Member Author

pshipton commented Apr 14, 2021

New grinder https://ci.eclipse.org/openj9/view/Test/job/Grinder/1686 - passed.

@keithc-ca keithc-ca merged commit c15c9f8 into eclipse-openj9:master Apr 14, 2021
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