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

Minikube: more options for connectivity #2312

Merged
merged 3 commits into from
Oct 21, 2021

Conversation

markmandel
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup

/kind documentation

/kind feature
/kind hotfix

What this PR does / Why we need it:

Minikube is a bit of a nightmare, since depending on your OS and your driver, you may or may not be able to connect to a UDP GameServer, with or without workarounds.

These changes capture two things:

  1. A few more workaround to try and some dead ends to avoid.
  2. Which OS+driver combos are known to work.
  3. That it may not actually be possible, in which case, it's time to try something else.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

@markmandel markmandel added kind/documentation Documentation for Agones kind/cleanup Refactoring code, fixing up documentation, etc labels Oct 14, 2021
@google-cla google-cla bot added the cla: yes label Oct 14, 2021
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0b33a33a-97a3-4ec1-8c3c-3bdf25e7ee11

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Member Author

  Compiling async-stream v0.3.2
   Compiling tracing v0.1.29
   Compiling thiserror v1.0.30
   Compiling pin-project v1.0.8
   Compiling tracing-futures v0.2.5
   Compiling prost v0.8.0
   Compiling prost-types v0.8.0
   Compiling tonic-build v0.5.2
   Compiling tokio-util v0.6.8
   Compiling tokio-stream v0.1.7
   Compiling tokio-io-timeout v1.1.1
   Compiling h2 v0.3.6
   Compiling tower v0.4.9
error[E0710]: an unknown tool name found in scoped lint: `rustdoc::broken_intra_doc_links`
 --> /go/src/agones.dev/agones/test/sdk/rust/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tower-0.4.9/src/lib.rs:8:9
  |
8 | #![deny(rustdoc::broken_intra_doc_links)]
  |         ^^^^^^^

   Compiling agones v0.1.0 (/go/src/agones.dev/agones/sdks/rust)
error: aborting due to previous error

For more information about this error, try `rustc --explain E0710`.
error: could not compile `tower`

includes/sdk.mk:168: recipe for target 'run-sdk-conformance-test-rust' failed rust sdk conformance failure.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: cce5eb14-b82a-434a-a214-99a58adb974f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b9efd895-b73e-4a60-acf9-d79e0ad5c451

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2312/head:pr_2312 && git checkout pr_2312
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-9c946b6

{{< /alert >}}

### Known working drivers

Other OS's and drivers may work, but at this stage have not been verified.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should this be OSes?

Copy link
Member

Choose a reason for hiding this comment

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

Or if we can't figure out the proper english, we could write it out as Operating Systems.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm definitely into clearer words. Switching to "Operating Systems".

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [markmandel,roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines 54 to 55
* hyper-v (might need [this comment](https://github.com/microsoft/WSL/issues/4288#issuecomment-652259640)
for WSL support)
Copy link
Contributor

@mvlabat mvlabat Oct 16, 2021

Choose a reason for hiding this comment

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

Users will likely need more workarounds to make WSL work. When I was experimenting with it, I was also partially following this guide:
https://blog.thepolyglotprogrammer.com/setting-up-kubernetes-on-wsl-to-work-with-minikube-on-windows-10-90dac3c72fa1

It mentions that users will need to use the Windows version of minikube and manually edit kubectl config to point it to the Windows certificates.

I found out that Windows version of minikube is optional unless you need UDP support, but using Linux minikube will also require you to use minikube tunnel.

In my opinion, we should either lead a user through the whole setup for WSL if we feel like we want to or avoid mentioning it at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the blog post link, added!

In my opinion, we should either lead a user through the whole setup for WSL if we feel like we want to or avoid mentioning it at all.

I'm happy to point to external resources, but these things go stale so quick, I'd rather not maintain a guide to setting up an external tool that we don't maintain.

Comment on lines 102 to 108
{{< alert title="Warning" color="warning">}}
`minikube tunnel` does not support UDP ([Github Issue](https://github.com/kubernetes/minikube/issues/12362)) on some
combination of OS platforms and drivers.
{{< /alert >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth giving a bit more context on why users may need to use minikube tunnel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Added!

Minikube is a bit of a nightmare, since depending on your OS and your
driver, you may or may not be able to connect to a UDP GameServer, with
or without workarounds.

These changes capture two things:

1. A few more workaround to try and some dead ends to avoid.
2. Which OS+driver combos are known to work.
3. That it may not actually be possible, in which case, it's time to try
 something else.
@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: f9b948c1-272e-4fb2-9144-2ad78b8abc06

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2312/head:pr_2312 && git checkout pr_2312
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-a5a7422

@roberthbailey
Copy link
Member

@mvlabat - want to take another look?

Comment on lines +105 to +108
`minikube tunnel` ([docs](https://minikube.sigs.k8s.io/docs/handbook/accessing/))
does not support UDP ([Github Issue](https://github.com/kubernetes/minikube/issues/12362)) on some combination of
operating system, platforms and drivers, but is required when using the `Service` workaround.
{{< /alert >}}
Copy link
Contributor

@mvlabat mvlabat Oct 20, 2021

Choose a reason for hiding this comment

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

From what I gathered when testing the workarounds locally, minikube tunnel is actually a self-sufficient workaround itself and can be used without creating a separate service.

minikube tunnel tunnels the traffic itself, thus making gameserver pods accessible from the host. On the setups where the address returned by minikube ip isn't accessible from the host, the service workaround will be useless, as the service is still behind the minikube ip address.

So if the purpose of the "Create a service" workaround is to tunnel the traffic to the host, I'm afraid it doesn't work at all (it'll be accessible if using minikube tunnel, but that defeats the purpose of the service then). Please correct me if I misinterpreted what the service workaround is supposed to do or if it did work in some cases. So my suggestion is to remove "Create a service" section completely and just mention minikube tunnel -p agones command as the workaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy pasting from https://minikube.sigs.k8s.io/docs/handbook/accessing/

Services of type LoadBalancer can be exposed via the minikube tunnel command. It must be run in a separate terminal window to keep the LoadBalancer running.

From: https://minikube.sigs.k8s.io/docs/commands/tunnel/

tunnel creates a route to services deployed with type LoadBalancer and sets their Ingress to their ClusterIP. for a detailed example see https://minikube.sigs.k8s.io/docs/tasks/loadbalancer

So as far as I read it, minikube tunnel can only be used with a Service. As far as I can tell it doesn't do anything else, so I don't think we should remove the Service workaround section, since it's part of the possible workaround solution.

I don't see anything in the documentation that states that minikube tunnel connects the node to the host machine for generalised traffic (maybe it does do that though on some platforms??? 🤔 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think you're right. I believe I tested it on a service that happened to be also a load balancer (and didn't notice that), so this is why I got such an impression.

I've got no other comments or suggestions then. Thanks a lot for updating the documentation!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for working through so much of this with me!

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 050517fe-0c46-49fd-b84b-adeb33159e14

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@roberthbailey
Copy link
Member

SDK conformance flaked.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: fada9015-8358-4000-a7c8-3ef02d847e82

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2312/head:pr_2312 && git checkout pr_2312
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-0caa728

@roberthbailey roberthbailey merged commit 717aa12 into googleforgames:main Oct 21, 2021
@markmandel markmandel deleted the docs/minikube-connect branch October 25, 2021 21:39
@roberthbailey roberthbailey added this to the 1.19.0 milestone Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cla: yes kind/cleanup Refactoring code, fixing up documentation, etc kind/documentation Documentation for Agones size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants