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.

Sign up for GitHub

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

Jump to bottom

Kill processes after completing build and before publishing artifacts #1023

Merged
merged 4 commits into from
Feb 1, 2019
Merged

Kill processes after completing build and before publishing artifacts #1023

merged 4 commits into from
Feb 1, 2019

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Jan 30, 2019 •
edited
Loading

  • aspnet/AspNetCore-Internal#1735

@dougbu
Copy link
Member Author

dougbu commented Jan 30, 2019

Oops, meant to update my comments: It's artifact publication that caused aspnet/AspNetCore-Internal#1735. Doesn't look like a sign check is enabled in this repo.

ryanbrandenburg
ryanbrandenburg approved these changes Jan 30, 2019
View reviewed changes
Copy link

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Are we confident that no process will be holding a lock on binlong in a failure case? If so let's try it.

natemcmaster
natemcmaster suggested changes Jan 30, 2019
View reviewed changes
eng/common/build.ps1 Outdated Show resolved Hide resolved
@natemcmaster
Copy link

I don't think signcheck is running on Arcade builds, btw. dotnet/arcade#1444. AspNetCore is the only repo running this tool right now, AFAIK. Some other process must be holding on to the binlog file.

@dougbu
Copy link
Member Author

dougbu commented Jan 30, 2019 •
edited
Loading

Are we confident that no process will be holding a lock on binlong in a failure case?

Thinking more about this, I suspect the Copy-Item attempt could also fail when the job is cancelled. The aspnet/AspNetCore-Internal#1735 failure occured because a later build step attempted to upload the binary log before it killed the msbuild process. Should change the condition from failure() to a check that TASK_STATUS is Falure or SucceededWithIssues. Another option would be to kill all msbuild processes before attempting the copy.

Uh oh, …

The other way 'round here would be to add a different step to PublishBuildArtifacts@1 which kills msbuild processes. Something similar to a step we have in most build configurations on http://aspnetci/ Thoughts?

@dougbu dougbu changed the title Move Build.binlog file to repo root; include it in artifacts on failure Kill processes after completing build and before publishing artifacts Jan 31, 2019
@dougbu
Copy link
Member Author

dougbu commented Jan 31, 2019

🆙📅 to change the way we avoid locked files completely. Bit more like our Teamcity infrastructure.

And, no, I couldn't find a built-in task that does anything similar.

@natemcmaster
Copy link

line 1: eng/scripts/KillProcesses.sh: Permission denied

I think you need to chmod +x.

natemcmaster
natemcmaster approved these changes Jan 31, 2019
View reviewed changes
Copy link

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

A few little tweaks needed, then :shipit:

@dougbu
Copy link
Member Author

dougbu commented Feb 1, 2019

@natemcmaster while #1043 remains open and tests continue to fail or hang (worse than flaky at the moment), what are your thoughts on retargeting this to release/2.1?

@dougbu dougbu mentioned this pull request Feb 1, 2019
Closed
@dougbu
Copy link
Member Author

dougbu commented Feb 1, 2019

@natemcmaster while #1043 remains open and …

Well, fifth time was the charm 🎉 🎆 🎉

New question: Should I back-port this feature to the release/2.1 and release/2.2 branches?

@dougbu dougbu merged commit e2bb8ac into master Feb 1, 2019
@dougbu dougbu deleted the dougbu/move.binlog.1735 branch February 1, 2019 07:12
@natemcmaster
Copy link

Should I back-port this feature to the release/2.1 and release/2.2 branches?

Yeah, probably worth doing.

dougbu added a commit that referenced this pull request Feb 4, 2019
@dougbu
Kill processes after completing build and before publishing artifacts (…
4e09bc6 
…#1023)

- dotnet/aspnetcore-internal#1735
- cherry pick e2bb8ac from `master`
  - change `default-build.yml` instead of `azure-pipelines.yml`
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@natemcmaster natemcmaster natemcmaster approved these changes

@ryanbrandenburg ryanbrandenburg ryanbrandenburg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dougbu @natemcmaster @ryanbrandenburg