Ensure only one instance of tunnel process runs #15834

OmSaran · 2023-02-12T03:57:26Z

Both outputs are for the second invocation

Before:

$ ./out/minikube tunnel
✅  Tunnel successfully started

📌  NOTE: Please do not close this terminal as this process must stay alive for the tunnel to be accessible ...

🏃  Starting tunnel for service balanced.

After:

$ ./out/minikube tunnel

💡  Exiting due to TUNNEL_ALREADY_RUNNING: Another tunnel process already running, terminate the older to start a new one

If cluster is not running, this is the behavior:
Cluster status:

$ ./out/minikube status
minikube
type: Control Plane
host: Stopped
kubelet: Stopped
apiserver: Stopped
kubeconfig: Stopped

Behavior:

$ ./out/minikube tunnel
🤷  The control plane node must be running for this command
👉  To start a cluster, run: "minikube start"

k8s-ci-robot · 2023-02-12T03:57:34Z

Hi @OmSaran. Thanks for your PR.

I'm waiting for a kubernetes 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.

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.

minikube-bot · 2023-02-12T03:59:05Z

Can one of the admins verify this patch?

cmd/minikube/cmd/tunnel.go

medyagh · 2023-02-12T17:25:22Z

cmd/minikube/cmd/tunnel.go

+		if os.IsNotExist(err) {
+			_, err = os.Create(tunnelLockPath)
+			if err != nil {
+				exit.Error(reason.SvcTunnelStart, fmt.Sprintf("error creating lock for tunnel file (%s)", tunnelLockPath), err)


Lets create a new reason in reason package and we can call it
TUNNEL_ALREADY_RUNNING

also make sure the exit message does not have github box since this is not a crash or issue. This is a usage problem.

look at minikube profile list code, when there is no cluster running it exits nicely (no need to show github issue box and crying emoji)

@medyagh

I've added the new reason as proposed.

Fixed the github box, however I've made this change only when we fail to acquire filelock (line 152). If there are errors in creating the file itself or creating the lock handle itself, then I have left it as is (with github box). Let me know if this is not the correct thing to do.

medyagh

Consider adding an integration test.

in functional test we already have a test that runs minikube tunnel, we could add a new test and run tunnel second time and expect it to fail on second time with exact error type

OmSaran · 2023-02-13T01:04:00Z

@medyagh I've added the integration test as well. Would be great if you could take a look!

medyagh

hi @OmSaran I requested a couple changes, but over all looks good

test/integration/functional_test_tunnel_test.go

cmd/minikube/cmd/tunnel.go

test/integration/functional_test_tunnel_test.go

cmd/minikube/cmd/tunnel.go

OmSaran · 2023-03-21T01:40:50Z

Hi, anyway I can help move this forward?

Looks like the new test you added fails on Windows

https://storage.googleapis.com/minikube-builds/logs/15834/28009/Docker_Windows.html#fail_TestFunctional/parallel/TunnelCmd/serial/RunSecondTunnel

https://storage.googleapis.com/minikube-builds/logs/15834/28303/Hyper-V_Windows.html#fail_TestFunctional/parallel/TunnelCmd/serial/RunSecondTunnel

I can think of 2 ways of debugging this:

Run this test manually on a windows machine.
Log the outputs in the code to take a look at the error in tests of CI/CD pipeline (I just pushed a commit to log stdout and stderr).

What do you think I should do? Or is there a better way to debug this? (Note that for 1. I'll need access to a windows machine which I don't have)

spowelljr

What do you think I should do? Or is there a better way to debug this? (Note that for 1. I'll need access to a windows machine which I don't have)

@OmSaran I ran it on Windows and found the issue. Window is finicky about open files, so you're creating the lock file with os.Create but not closing the file, on Windows you can't interact with a file if it's open in another process. So when you call fslock.New(tunnelLockPath) & lockHandle.TryLock() it throws an error as os.Create is the active one using it. I would have recommended you close the file after creating it, but I don't think all this open and creation code is needed as I believe fslock.New will handle creating the file for us. We have other cases of using fslock.New and we don't create the file ahead of time. I also tried removing all the file creation code and the file still existed for me after running the tunnel command.

cmd/minikube/cmd/tunnel.go

test/integration/functional_test_tunnel_test.go

spowelljr

Looks good, just one last thing, can you run make generate-docs to update the translations and test list, thanks!

OmSaran · 2023-03-24T02:18:59Z

Looks good, just one last thing, can you run make generate-docs to update the translations and test list, thanks!

Done, thank you for your patience and guidance!

minikube-pr-bot · 2023-03-24T03:36:53Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15834) |
+----------------+----------+---------------------+
| minikube start | 52.9s    | 54.3s               |
| enable ingress | 28.6s    | 28.3s               |
+----------------+----------+---------------------+

Times for minikube start: 53.6s 50.4s 52.5s 55.4s 52.3s
Times for minikube (PR 15834) start: 54.8s 54.0s 56.8s 54.9s 51.0s

Times for minikube ingress: 26.9s 28.8s 28.8s 29.8s 28.8s
Times for minikube (PR 15834) ingress: 29.3s 28.3s 28.8s 25.3s 29.8s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15834) |
+----------------+----------+---------------------+
| minikube start | 24.8s    | 24.8s               |
| enable ingress | 27.4s    | 22.3s               |
+----------------+----------+---------------------+

Times for minikube start: 25.1s 25.0s 24.6s 24.6s 24.9s
Times for minikube (PR 15834) start: 24.8s 25.4s 24.2s 25.0s 24.7s

Times for minikube ingress: 21.1s 51.1s 21.7s 21.1s 22.1s
Times for minikube (PR 15834) ingress: 22.2s 22.1s 22.1s 22.1s 23.1s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15834) |
+----------------+----------+---------------------+
| minikube start | 23.1s    | 22.8s               |
| enable ingress | 35.6s    | 36.0s               |
+----------------+----------+---------------------+

Times for minikube (PR 15834) ingress: 19.6s 47.6s 32.6s 32.7s 47.6s
Times for minikube ingress: 31.6s 32.6s 32.6s 33.6s 47.6s

Times for minikube start: 23.3s 22.8s 23.7s 23.5s 22.4s
Times for minikube (PR 15834) start: 22.7s 22.8s 22.9s 22.8s 22.7s

minikube-pr-bot · 2023-03-24T04:42:32Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Docker_Windows	TestRunningBinaryUpgrade (gopogh)	0.00 (chart)
Docker_Windows	TestStartStop/group/no-preload/serial/AddonExistsAfterStop (gopogh)	0.00 (chart)
Docker_Windows	TestStartStop/group/no-preload/serial/SecondStart (gopogh)	0.00 (chart)
Docker_Windows	TestStartStop/group/no-preload/serial/UserAppExistsAfterStop (gopogh)	0.00 (chart)
Hyperkit_macOS	TestNetworkPlugins/group/calico/Start (gopogh)	0.00 (chart)
Hyperkit_macOS	TestNetworkPlugins/group/custom-flannel/Start (gopogh)	0.00 (chart)
KVM_Linux	TestNetworkPlugins/group/flannel/Start (gopogh)	0.62 (chart)
Hyperkit_macOS	TestStoppedBinaryUpgrade/Upgrade (gopogh)	14.29 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddonActivation (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressAddons (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/serial/ValidateIngressDNSAddonActivation (gopogh)	100.00 (chart)
Docker_macOS	TestIngressAddonLegacy/StartLegacyK8sCluster (gopogh)	100.00 (chart)
Docker_macOS	TestKubernetesUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestMissingContainerUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestRunningBinaryUpgrade (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/AddonExistsAfterStop (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/DeployApp (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/EnableAddonWhileActive (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/FirstStart (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/SecondStart (gopogh)	100.00 (chart)
Docker_macOS	TestStartStop/group/old-k8s-version/serial/UserAppExistsAfterStop (gopogh)	100.00 (chart)
Docker_macOS	TestStoppedBinaryUpgrade/Upgrade (gopogh)	100.00 (chart)
KVM_Linux_containerd	TestPreload (gopogh)	100.00 (chart)

To see the flake rates of all tests by environment, click here.

spowelljr

Thanks for persisting through this PR and helping add this feature!

k8s-ci-robot · 2023-03-24T17:03:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: OmSaran, spowelljr

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~~ [spowelljr]

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

raylau1234 · 2023-07-07T01:49:31Z

What if I have multiple profiles with multi-node clusters?

spowelljr · 2023-07-07T17:08:02Z

What if I have multiple profiles with multi-node clusters?

There's no way to specify which node to use with minikube tunnel so I don't think the multi-node part is a concern, but you are correct that the lock file should be per profile and not a global one, good catch, I'll make a PR to fix this!

raylau1234 · 2023-07-07T17:10:56Z

There's no way to specify which node to use with minikube tunnel so I don't think the multi-node part is a concern, but you are correct that the lock file should be per profile and not a global one, good catch, I'll make a PR to fix this!

Thanks!!

Yes, that's what I was intending to say - multi profile... I said "multi node" bec that's the multiple profiles is introduced (AFAIK) in docs at https://minikube.sigs.k8s.io/docs/tutorials/multi_node/

spowelljr · 2023-07-07T17:28:52Z

Yes, that's what I was intending to say - multi profile... I said "multi node" bec that's the multiple profiles is introduced (AFAIK) in docs at https://minikube.sigs.k8s.io/docs/tutorials/multi_node/

Just to clarify, multiple profiles and multi-node are two different things.

You can start independent clusters with completely different configurations using the --profile flag. If you don't provide a --profile flag it defaults the flag to minikube.

Example:

$ minikube start -p profile1 --driver docker
😄  [profile1] minikube v1.30.1 on Darwin 13.4.1 (arm64)
...

$ minikube start -p profile2 --driver qemu --container-runtime=containerd
😄  [profile2] minikube v1.30.1 on Darwin 13.4.1 (arm64)
...

$ minikube profile list
|----------|-----------|------------|----------------|------|---------|---------|-------|--------|
| Profile  | VM Driver |  Runtime   |       IP       | Port | Version | Status  | Nodes | Active |
|----------|-----------|------------|----------------|------|---------|---------|-------|--------|
| profile1 | docker    | docker     | 192.168.49.2   | 8443 | v1.27.3 | Running |     1 |        |
| profile2 | qemu2     | containerd | 192.168.105.63 | 8443 | v1.27.3 | Running |     1 |        |
|----------|-----------|------------|----------------|------|---------|---------|-------|--------|

The link you posted is multi-node, which is adding worker nodes to a profile.

Example:

$ minikube start -n=3

# see the Nodes column below
$ minikube profile list
|----------|-----------|---------|--------------|------|---------|---------|-------|--------|
| Profile  | VM Driver | Runtime |      IP      | Port | Version | Status  | Nodes | Active |
|----------|-----------|---------|--------------|------|---------|---------|-------|--------|
| minikube | docker    | docker  | 192.168.49.2 | 8443 | v1.27.3 | Running |     3 | *      |
|----------|-----------|---------|--------------|------|---------|---------|-------|--------|

# below you can see there's an instance of kindnet and kube-proxy on each node, hence the three of each
$ kubectl get pods -A
kubectl get pod -A
NAMESPACE     NAME                               READY   STATUS    RESTARTS      AGE
kube-system   coredns-5d78c9869d-tw7xr           1/1     Running   1 (81s ago)   95s
kube-system   etcd-minikube                      1/1     Running   0             108s
kube-system   kindnet-22wzx                      1/1     Running   0             95s
kube-system   kindnet-n8bmm                      1/1     Running   0             90s
kube-system   kindnet-xrj72                      1/1     Running   0             76s
kube-system   kube-apiserver-minikube            1/1     Running   0             107s
kube-system   kube-controller-manager-minikube   1/1     Running   0             107s
kube-system   kube-proxy-bm529                   1/1     Running   0             95s
kube-system   kube-proxy-q7kr8                   1/1     Running   0             90s
kube-system   kube-proxy-wccfj                   1/1     Running   0             76s
kube-system   kube-scheduler-minikube            1/1     Running   0             109s
kube-system   storage-provisioner                1/1     Running   1 (64s ago)   106s

Hope that clears it up

raylau1234 · 2023-07-07T17:31:08Z

Understood... my usual practice is to have one profile per project.... but have multi nodes w/in the profile bec testing rack awareness is usually on the list....

So when I have 2 projects going (or sometimes even 3!), would love to be able to have 2 tunnels going.

spowelljr · 2023-07-07T17:37:19Z

So when I have 2 projects going (or sometimes even 3!), would love to be able to have 2 tunnels going.

For sure, that was an oversight by me when reviewing the PR, thanks for catching it before the release!

Here's the PR #16839

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 12, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 12, 2023

k8s-ci-robot requested review from afbjorklund and prezha February 12, 2023 03:57

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 12, 2023

OmSaran mentioned this pull request Feb 12, 2023

minikube tunnel should not be allowed to run twice #15825

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 12, 2023

medyagh requested changes Feb 12, 2023

View reviewed changes

OmSaran requested review from medyagh and removed request for prezha and afbjorklund February 13, 2023 01:09

OmSaran force-pushed the tunnel branch 3 times, most recently from ef2a630 to 40b1bb5 Compare February 13, 2023 14:56

medyagh requested changes Feb 14, 2023

View reviewed changes

test/integration/functional_test_tunnel_test.go Show resolved Hide resolved

cmd/minikube/cmd/tunnel.go Outdated Show resolved Hide resolved

spowelljr reviewed Feb 14, 2023

View reviewed changes

test/integration/functional_test_tunnel_test.go Outdated Show resolved Hide resolved

cmd/minikube/cmd/tunnel.go Outdated Show resolved Hide resolved

spowelljr requested changes Feb 14, 2023

View reviewed changes

cmd/minikube/cmd/tunnel.go Outdated Show resolved Hide resolved

OmSaran requested review from spowelljr and medyagh February 15, 2023 21:55

spowelljr requested changes Feb 16, 2023

View reviewed changes

cmd/minikube/cmd/tunnel.go Outdated Show resolved Hide resolved

OmSaran requested review from spowelljr and medyagh and removed request for medyagh and spowelljr February 16, 2023 17:38

Log stdout and stderr upon unexpected failure in RunSecondTunnel test

48cee32

OmSaran force-pushed the tunnel branch from 6969eed to 48cee32 Compare March 21, 2023 01:34

This comment has been minimized.

Sign in to view

spowelljr requested changes Mar 21, 2023

View reviewed changes

cmd/minikube/cmd/tunnel.go Outdated Show resolved Hide resolved

Remove unnecessary file creation for lock

b0547a1

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 22, 2023

This comment has been minimized.

Sign in to view

spowelljr requested changes Mar 22, 2023

View reviewed changes

test/integration/functional_test_tunnel_test.go Show resolved Hide resolved

spowelljr requested changes Mar 23, 2023

View reviewed changes

Generate docs

c6ea35f

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 24, 2023

spowelljr approved these changes Mar 24, 2023

View reviewed changes

spowelljr merged commit 10145a2 into kubernetes:master Mar 24, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2023

spowelljr mentioned this pull request Jul 7, 2023

tunnel: Allow one tunnel per profile #16839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure only one instance of tunnel process runs #15834

Ensure only one instance of tunnel process runs #15834

OmSaran commented Feb 12, 2023 •

edited

Loading

k8s-ci-robot commented Feb 12, 2023

minikube-bot commented Feb 12, 2023

medyagh Feb 12, 2023

OmSaran Feb 13, 2023 •

edited

Loading

medyagh left a comment

OmSaran commented Feb 13, 2023

medyagh left a comment

OmSaran commented Mar 21, 2023

This comment has been minimized.

This comment has been minimized.

spowelljr left a comment •

edited

Loading

This comment has been minimized.

This comment has been minimized.

spowelljr left a comment

OmSaran commented Mar 24, 2023

minikube-pr-bot commented Mar 24, 2023

minikube-pr-bot commented Mar 24, 2023

spowelljr left a comment

k8s-ci-robot commented Mar 24, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

Ensure only one instance of tunnel process runs #15834

Ensure only one instance of tunnel process runs #15834

Conversation

OmSaran commented Feb 12, 2023 • edited Loading

k8s-ci-robot commented Feb 12, 2023

minikube-bot commented Feb 12, 2023

medyagh Feb 12, 2023

Choose a reason for hiding this comment

OmSaran Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

medyagh left a comment

Choose a reason for hiding this comment

OmSaran commented Feb 13, 2023

medyagh left a comment

Choose a reason for hiding this comment

OmSaran commented Mar 21, 2023

This comment has been minimized.

This comment has been minimized.

spowelljr left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

spowelljr left a comment

Choose a reason for hiding this comment

OmSaran commented Mar 24, 2023

minikube-pr-bot commented Mar 24, 2023

minikube-pr-bot commented Mar 24, 2023

spowelljr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 24, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

raylau1234 commented Jul 7, 2023

spowelljr commented Jul 7, 2023

OmSaran commented Feb 12, 2023 •

edited

Loading

OmSaran Feb 13, 2023 •

edited

Loading

spowelljr left a comment •

edited

Loading