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

[release/6.0] Single-file apps should not use copy-on-write mapping of the .exe on windows. #59194

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 16, 2021

Backport of #59033 to release/6.0

/cc @agocke @VSadov

Fixes: #55405 (App published as single file fails to run in Windows 10.0.17763 containers)

Customer Impact

Regression from .NET Core 2.1 and 3.1 causing failure when a single-file app runs in a Windows 10.0.17763 container.

(This is a relatively old 2018 RS5 build, but it is an LTS so it will be around for a while)

Testing

Manually verified in an actual container. Regular Unit tests.

Risk

Very low.

We do not benefit from copy-on-write part of the mapping on Windows since we do not do in-situ json parsing due to UTF8/UTF16 differences. We basically request COW for consistency with Unix. Somehow it triggers unexpected "access denied" failure for superhost-style single-file apps on this particular build/container.

Since we do not do in-situ parsing on Windows, a read-only mapping is sufficient and we should request just what we need.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please take a look at the failing CI and we can take for consideration in RC2.

@ghost
Copy link

ghost commented Sep 16, 2021

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #59033 to release/6.0

/cc @agocke @VSadov

Fixes: #55405 (App published as single file fails to run in Windows 10.0.17763 containers)

Customer Impact

Regression from .NET Core 2.1 and 3.1 causing failure when a single-file app runs in a Windows 10.0.17763 container.

(This is a relatively old 2018 RS5 build, but it is an LTS so it will be around for a while)

Testing

Manually verified in an actual container. Regular Unit tests.

Risk

Very low.

We do not benefit from copy-on-write part of the mapping on Windows since we do not do in-situ json parsing due to UTF8/UTF16 differences. We basically request COW for consistency with Unix. Somehow it triggers unexpected "access denied" failure for superhost-style single-file apps on this particular build/container.

Since we do not do in-situ parsing on Windows, a read-only mapping is sufficient and we should request just what we need.

Author: github-actions[bot]
Assignees: -
Labels:

area-Single-File

Milestone: -

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Sep 16, 2021
@jeffschwMSFT jeffschwMSFT added this to the 6.0.0 milestone Sep 16, 2021
@VSadov
Copy link
Member

VSadov commented Sep 16, 2021

bunch of legs cancelled with

The agent did not connect within the alloted time of 45 minute(s).

is there a way to un-cancel?

@janvorli
Copy link
Member

@VSadov rerunning the legs usually helps. If not, then closing / reopening the PR would re-run all the legs.

@VSadov
Copy link
Member

VSadov commented Sep 16, 2021

I did not see an option to rerun on any leg, so i wondered if i would need to close/reopen.
Now it seems unstuck again.

@VSadov VSadov closed this Sep 16, 2021
@VSadov VSadov reopened this Sep 16, 2021
@danmoseley
Copy link
Member

@MattGal here is another case where there are a bunch of canceled ones after close/reopen a PR. This time they didn't clean themselves up. I'm guessing @VSadov didn't cancel them.

@MattGal
Copy link
Member

MattGal commented Sep 16, 2021

@MattGal here is another case where there are a bunch of canceled ones after close/reopen a PR. This time they didn't clean themselves up. I'm guessing @VSadov didn't cancel them.

Based off what was happening (machines failing to "phone home" after provisioning) this is what I would expect.

I was just on a call with @ulisesh and we narrowed this down to a setting about interactive login for the agent. While some repos that do UI-testing will be adversely affected by this, un-setting the value seemed to immediately allow machines to provision at scale (even when their provisioning scripts "fail"; this seems to be a red herring found on the way to the answer).

The interactive login setting is off now, so clicking through to AzDO and doing "Rerun failed jobs" should now work (I tried it here to check)

@danmoseley
Copy link
Member

@mdh1418 @steveisok is this a known issue?

               adb: failed to install /datadisks/disk1/work/B2C50999/w/ABD90955/e/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.apk: Failure [INSTALL_PARSE_FAILED_BAD_PACKAGE_NAME: Invalid manifest package: bad character '1']
                 

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-59194-merge-cc1b7cc32c704243a1/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.Attempt.3/1/console.a68ff716.log?sv=2019-07-07&se=2021-10-06T19%3A17%3A32Z&sr=c&sp=rl&sig=469W5Gc9%2BNsfQxKCfVL%2B%2Bn97GUC0SF5e3nDGPcLcT7o%3D

@steveisok
Copy link
Member

Yes, spotted it earlier today.

We use the name of the project as the name of the android app. The numbers in the name violate android naming rules. We'll be able to address them tomorrow.

@danmoseley
Copy link
Member

Any idea how it got through the system though? This project was added by #59074 and as far as I can tell, all its runtime-staging lanes passed.

@steveisok
Copy link
Member

@danmoseley You won't see the test failures unless you look on the azdo side.

https://dev.azure.com/dnceng/public/_build/results?buildId=1361380&view=results

@VSadov
Copy link
Member

VSadov commented Sep 17, 2021

All tests green, finally.

@agocke
Copy link
Member

agocke commented Sep 17, 2021

Ironically, I think the right branch for this is now -rc2

@agocke agocke changed the base branch from release/6.0 to release/6.0-rc2 September 17, 2021 06:25
@agocke agocke force-pushed the backport/pr-59033-to-release/6.0 branch from 33ac523 to 2f0f10c Compare September 17, 2021 06:26
@agocke agocke added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 17, 2021
@danmoseley
Copy link
Member

logged both failures: should be good to merge.

@danmoseley danmoseley merged commit c3b2aa4 into release/6.0-rc2 Sep 17, 2021
@danmoseley danmoseley deleted the backport/pr-59033-to-release/6.0 branch September 17, 2021 12:55
@danmoseley
Copy link
Member

@steveisok

@danmoseley You won't see the test failures unless you look on the azdo side.

Should I expect a yellow on the Github side though? I mean, what is the indication that someone should go look at the azdo side?

@steveisok
Copy link
Member

I wish that was the case, but from what I understand is that we can't due to AzDo limitations. With the staging pipeline, we have to go in and look.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Single-File Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants