[WIP] Fix ValidateTunnelCmd test #8729
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @11janci. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 11janci The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Can one of the admins verify this patch? |
|
Failed test on windows hyperv |
|
@11janci thanks for picking this task 2020-07-15T18:45:18.4696923Z --- FAIL: TestFunctional/parallel/TunnelCmd/serial/AccessDirect (148.13s) |
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
from
35efb9b
to
74da82c
Compare
Codecov Report
@@ Coverage Diff @@
## master #8729 +/- ##
=======================================
Coverage 31.52% 31.52%
=======================================
Files 166 166
Lines 11325 11325
=======================================
Hits 3570 3570
Misses 7322 7322
Partials 433 433 |
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
3 times, most recently
from
e0d3848
to
d58c6cd
Compare
k8s-ci-robot
added
size/M
Travis tests have failedHey @11janci, 1st Buildmake testTravisBuddy Request Identifier: 35dbf8d0-c749-11ea-999c-bf5bae978323 |
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
from
eb79da3
to
f4d2cdb
Compare
k8s-ci-robot
added
size/S
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
from
f4d2cdb
to
21b9eea
Compare
|
@medyagh I'm afraid I'm gonna need some help with this. For For Any help would be welcome! |
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
from
21b9eea
to
8f37f21
Compare
k8s-ci-robot
added
size/M
11janci
force-pushed
the
jjanik-fix-validateTunnelCmd
branch
from
86c5b30
to
bb681f0
Compare
This is expected -- the Docker driver uses the loopback interface. I wouldn't expect it to be on port 80, however.
|
tstromberg
reviewed
Oct 28, 2020
| if runtime.GOOS == "windows" { | ||
| t.Skip("skipping: access direct test is broken on windows: https://github.com/kubernetes/minikube/issues/8304") | ||
| } | ||
| //if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
This should continue to be disabled for the Docker driver.
tstromberg
reviewed
Oct 28, 2020
| @@ -435,7 +435,9 @@ jobs: | |||
| $env:MINIKUBE_HOME="${pwd}\testhome" | |||
| .\minikube-windows-amd64.exe delete --all --purge | |||
| Get-VM | Where-Object {$_.Name -ne "DockerDesktopVM"} | Foreach { | |||
| Stop-VM -Name $_.Name -Force | |||
| .\minikube-windows-amd64.exe delete -p $_.Name | |||
| Suspend-VM $_.Name | |||
There was a problem hiding this comment.
I don't think the Suspend-VM/Stop-VM call should be required.
|
Closing this stale PR - please reopen when you have a chance to address the PR comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8304
On windows, when tests run
minikube tunnel, it does not inherit the elevated privileges of the parent process - it needs to be set explicitly during the invocation. The chosen approach is using powershell and Start-Process because (almost) no additional code changes are required (especially for termination). Other possible approach would be through lower level win specific API, but the required code changes would be much larger.