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.

Sign up for GitHub

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

Jump to bottom

Change port forwarding on windows #9679

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Change port forwarding on windows #9679

merged 1 commit into from
Mar 27, 2024

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Jan 23, 2024

Remove dependance on wincat.exe for port forwarding on windows

@kiashok
Copy link
Contributor Author

kiashok commented Jan 23, 2024 •
edited
Loading

cc @sbangari @jsturtevant

@dcantah
Copy link
Member

dcantah commented Jan 24, 2024

I gather we were using wincat because there's no (afaik) documented way to join a network compartment? There definitely was a way, but it eludes me now.. I think you needed a job object handle 😅

@kiashok
Copy link
Contributor Author

kiashok commented Jan 24, 2024 •
edited
Loading

I gather we were using wincat because there's no (afaik) documented way to join a network compartment? There definitely was a way, but it eludes me now.. I think you needed a job object handle 😅

I think if you use dial the IP address of the pod, that is not needed? Was it specific to some edge case or smth? Its working as expected on AKS when I tested.
cc @sbangari could you confirm if a job object handle is needed?

@dcantah
Copy link
Member

dcantah commented Jan 27, 2024

@kiashok Ignore the job object handle bit, just me trying to remember how that API worked out loud, nothing related to if this would work.

mikebrow
mikebrow reviewed Jan 31, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comment about the err msg..

will all runtimes in the windows version be able to do the port routing from the root host here?

pkg/cri/server/sandbox_portforward_windows.go Outdated Show resolved Hide resolved
@kiashok
Copy link
Contributor Author

kiashok commented Feb 3, 2024

see comment about the err msg..

will all runtimes in the windows version be able to do the port routing from the root host here?

it works fine in testing. for HPC port forwarding is not allowed

@kiashok kiashok force-pushed the portForwardingWindows-main branch 2 times, most recently from 72b8597 to 607f21b Compare February 3, 2024 21:58
@kiashok
Copy link
Contributor Author

kiashok commented Mar 6, 2024

@MikeZappa87 could you please take a look when you have some time please?

@kiashok
Copy link
Contributor Author

kiashok commented Mar 6, 2024

@fuweid @thaJeztah @cpuguy83 could you please take a look when you have some time please?

@kiashok kiashok force-pushed the portForwardingWindows-main branch 2 times, most recently from 886f1af to e408979 Compare March 7, 2024 00:21
@kiashok
Copy link
Contributor Author

kiashok commented Mar 12, 2024

@kevpar could you please take a look when you have some time? Thanks!

@kiashok
Copy link
Contributor Author

kiashok commented Mar 12, 2024

/retest-required

@kiashok
Copy link
Contributor Author

kiashok commented Mar 12, 2024

The pull-containerd-k8s-e2e-ec2 test failures are not related to this change. See conversation here https://cloud-native.slack.com/archives/CGEQHPYF4/p1710279842691129. This is due to a misconfiguration between apiserver and e2e.test version.

@kiashok
Copy link
Contributor Author

kiashok commented Mar 21, 2024 •
edited
Loading

@mikebrow @fuweid could you please take a look? :) I have two sign offs from MS folks. If it looks good, I think it could be merged. Thanks!

@kiashok
Copy link
Contributor Author

kiashok commented Mar 22, 2024 •
edited
Loading

cc @MikeZappa87 @cpuguy83 after some additional discussion and testing, enabled the new changes for host process containers as well. Could you please take a look again? Thanks!
This change now works for all types of windows containers - process isolated, hyperv and HPCs. The wincat.exe method actually only worked for process isolated and was a limitation as other folks using hyperV and HPCs might also be interested in port forwarding. cc @sbangari

@kiashok kiashok removed the request for review from kevpar March 26, 2024 04:04
fuweid
fuweid approved these changes Mar 26, 2024
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@fuweid fuweid added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
mikebrow
mikebrow approved these changes Mar 26, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow mikebrow added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@kiashok
Change port forwarding on windows
b97ef91 
Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@kiashok
Copy link
Contributor Author

kiashok commented Mar 26, 2024

image

This test is being flaky (unrelated to anything on this PR) on the merge queue and causes the merge queue to remove this PR from the queue. It passes on retries.

Updated/rebased the changes on this PR and pushed new changes. Nothing has changed wrt to the changes in this PR otherwise.

@mikebrow mikebrow added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@mikebrow mikebrow added this pull request to the merge queue Mar 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@kiashok
Copy link
Contributor Author

kiashok commented Mar 26, 2024

image
this is an extremely flaky test - this is causing the PR to be thrown out from merge queue repeatedly. Is there something that can be done to force merge or remove this test from required tests until its more reliable? @mikebrow @dmcgowan ?

@fuweid fuweid added this pull request to the merge queue Mar 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 27, 2024
@fuweid
Copy link
Member

fuweid commented Mar 27, 2024

=== FAIL: core/metadata TestImagesCreateUpdateDelete/ReplaceLabels (0.01s)
images_test.go:581: timestamp for updatedat not after createdat: 2024-03-27 02:28:49.9142049 +0000 UTC <= 2024-03-27 02:28:49.9142049 +0000 UTC

@fuweid fuweid added this pull request to the merge queue Mar 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 27, 2024
@fuweid fuweid added this pull request to the merge queue Mar 27, 2024
Merged via the queue into containerd:main with commit 61ec2e9 Mar 27, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcantah dcantah dcantah left review comments

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@mikebrow mikebrow mikebrow approved these changes

@fuweid fuweid fuweid approved these changes

@MikeZappa87 MikeZappa87 MikeZappa87 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kiashok @dcantah @fuweid @cpuguy83 @mikebrow @MikeZappa87 @k8s-ci-robot