Skip to content

Initial GRPC shims support #8052

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
Merged
dmcgowan merged 7 commits into from
Feb 15, 2023
Merged

Initial GRPC shims support #8052

dmcgowan merged 7 commits into from
Feb 15, 2023

Conversation

@mxpv
Copy link
Member

@mxpv mxpv commented Feb 5, 2023 •
edited
Loading

  • Adds a unified (TTRPC + GRPC) clients for TaskService and SandboxService.
  • Shim client support
  • Introduce shim bootstrap params (a JSON payload returned in stdout from Shim.Start to specify protocol, shim caps, etc).

@k8s-ci-robot
Copy link

k8s-ci-robot commented Feb 5, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mxpv mxpv force-pushed the grpc_shim branch 2 times, most recently from b4810da to 197aa93 Compare February 6, 2023 02:43
@mxpv mxpv marked this pull request as ready for review February 6, 2023 14:52
@mxpv
Copy link
Member Author

mxpv commented Feb 6, 2023

/test pull-containerd-sandboxed-node-e2e

@mxpv mxpv changed the title GRPC shims support Initial GRPC shims support Feb 6, 2023
kzys
kzys reviewed Feb 6, 2023
View reviewed changes
@mxpv mxpv force-pushed the grpc_shim branch 2 times, most recently from c779519 to 1863842 Compare February 6, 2023 20:12
dcantah
dcantah reviewed Feb 6, 2023
View reviewed changes
@containerd containerd deleted a comment from k8s-ci-robot Feb 6, 2023
dcantah
dcantah reviewed Feb 6, 2023
View reviewed changes
dcantah
dcantah approved these changes Feb 6, 2023
View reviewed changes
Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

Approach makes sense, LGTM

dcantah
dcantah reviewed Feb 7, 2023
View reviewed changes
runtime/v2/bridge.go
Comment on lines +59 to +62
resp, err := g.client.State(ctx, &v3.StateRequest{
ID: request.GetID(),
ExecID: request.GetExecID(),
})
Copy link
Member

@dcantah dcantah Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Minor nit after looking again, but for all of these that don't return an emptypb we should probably just return nil for the actual responses if we got an error from the original call (g.client.State in this example) to be idiomatic

Copy link
Member Author

@mxpv mxpv Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Once we release 1.7 and switch to v3, we can clean this code (as there will be no need to convert between v2 and v3 contracts), so it'll be a lot slimmer (like sandbox/bridge.go)

Copy link
Member

@dcantah dcantah Feb 15, 2023

Choose a reason for hiding this comment

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

Fine with me then

@dcantah
Copy link
Member

dcantah commented Feb 7, 2023 •
edited
Loading

There any plans to make registering the event service as GRPC a reality? Poorly worded, but for the go library I mean (instead of the manager registering as ttrpc always and writing the address to an env var)

containerd/runtime/v2/shim/shim.go

Lines 279 to 284 in f82ae8a

ttrpcAddress := os.Getenv(ttrpcAddressEnv)
publisher, err := NewPublisher(ttrpcAddress)
if err != nil {
return err
}
defer publisher.Close()
I suppose it might not even matter as this work is mainly for non-go/non-rust runtimes

@dcantah
Copy link
Member

dcantah commented Feb 10, 2023

Just trialed this, we'll need at least grpc.WithTransportCredentials(insecure.NewCredentials()) on the dialopts for the connection otherwise you get the famed: grpc: no transport security set (use grpc.WithTransportCredentials(insecure.NewCredentials()) explicitly or set credentials)

mxpv added 7 commits February 10, 2023 21:53
@mxpv
Add Task Service GRPC bridge
47cb5f6 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Add Sandbox service GRPC bridge
4b1ebef 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Wire up client bridges
9e5c207 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Initial GRPC client support
fc2e761 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Add shim bootstrap params
a82e37a 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Use switch when creating TTRPC/GRPC client
77fc094 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Add transport credentials GRPC opt
8ef298d 
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv
Copy link
Member Author

mxpv commented Feb 11, 2023

Rebased against main and fixed merge conflicts.

@mxpv mxpv added this to the 1.7 milestone Feb 11, 2023
dcantah
dcantah approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM again, dummy shim I have makes it to TaskShutdownRequest just fine if that's anything to go by 🤷

@dmcgowan dmcgowan merged commit 4b80a2b into containerd:main Feb 15, 2023
@dmcgowan dmcgowan mentioned this pull request Feb 15, 2023
Merged
@mxpv mxpv deleted the grpc_shim branch February 16, 2023 19:45
@dcantah dcantah mentioned this pull request Feb 17, 2023
Merged
This was referenced Feb 25, 2023
Merged
Merged
This was referenced Mar 7, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcantah dcantah dcantah approved these changes

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@crosbymichael crosbymichael Awaiting requested review from crosbymichael

Assignees

No one assigned

Labels

impact/changelog kind/enhancement

Projects

Pull Request Review
Archived in project

Milestone

1.7

Development

Successfully merging this pull request may close these issues.

5 participants

@mxpv @k8s-ci-robot @dcantah @kzys @dmcgowan