Jenkinsfile: Use new windows labels#39747
Conversation
d450521 to
4ff07d0
Compare
ddebroy
left a comment
There was a problem hiding this comment.
Thanks for this PR! Just a minor comment. Otherwise lgtm.
hack/ci/windows.ps1
Outdated
There was a problem hiding this comment.
I think this tag step may not be necessary any more. Do you know if anything fails if the tagging is skipped?
There was a problem hiding this comment.
I had to create the microsoft/windowsservercore tag to make it work on new CI.
I saw changes from @thaJeztah in our internal repos, maybe that's not upstreamed yet.
For this PR I didn't want to mix other PR's into one.
There was a problem hiding this comment.
I saw changes from @thaJeztah in our internal repos, maybe that's not upstreamed yet.
For this PR I didn't want to mix other PR's into one.
Perhaps we should upstream those changes before this PR (to reduce merge-conflicts once it gets synced with our internal repositories)
|
Current status with new CI infra: We can compile, and run the unit tests and integration tests on Windows Server 2016 and 2019. Errors that appear at the moment in the integration tests:
This should be investigated/fixed in separate PR's. @tiborvass This PR is no longer a Draft. |
|
Pushed another commit to fix |
2cdae31 to
8618a58
Compare
8618a58 to
3091aca
Compare
|
I'd really prefer to get the new PR check status green since the legacy PR checks for windowsRS1 and windowsRS5-process are green and we depend on those for merge ready status. The windows-2016 error seems repeatable, @ddebroy should this test be disabled or fixed? I see a new windows-2019 error: Also, should Windows Defender be disabled? |
|
Good catch, @andrewhsu . Windows Defender should indeed be disabled. The 2019/rs5 failure signature
|
3091aca to
ad80995
Compare
|
New CI is green, and |
Jenkinsfile
Outdated
There was a problem hiding this comment.
Can you use 1 here? It's what we do elsewhere
| $env:SKIP_VALIDATION_TESTS="true" | |
| $env:SKIP_VALIDATION_TESTS="1" |
Jenkinsfile
Outdated
There was a problem hiding this comment.
Is there an advantage to setting these environment variables in powershell instead of using something like;
environment {
DOCKER_BUILDKIT = '0'
SKIP_VALIDATION_TESTS = '1'
SOURCES_DRIVE = 'd'
SOURCES_SUBDIR = 'gopath'
TESTRUN_DRIVE = 'd'
TESTRUN_SUBDIR = $(BUILD_NUMBER)
WINDOWS_BASE_IMAGE = 'mcr.microsoft.com/windows/servercore'
WINDOWS_BASE_IMAGE_TAG = 'ltsc2016'
}There was a problem hiding this comment.
All right, that looks better. I'll update the Jenkinsfile
hack/ci/windows.ps1
Outdated
There was a problem hiding this comment.
I saw changes from @thaJeztah in our internal repos, maybe that's not upstreamed yet.
For this PR I didn't want to mix other PR's into one.
Perhaps we should upstream those changes before this PR (to reduce merge-conflicts once it gets synced with our internal repositories)
Jenkinsfile
Outdated
Signed-off-by: Stefan Scherer <stefan.scherer@docker.com>
ad80995 to
ca3e230
Compare
|
@StefanScherer Seeing this on a previous run of RS1; https://ci.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-39747/runs/7/nodes/213/log/?start=0 For RS5 I also see https://ci.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-39747/runs/7/nodes/183/log/?start=0 Not sure if Build proceeded after that, but looks like there may be something to prevent the error |
|
Both RS1 and RS5 seem to hit some cleanup step in the middle of tests (why?) |
|
Yes, in the middle of the docker build both agents. Looks like someone accidentally hit the abort button. |
|
Jenkins is weird; how can it skip the DCO check??
|
|
@StefanScherer the PR check that looked like an abort was actually the 2-hr timelimit being hit. Jenkins will send a signal to the process to bow out. |
|
@tiborvass i think this PR is ready for the effects of mergeability |
|
@StefanScherer @andrewhsu this will bring the CI time to over 1h as windows-RS1 took 1h43min. We should probably do something similar to what we do in janky here #39682 |
|
@benhillis I understand you concerns. We have problems on RS1 with The old pet machines also have OnAccessProtectionEnabled set to false for a very long time. I've only added that check to ensure the currently needed settings are given on the nodes and to give a hint how to reach that. It would be great to have a change/PR, so that the integration tests could also run with realtime protection enabled. |
|
@StefanScherer if it's only those two tests, could we somehow perform that check from the Go code, and skip those tests if moby/integration/build/build_cgroupns_linux_test.go Lines 73 to 75 in 3042254 |
|
I would prefer skipping tests that do not work correctly rather than failing the entire suite. |
|
@benhillis @StefanScherer I opened #39804 for now, so that we can continue looking for the better approach. |
Signed-off-by: Stefan Scherer stefan.scherer@docker.com
Opening on behalf of @StefanScherer
Ref #39691