Skip to content

Fix use of SHM for Selenium containers on Windows#1948

Merged
rnorth merged 8 commits intomasterfrom
fix-shm-assertion-for-windows
Oct 6, 2019
Merged

Fix use of SHM for Selenium containers on Windows#1948
rnorth merged 8 commits intomasterfrom
fix-shm-assertion-for-windows

Conversation

@rnorth
Copy link
Copy Markdown
Member

@rnorth rnorth commented Oct 4, 2019

No description provided.

@rnorth rnorth requested review from bsideup and kiview as code owners October 4, 2019 18:52
@rnorth
Copy link
Copy Markdown
Member Author

rnorth commented Oct 4, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

by only checking counts of SHM volumes
@rnorth
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

rnorth commented Oct 4, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).


if (SystemUtils.IS_OS_WINDOWS && result.startsWith("/")) {
// Special cases for Windows
if (SystemUtils.IS_OS_WINDOWS && result.matches("^/[A-Z]:/dev/.*")) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

public void createContainerWithShmVolume() {
try (
BrowserWebDriverContainer webDriverContainer = new BrowserWebDriverContainer()
.withCapabilities(new FirefoxOptions())
Copy link
Copy Markdown
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
Copy Markdown
Member Author

rnorth commented Oct 5, 2019

/AzurePipelines run Windows 10 - Docker for Windows

@azure-pipelines
Copy link
Copy Markdown

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.

2 participants