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

Fix use of SHM for Selenium containers on Windows #1948

Merged
merged 8 commits into from
Oct 6, 2019

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Oct 4, 2019

No description provided.

@rnorth
Copy link
Member Author

rnorth commented Oct 4, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

by only checking counts of SHM volumes
@rnorth
Copy link
Member Author

rnorth commented Oct 4, 2019

Latest change to tests augments #1751

Looks like we still have some failures - checking

@rnorth rnorth changed the title Change assertion used for SHM testing to support Windows WIP: Fix use of SHM for Selenium containers on Windows Oct 4, 2019
@rnorth
Copy link
Member Author

rnorth commented Oct 4, 2019

Please ignore - this is not the right solution to the problem.

Updated

@rnorth rnorth closed this Oct 4, 2019
@rnorth rnorth reopened this Oct 4, 2019
@rnorth
Copy link
Member Author

rnorth commented Oct 4, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -167,7 +167,12 @@ private static String unencodeResourceURIToFilePath(@NotNull final String resour
private String resolvePath() {
String result = getResourcePath();

if (SystemUtils.IS_OS_WINDOWS && result.startsWith("/")) {
// Special cases for Windows
if (SystemUtils.IS_OS_WINDOWS && result.matches("^/[A-Z]:/dev/.*")) {
Copy link
Member Author

@rnorth rnorth Oct 4, 2019

Choose a reason for hiding this comment

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

I don't particularly like having to add this special case behaviour - may move elsewhere. I don't really see a better place for it at the moment, particularly as we already had one Windows special-case here already.

This prevents /dev/shm eventually becoming /host_mnt/c/dev/shm on Docker for Windows. If this broken path is used for the SHM mount then SHM is unusable inside the container; the path needs to not be remapped onto /host_mnt/c, and should remain /dev/shm.

Copy link
Member

Choose a reason for hiding this comment

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

Actually.... what if we avoid using addFileSystemBind (that creates a mountable file) but use the bind API directly for the SHM?

This hack adjustment does solve the problem too, but I think we're fixing it in the wrong place. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point - I did think of something similar but didn't want to extend our API just for this. What I didn't think of was just doing this.getBinds().add( ... ), which I suppose would work.

Will give it a try.

@rnorth
Copy link
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rnorth rnorth changed the title WIP: Fix use of SHM for Selenium containers on Windows Fix use of SHM for Selenium containers on Windows Oct 5, 2019
final MountableFile mountableFile = MountableFile.forHostPath("/dev/shm");

final String resolvedPath = mountableFile.getResolvedPath();
assertThat("the /dev/shm path should always be direct", resolvedPath, startsWith("/dev/shm"));
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 would be nice if we could set this up so that it runs thinking it's on a Windows machine. Probably do-able.

However, we do have the safeguard that this test will be run on Windows CI at some point. Maybe that's enough?

@rnorth
Copy link
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -51,34 +52,40 @@ public void provideDefaultNoProxyEnvironmentIfNotSet() {
public void createContainerWithShmVolume() {
try (
BrowserWebDriverContainer webDriverContainer = new BrowserWebDriverContainer()
.withCapabilities(new FirefoxOptions())
Copy link
Member Author

Choose a reason for hiding this comment

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

Using Firefox because it is more sensitive to SHM problems (Only Firefox tests failed when the Windows SHM mount was working incorrectly)

@rnorth
Copy link
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bsideup bsideup added this to the next milestone Oct 6, 2019
@rnorth rnorth merged commit 94c903d into master Oct 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-shm-assertion-for-windows branch October 6, 2019 07:16
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.

None yet

2 participants