Skip to content

Test server listening on IPv4/IPv6 #2255

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

Merged
merged 25 commits into from
Mar 20, 2025
Merged

Conversation

mathbunnyru
Copy link
Member

Describe your changes

Issue ticket if applicable

Checklist (especially for first-time contributors)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • I will try not to use force-push to make the review process easier for reviewers
  • I have updated the documentation for significant changes

@mathbunnyru
Copy link
Member Author

All the credit goes to @manics and #2206

Just wanted to test a few different things here.

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Mar 20, 2025

The error we get is could not find an available, non-overlapping IPv6 address pool among the defaults to assign to the network

It was reported here: moby/moby#41438
Which was marked as duplicate of: moby/moby#42801
Which was fixed in: moby/moby#47768
And released in: https://github.com/moby/moby/releases/tag/v27.0.1, on Jun 25, 2024

Ubuntu 24.04 runner uses Docker Client 26: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md
And Docker Engine Version 26.* will be updated to Docker Engine Version 28* from 2025-05-09: actions/runner-images#11766

That's why I was not able to reproduce it locally, and self-hosted runners were working properly (they were using docker from docker repo, not ubuntu, so, more or less recent version has been installed).

My guess is that if I install latest docker inside our runners, it will work just fine.

Comment on lines 11 to 20
# Give some time for server to start
finish_time = time.time() + 10
sleep_time = 1
while time.time() < finish_time:
time.sleep(sleep_time)
try:
requests.get("http://localhost:8888/api")
break
except requests.RequestException:
pass
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a bug here - we fall through no matter if requests was successful or not, will fix it

@mathbunnyru
Copy link
Member Author

When running from python still uses old version 'Name': 'Engine', 'Version': '26.1.3'

@manics
Copy link
Contributor

manics commented Mar 20, 2025

Thanks for following this up, been busy with too many other things!

@mathbunnyru
Copy link
Member Author

[gw1] [ 98%] PASSED tests/by_image/base-notebook/test_ips.py::test_ipv46

Yep, made it work, now it's time to make it work for other tests as well 😃

@@ -4,7 +4,6 @@
from docker.models.containers import Container


def get_health(container: Container) -> str:
api_client = docker.APIClient()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is funny coincidence.
I refactored code around health check earlier today 😃

Didn't notice this bug.
We're using separate API client here, not the one from the docker_client fixture.

That's why it was impossible to make both healthcheck and ipv6 tests work.
Passing client as param and using it should fix the problem

- name: Set Up Docker 🐳
uses: docker/setup-docker-action@b60f85385d03ac8acfca6d9996982511d8620a19 # v4.3.0
with:
set-host: true
Copy link
Member Author

@mathbunnyru mathbunnyru Mar 20, 2025

Choose a reason for hiding this comment

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

This magic option changes anything - using it there is need to change other code around docker client creation, it will automatically use the latest docker client.

It is also more general, as it will also work for other places where we're creating docker client - in tagging, for example.

@mathbunnyru
Copy link
Member Author

x86_64-base works 🎉

@consideRatio
Copy link
Member

Wieeee nice debugging effort into this @mathbunnyru!!!!

@mathbunnyru
Copy link
Member Author

@manics @consideRatio may I ask you to review this change?

Fun fact - I didn't even read what code by @manics about ipv6 does while doing all this 😆

@manics please, also tell me if you're ok if I just squash merge this PR as is (so the commit will be under my name).
If not, please, let me know what I should do - in any case, I don't want to take someone else's work and tell that it's mine.

@mathbunnyru
Copy link
Member Author

Wieeee nice debugging effort into this @mathbunnyru!!!!

Thanks!
As you can see, it was quite a journey 😃

It's nice that when GitHub runners update their Docker version, we can just remove setup action, and everything else will stay the same (no changes to code at all).

@manics
Copy link
Contributor

manics commented Mar 20, 2025

tell me if you're ok if I just squash merge this PR as is (so the commit will be under my name).

Fine with me!

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

This seems right! Thank you @manics and @mathbunnyru!!

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Mar 20, 2025

Thanks!

All the images build and test just fine.
Rewrote check_listening.py a little bit to use one function for both ipv4 and ipv6, will merge this soon.

@mathbunnyru mathbunnyru merged commit b35f155 into jupyter:main Mar 20, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants