Skip to content

Conversation

lengrongfu
Copy link
Contributor

This pr is Sansbox API work in #7312. implemented an optional PortForward interface

@k8s-ci-robot
Copy link

Hi @lengrongfu. Thanks for your PR.

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

@lengrongfu lengrongfu force-pushed the feat/sandbox_portforward branch 2 times, most recently from 5aacf02 to 9b4a8d2 Compare October 12, 2022 06:30
@mxpv
Copy link
Member

mxpv commented Oct 12, 2022

/ok-to-test

@mxpv
Copy link
Member

mxpv commented Oct 12, 2022

/test pull-containerd-sandboxed-node-e2e

@mxpv mxpv added this to the 1.7 milestone Oct 12, 2022
@mxpv mxpv mentioned this pull request Oct 12, 2022
17 tasks

package podsandbox

import (
Copy link
Member

Choose a reason for hiding this comment

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

I don't have window platform to test this.
And I am not sure that ENABLE_CRI_SANDBOXES is working.

Hopefully, we can make sure that Test is working before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

@@ -41,3 +42,8 @@ type Controller interface {
// Delete deletes and cleans all tasks and sandbox instance.
Delete(ctx context.Context, sandboxID string) (*sandbox.ControllerDeleteResponse, error)
}

// PortForward is an interface to port forward sandboxes at runtime
type PortForward interface {
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to have this interface exposed under the sandbox package? New functionality should be defined where they are used or in a package that owns the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't need, use podsandbox.Controller imple PortForward method.

@@ -158,7 +160,10 @@ func (s *streamRuntime) PortForward(podSandboxID string, port int32, stream io.R
return fmt.Errorf("invalid port %d", port)
}
ctx := ctrdutil.NamespacedContext()
return s.c.portForward(ctx, podSandboxID, port, stream)
if pf, ok := s.c.sandboxController.(sandbox.PortForward); ok {
Copy link
Member

@mxpv mxpv Nov 30, 2022

Choose a reason for hiding this comment

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

I think this needs to be updated.
We use c.getSandboxController to obtain controller interface.
Also shim controllers go throught containerd backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

@dmcgowan dmcgowan added status/needs-discussion Needs discussion and decision from maintainers and removed status/needs-discussion Needs discussion and decision from maintainers labels Nov 30, 2022
@dmcgowan dmcgowan modified the milestones: 1.7, 2.0 Feb 10, 2023
@lengrongfu lengrongfu force-pushed the feat/sandbox_portforward branch 2 times, most recently from 46fce43 to 6eaf711 Compare May 29, 2023 15:34
Signed-off-by: rongfu.leng <rongfu.leng@daocloud.io>
@k8s-ci-robot
Copy link

PR needs rebase.

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.

@lengrongfu lengrongfu closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants