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

Issue 1368: fix to _base_provider.py to fix install on MacOS #2823

Closed
wants to merge 2 commits into from
Closed

Issue 1368: fix to _base_provider.py to fix install on MacOS #2823

wants to merge 2 commits into from

Conversation

tsluyter
Copy link

@tsluyter tsluyter commented Jan 8, 2022

See issue #1368 . Related to the fact that the ping test may not work on MacOS HyperKit. Better to make it a more reliable test for connectivity.

Thank you for making MicroK8s better

Please reference the issue this PR is fixing, or provide a description of the problem addressed.

Also verify you have:

See issue #1368 . Related to the fact that the ping test may not work on MacOS HyperKit. Better to make it a more reliable test for connectivity.
@tsluyter tsluyter requested a review from joedborg as a code owner January 8, 2022 16:18
@tsluyter
Copy link
Author

tsluyter commented Jan 8, 2022

Link needed to issue #1368

@joedborg
Copy link
Contributor

Hey @tsluyter

Thank you for the PR! Can you please sign the CLA? https://github.com/ubuntu/microk8s/runs/4776381390?check_suite_focus=true

I'm also just wondering if we can get away with doing this implicitly for every install? Some corporate settings might block 8.8.8.8 or 1.1.1.1 and only allow their own DNS servers. Perhaps it should be something we fall back on or recommend to do if it fails?

Many thanks.

@tsluyter
Copy link
Author

Can you please sign the CLA? https://github.com/ubuntu/microk8s/runs/4776381390?check_suite_focus=true

I'd love to, but I reckon I need to be re-added to the right community. I was added to CanonicalContributorAgreement by @SallyCoome seemingly at random last week, but without any instructions and with a dead silent group and no clear indications of who Sally was, I assumed it was phishing.

Please provide exact instructions and re-invite me if needed. :)

I'm also just wondering if we can get away with doing this implicitly for every install? Some corporate settings might block 8.8.8.8 or 1.1.1.1 and only allow their own DNS servers. Perhaps it should be something we fall back on or recommend to do if it fails?

I agree that the DNS config itself is a naive approach; you're right that it might not work everywhere.

But, this particular PR isn't concerned with that particular topic. This PR tries to solve one simple problem where the internet-connectivity test fails in certain situations. I'll gladly come back later to also think about the DNS setup itself.

@tsluyter
Copy link
Author

tsluyter commented Jan 14, 2022

I see the linter also fails on a line of code that I didn't write.

I've changed the file in my fork, now need to figure out how I can update this running PR to use the adjusted file.

EDIT: Ah, it seems to have picked up the change by itself. Nice.

split the self.run() calls across multiple lines for readability.
@SallyCoome
Copy link

SallyCoome commented Jan 14, 2022 via email

@tsluyter
Copy link
Author

tsluyter commented Jan 14, 2022

Hello Tess, You were not added 'at random', you were added because you signed the Contributor Agreement per this screenshot. You are then added to the group on GitHub. I have resent the invitation to the GitHub group. Kind regards, Sally [image: image.png]

I did say "seemingly" ;) I meant, I had no idea who you were, so I was in doubt whether it's real. That issue has now clearly been resolved. :D I appreciate your help and joined the group, thank you!

@ktsakalozos
Copy link
Member

Hi @tsluyter, I understand the ping to snapcraft.io to check connectivity may fail in some installations. However, this PR seems to completely disable the connectivity check because it goes and asks the DNS of Google (8.8.8.8) if it knows snapcraft.io it never tries to connect to snapcraft.io. Google will always tell you "yes I know who snapcraft.io is" but later when you try to snap install the MicroK8s snap you may fail. Instead I would suggest we keep the ping as it works most of the times and when the ping fails try doing a GET request to snapcraft.io as a fallback.

@tsluyter
Copy link
Author

tsluyter commented Jan 23, 2022

I understand the ping to snapcraft.io to check connectivity may fail in some installations. However, this PR seems to completely disable the connectivity check because it goes and asks the DNS of Google (8.8.8.8) if it knows snapcraft.io it never tries to connect to snapcraft.io.

Correct. My reasoning being: if I can query an external DNS, I can assume that Internet-connectivity is also present and functional.

Instead I would suggest we keep the ping as it works most of the times and when the ping fails try doing a GET request to snapcraft.io as a fallback.

I was thinking along the same lines... but while we're at it, why not replace the ping outright with a GET request? ICMP may be blocked in some cases, while HTTPS is less likely to be. Unless you're in a corporate network ;)

@ktsakalozos
Copy link
Member

why not replace the ping outright with a GET request?

Sure we can try that.

Copy link

@mbarbero mbarbero left a comment

Choose a reason for hiding this comment

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

Added a suggestion that implement a HEAD request with curl rather than a ping or a nslookup (as per #2823 (comment))

Comment on lines -131 to +142
self.run("ping -c 1 snapcraft.io".split(), hide_output=True)
self.run(
"sed -i -e s/^#DNS=.*/DNS=8.8.8.8/ /etc/systemd/resolved.conf".split(),
hide_output=True,
)
self.run(
"systemctl restart systemd-resolved.service".split(),
hide_output=True
)
self.run(
"nslookup snapcraft.io".split(),
hide_output=True
)
Copy link

@mbarbero mbarbero Feb 23, 2022

Choose a reason for hiding this comment

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

Suggested change
self.run("ping -c 1 snapcraft.io".split(), hide_output=True)
self.run(
"sed -i -e s/^#DNS=.*/DNS=8.8.8.8/ /etc/systemd/resolved.conf".split(),
hide_output=True,
)
self.run(
"systemctl restart systemd-resolved.service".split(),
hide_output=True
)
self.run(
"nslookup snapcraft.io".split(),
hide_output=True
)
self.run("curl -IsSLf https://snapcraft.io".split(), hide_output=True)

This pull request was closed.
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.

5 participants