Invoke stable ABI compliant platform check in windows matcher #8778

kiashok · 2023-07-05T19:22:38Z

This PR does the following:

Uses the function introduced by Add support for platform compatibility check for windows microsoft/hcsshim#1821 to invoke stable ABI compliant function in windows platform matcher. (In this PR, this change is limited to platforms/default_windows.go file. platform/default_windows_test.go file has some new tests added).
Exact platform match is required for OS Versions < WS2022. The exact version match is not needed in versions > WS2022 due to stable ABI compliance.

k8s-ci-robot · 2023-07-05T19:22:48Z

Hi @kiashok. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kiashok · 2023-07-05T19:27:58Z

@jterry75 @dmcgowan @jsturtevant @dcantah could you please take a look at this PR when you have some time please?

dcantah · 2023-07-06T22:03:22Z

@kiashok Can you check the project check go.mod errors?

platforms/defaults_windows.go

dcantah · 2023-07-12T08:52:08Z

The project checks are still unhappy with the vendoring

kiashok · 2023-07-12T16:57:05Z

The project checks are still unhappy with the vendoring

go mod tidy is messing with the versions after making the change to integration/client/go.mod file . Taking a look

kiashok · 2023-07-13T17:40:28Z

The project checks are still unhappy with the vendoring

resolved :)

kiashok · 2023-07-13T17:41:15Z

@dmcgowan could you take a look when you have some time please?

kiashok · 2023-07-13T21:03:17Z

@estesp the test failures seem to be unrelated to my change. Could you please restart them?

dcantah · 2023-07-13T21:07:42Z

I restarted them, but the failures odd that it's every time for all windows runs

kiashok · 2023-07-13T23:13:54Z

I restarted them, but the failures odd that it's every time for all windows runs

Created a draft PR with just the hcsshim tag update changes and the same test seems to fail: #8811. Pretty unrelated to my change. Investigating..

kiashok · 2023-07-14T18:20:19Z

I restarted them, but the failures odd that it's every time for all windows runs

Created a draft PR with just the hcsshim tag update changes and the same test seems to fail: #8811. Pretty unrelated to my change. Investigating..

@dcantah doesn't seem like these are related to any of the changes in this PR. Is there a way to kick off the windows integration runs for existing current main branch to see if the tests pass or are flaky there?

kiashok · 2023-07-19T04:48:17Z

@dmcgowan Hi Derek there are some test failures in this PR related to the tests you added in import_test.go with #7964 . I see that this image is creating image with linux as the OS. Typically these TestTransferImport() functions should be skipped for windows OS runs right? Is my understanding correct? The tests are failing with some race condition while calling client.Transfer() and I am not sure how my changes are triggering this failures and was looking into this.

kiashok · 2023-07-21T15:32:02Z

@dmcgowan Hi Derek there are some test failures in this PR related to the tests you added in import_test.go with #7964 . I see that this image is creating image with linux as the OS. Typically these TestTransferImport() functions should be skipped for windows OS runs right? Is my understanding correct? The tests are failing with some race condition while calling client.Transfer() and I am not sure how my changes are triggering this failures and was looking into this.

The test failures here definitely seem to be a race in the logrus or transfer service and I've opened a bug here - #8853 . This is an already existing race condition which is just being exposed due to the slightly longer time that WindowsPlatformMatcher.Match() will take with the new changes. The TestMain() in integration/client/client_test.go calls client.Pull() in https://github.com/containerd/containerd/blob/main/integration/client/client_test.go#L152 where the platform matcher will be invoked before calling m.Run() that starts the different Transfer service tests. While I continue to take a look at how this data race can be resolved, could we unblock this PR as it is not causing these failures? Thoughts @jterry75 @dcantah ?

kiashok · 2023-07-21T17:19:25Z

@dmcgowan Hi Derek there are some test failures in this PR related to the tests you added in import_test.go with #7964 . I see that this image is creating image with linux as the OS. Typically these TestTransferImport() functions should be skipped for windows OS runs right? Is my understanding correct? The tests are failing with some race condition while calling client.Transfer() and I am not sure how my changes are triggering this failures and was looking into this.

The test failures here definitely seem to be a race in the logrus or transfer service and I've opened a bug here - #8853 . This is an already existing race condition which is just being exposed due to the slightly longer time that WindowsPlatformMatcher.Match() will take with the new changes. The TestMain() in integration/client/client_test.go calls client.Pull() in https://github.com/containerd/containerd/blob/main/integration/client/client_test.go#L152 where the platform matcher will be invoked before calling m.Run() that starts the different Transfer service tests. While I continue to take a look at how this data race can be resolved, could we unblock this PR as it is not causing these failures? Thoughts @jterry75 @dcantah @mikebrow ?

kiashok · 2023-07-26T16:52:40Z

@dmcgowan Hi Derek there are some test failures in this PR related to the tests you added in import_test.go with #7964 . I see that this image is creating image with linux as the OS. Typically these TestTransferImport() functions should be skipped for windows OS runs right? Is my understanding correct? The tests are failing with some race condition while calling client.Transfer() and I am not sure how my changes are triggering this failures and was looking into this.

The test failures here definitely seem to be a race in the logrus or transfer service and I've opened a bug here - #8853 . This is an already existing race condition which is just being exposed due to the slightly longer time that WindowsPlatformMatcher.Match() will take with the new changes. The TestMain() in integration/client/client_test.go calls client.Pull() in https://github.com/containerd/containerd/blob/main/integration/client/client_test.go#L152 where the platform matcher will be invoked before calling m.Run() that starts the different Transfer service tests. While I continue to take a look at how this data race can be resolved, could we unblock this PR as it is not causing these failures? Thoughts @jterry75 @dcantah @mikebrow ?

Ping on this :)

mikebrow · 2023-08-08T00:21:29Z

restarted failed buckets..

kiashok · 2023-08-08T21:37:19Z

restarted failed buckets..

the transfer service tests that are failing due to data race in transfer service do not even run for windows (checked recent PRs that are open and merged). I am not sure how a change in windows platform matcher is causing these tests to run on windows. Seems pretty unrelated to the changes I am making. Could this PR be merged as the changes are unrelated?
Opened a bug in containerd to track this #8853 and will look into this hopefully this week or the next.

cc @dmcgowan

mikebrow · 2023-08-08T23:44:11Z

you could rebase to pick up the latest ci/cd

kiashok · 2023-08-09T17:13:22Z

you could rebase to pick up the latest ci/cd

Hi @mikebrow , the failure does not show up as data race anymore. It looks like there is some trouble loading transfer service and there is a premature closure of the pipe? Has anything changed in transfer service lately? Would really appreciate your help with this!

kiashok · 2023-08-10T17:41:55Z

you could rebase to pick up the latest ci/cd

Hi @mikebrow , the failure does not show up as data race anymore. It looks like there is some trouble loading transfer service and there is a premature closure of the pipe? Has anything changed in transfer service lately? Would really appreciate your help with this!

@mikebrow there is a still a data race after rebasing this PR. One strange thing I do see is that TestTransferImport (oneo of the failing test) is being skipped for windows tests - https://github.com/containerd/containerd/actions/runs/5813022917/job/15759858411#step:19:104 as testing.short() evaluates to true. But in this PR its attempting to run these tests and I am not sure why (as this PR is doing nothing to change behavior). This makes me think that the data race showing up in PR test failures existed from day 1?

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>

kiashok · 2023-08-15T21:25:20Z

@mikebrow the windows integration test failures have now been fixed. The CI/Binaries (windows-2022, 1.19.12) says failed but when I access the logs, I do not see any failures/errors being reported. Do you know why it could be flagging the test suites as failed? Could you please restart this suite if needed?

kiashok · 2023-08-15T21:58:24Z

@mikebrow the windows integration test failures have now been fixed. The CI/Binaries (windows-2022, 1.19.12) says failed but when I access the logs, I do not see any failures/errors being reported. Do you know why it could be flagging the test suites as failed? Could you please restart this suite if needed?

Please ignore. I was able to do a force push to the PR to restart the tests without intervention. Thanks!

- Fill OSVersion field of ocispec.Platform for windows OS in transfer service plugin init() - Do not return error from transfer service ReceiveStream if stream.Recv() returned context.Canceled error Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>

jsturtevant

LGTM, one minor comment about test scenarios.

This looks like it would also fix #8563 with option number 3 in the issue (repopluate OS version during Parse). This works for now and we can address the Hyper-v scenarios later.

platforms/defaults_windows.go

kiashok · 2023-08-15T23:22:26Z

All CI runs are now passing

fuweid

LGTM

k8s-ci-robot added the needs-ok-to-test label Jul 5, 2023

kiashok changed the title ~~Invoke stable ABI compliant platform check in windows matcher~~ Invoke stable ABI compliant platform check in windows matcher + update hcsshim tag Jul 5, 2023

kiashok force-pushed the stableABI_WindowsMatcher branch from 6fbde64 to 63bc24c Compare July 5, 2023 21:52

kiashok force-pushed the stableABI_WindowsMatcher branch 2 times, most recently from c0a88ac to 78fd736 Compare July 11, 2023 23:29

dcantah reviewed Jul 12, 2023

View reviewed changes

platforms/defaults_windows.go Show resolved Hide resolved

kiashok force-pushed the stableABI_WindowsMatcher branch 2 times, most recently from 24f74bd to e6bb2e7 Compare July 13, 2023 17:27

jterry75 approved these changes Jul 13, 2023

View reviewed changes

kiashok force-pushed the stableABI_WindowsMatcher branch from e6bb2e7 to 31d7205 Compare July 18, 2023 00:42

kiashok changed the title ~~Invoke stable ABI compliant platform check in windows matcher + update hcsshim tag~~ Invoke stable ABI compliant platform check in windows matcher Jul 18, 2023

kiashok force-pushed the stableABI_WindowsMatcher branch from 31d7205 to 0e83df7 Compare July 18, 2023 23:00

kiashok force-pushed the stableABI_WindowsMatcher branch from 0e83df7 to de8f2b2 Compare July 19, 2023 18:29

kiashok mentioned this pull request Jul 20, 2023

Data race in transfer service tests #8853

Closed

kiashok force-pushed the stableABI_WindowsMatcher branch 2 times, most recently from 768a0ce to 2d3061d Compare July 21, 2023 04:10

kiashok force-pushed the stableABI_WindowsMatcher branch from 2d3061d to 14e61d5 Compare August 9, 2023 16:30

kiashok force-pushed the stableABI_WindowsMatcher branch from 14e61d5 to f49947d Compare August 10, 2023 16:48

kiashok mentioned this pull request Aug 15, 2023

[release/1.7] Run Integration 1 tests only #8967

Closed

Invoke Stable ABI compatibility function in windows platform matcher

cfb30a3

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>

kiashok force-pushed the stableABI_WindowsMatcher branch 2 times, most recently from 2789a8e to bcc8269 Compare August 15, 2023 20:22

kiashok force-pushed the stableABI_WindowsMatcher branch from bcc8269 to 884af45 Compare August 15, 2023 21:30

Fix transfer service dependencies:

823e042

- Fill OSVersion field of ocispec.Platform for windows OS in transfer service plugin init() - Do not return error from transfer service ReceiveStream if stream.Recv() returned context.Canceled error Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>

kiashok force-pushed the stableABI_WindowsMatcher branch from 884af45 to 823e042 Compare August 15, 2023 22:33

dmcgowan approved these changes Aug 15, 2023

View reviewed changes

jsturtevant reviewed Aug 15, 2023

View reviewed changes

platforms/defaults_windows.go Show resolved Hide resolved

fuweid approved these changes Aug 16, 2023

View reviewed changes

fuweid merged commit a3c68d1 into containerd:main Aug 16, 2023

fuweid added the platform/windows Windows label Aug 16, 2023

This was referenced Oct 10, 2023

Bump up containerd to v1.7.6 moby/buildkit#4301

Closed

vendor: github.com/containerd/containerd v1.7.7 moby/buildkit#4315

Merged

thaJeztah mentioned this pull request Jun 11, 2024

[release/1.7] Invoke Stable ABI compatibility function in windows platform matcher #9069

Merged

thaJeztah added the cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch label Jun 11, 2024

thaJeztah mentioned this pull request Jun 11, 2024

vendor: github.com/containerd/platforms v0.2.1 #10322

Merged

Invoke stable ABI compliant platform check in windows matcher #8778

Invoke stable ABI compliant platform check in windows matcher #8778

Uh oh!

Conversation

kiashok commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 5, 2023

Uh oh!

kiashok commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcantah commented Jul 6, 2023

Uh oh!

Uh oh!

dcantah commented Jul 12, 2023

Uh oh!

kiashok commented Jul 12, 2023

Uh oh!

kiashok commented Jul 13, 2023

Uh oh!

kiashok commented Jul 13, 2023

Uh oh!

kiashok commented Jul 13, 2023

Uh oh!

dcantah commented Jul 13, 2023

Uh oh!

kiashok commented Jul 13, 2023

Uh oh!

kiashok commented Jul 14, 2023

Uh oh!

kiashok commented Jul 19, 2023

Uh oh!

kiashok commented Jul 21, 2023

Uh oh!

kiashok commented Jul 21, 2023

Uh oh!

kiashok commented Jul 26, 2023

Uh oh!

mikebrow commented Aug 8, 2023

Uh oh!

kiashok commented Aug 8, 2023

Uh oh!

mikebrow commented Aug 8, 2023

Uh oh!

kiashok commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiashok commented Aug 10, 2023

Uh oh!

kiashok commented Aug 15, 2023

Uh oh!

kiashok commented Aug 15, 2023

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiashok commented Aug 15, 2023

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kiashok commented Jul 5, 2023 •

edited

Loading

kiashok commented Jul 5, 2023 •

edited

Loading

kiashok commented Aug 9, 2023 •

edited

Loading