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

Alpine > Common #3632

Merged
merged 3 commits into from
Nov 4, 2024
Merged

Alpine > Common #3632

merged 3 commits into from
Nov 4, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Nov 1, 2024

On top of #3631

More changes to allow better compatibility with windows test (move from AlpineImage to CommonImage where appropriate).

Second commit fixes an image test that did not match the description.

@apostasie apostasie changed the title Series 14b Alpine > Common Nov 1, 2024
// that does not support inf/infinity.
// This constant is provided as a mean for tests to express the intention of sleep infinity without having to
// worry about that and get windows compatibility.
Infinity = "3600"
Copy link
Member

Choose a reason for hiding this comment

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

This is a finite number 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an infinitely finite number, right? :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make this an infinite number, but I am pretty sure you are going to complain about the size of the PR 😂

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use "18446744073709551615" (or "4294967295" if it overflows)?
It should be also moved to *_windows.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this is worth the effort.

For all intents and purposes, these containers never live more than a few seconds.
Furthermore, having two distinct behaviors for linux (infinity) and windows (4294967295) is inviting trouble.
Finally, this is just a workaround for a situation that hopefully will resolve eventually.

wdyt?

@@ -29,6 +29,14 @@ import (
"github.com/containerd/nerdctl/v2/pkg/testutil/test"
)

const (
// It seems that at this moment, the busybox on windows image we are using has an outdated version of sleep
Copy link
Member

Choose a reason for hiding this comment

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

Can't we update the image?

Copy link
Member

Choose a reason for hiding this comment

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

busybox should have been supporting inf since 2019 🤔
mirror/busybox@edca770

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried with #3617 - for some reason that escapes me, this still somehow ship with outdated sleep... more details in the dependent PR: #3631

Maybe we could move away from k8 image, but I have no clue how to build this busybox-window-frankenstein thing :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure the k8s folks are using https://github.com/rmyorston/busybox-w32
We could get our own rolled-up with some effort.
I do not have a windows rig here though... touching anything windows for me means I have to code blind and hope the CI will be happy :s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like everybody is still on 1.29-2 - untouched in 6 years... k8s image tag is likely lying for windows.

@apostasie apostasie marked this pull request as ready for review November 2, 2024 22:55
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Nov 4, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 2b0550f into containerd:main Nov 4, 2024
26 checks passed
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants