Cleanup v2 shim #5813
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ func loadAddress(path string) (string, error) { | |
| return string(data), nil | ||
| } | ||
|
|
||
| func loadShim(ctx context.Context, bundle *Bundle, onClose func()) (_ *shim, err error) { | ||
| address, err := loadAddress(filepath.Join(bundle.Path, "address")) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -117,15 +117,15 @@ func loadShim(ctx context.Context, bundle *Bundle, events *exchange.Exchange, rt | |
| } | ||
| }() | ||
| s := &shim{ | ||
| client: client, | ||
| task: task.NewTaskClient(client), | ||
| bundle: bundle, | ||
| } | ||
| ctx, cancel := timeout.WithContext(ctx, loadTimeout) | ||
| defer cancel() | ||
|
|
||
| // Check connectivity | ||
| if _, err := s.PID(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| return s, nil | ||
|
|
@@ -186,23 +186,9 @@ func cleanupAfterDeadShim(ctx context.Context, id, ns string, rt *runtime.TaskLi | |
| var _ runtime.Task = &shim{} | ||
|
|
||
| type shim struct { | ||
| bundle *Bundle | ||
| client *ttrpc.Client | ||
| task task.TaskService | ||
| } | ||
|
|
||
| func (s *shim) Shutdown(ctx context.Context) error { | ||
|
|
@@ -227,8 +213,15 @@ func (s *shim) ID() string { | |
| } | ||
|
|
||
| // PID of the task | ||
| func (s *shim) PID(ctx context.Context) (uint32, error) { | ||
|
mxpv marked this conversation as resolved.
|
||
| response, err := s.task.Connect(ctx, &task.ConnectRequest{ | ||
| ID: s.ID(), | ||
| }) | ||
| if err != nil { | ||
| return 0, errdefs.FromGRPC(err) | ||
| } | ||
|
|
||
| return response.TaskPid, nil | ||
| } | ||
|
|
||
| func (s *shim) Namespace() string { | ||
|
|
@@ -239,7 +232,7 @@ func (s *shim) Close() error { | |
| return s.client.Close() | ||
| } | ||
|
|
||
| func (s *shim) delete(ctx context.Context, removeTask func(ctx context.Context, id string)) (*runtime.Exit, error) { | ||
| response, shimErr := s.task.Delete(ctx, &task.DeleteRequest{ | ||
| ID: s.ID(), | ||
| }) | ||
|
|
@@ -264,7 +257,7 @@ func (s *shim) Delete(ctx context.Context) (*runtime.Exit, error) { | |
| // So we should remove the record and prevent duplicate events from | ||
| // ttrpc-callback-on-close. | ||
| if shimErr == nil { | ||
| removeTask(ctx, s.ID()) | ||
| } | ||
|
|
||
| if err := s.waitShutdown(ctx); err != nil { | ||
|
|
@@ -275,7 +268,7 @@ func (s *shim) Delete(ctx context.Context) (*runtime.Exit, error) { | |
|
|
||
| // remove self from the runtime task list | ||
| // this seems dirty but it cleans up the API across runtimes, tasks, and the service | ||
| removeTask(ctx, s.ID()) | ||
| if err := s.bundle.Delete(); err != nil { | ||
| log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to delete bundle") | ||
| } | ||
|
|
@@ -311,11 +304,12 @@ func (s *shim) Create(ctx context.Context, opts runtime.CreateOpts) (runtime.Tas | |
| Options: m.Options, | ||
| }) | ||
| } | ||
|
|
||
| _, err := s.task.Create(ctx, request) | ||
| if err != nil { | ||
| return nil, errdefs.FromGRPC(err) | ||
| } | ||
|
|
||
| return s, nil | ||
| } | ||
|
|
||
|
|
@@ -338,13 +332,12 @@ func (s *shim) Resume(ctx context.Context) error { | |
| } | ||
|
|
||
| func (s *shim) Start(ctx context.Context) error { | ||
| _, err := s.task.Start(ctx, &task.StartRequest{ | ||
| ID: s.ID(), | ||
| }) | ||
| if err != nil { | ||
| return errdefs.FromGRPC(err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -359,7 +352,7 @@ func (s *shim) Kill(ctx context.Context, signal uint32, all bool) error { | |
| return nil | ||
| } | ||
|
|
||
| func (s *shim) Exec(ctx context.Context, id string, opts runtime.ExecOpts) (runtime.ExecProcess, error) { | ||
| if err := identifiers.Validate(id); err != nil { | ||
| return nil, errors.Wrapf(err, "invalid exec id %s", id) | ||
| } | ||
|
|
@@ -422,14 +415,18 @@ func (s *shim) CloseIO(ctx context.Context) error { | |
| } | ||
|
|
||
| func (s *shim) Wait(ctx context.Context) (*runtime.Exit, error) { | ||
| taskPid, err := s.PID(ctx) | ||
|
Member
There was a problem hiding this comment. why not just record the taskPid?
Member
Author
There was a problem hiding this comment. Pid is task service specific thing. The goal of this PR is to make shim stateless and unaware about services shim instance provides. This way we can add new services on top of shims independently - task service, sandbox api, port forwarding, pulling inside sandbox, etc (lots of context described in #5742). Pid will move to task service back, once we decouple shim v2 |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| response, err := s.task.Wait(ctx, &task.WaitRequest{ | ||
| ID: s.ID(), | ||
| }) | ||
| if err != nil { | ||
| return nil, errdefs.FromGRPC(err) | ||
| } | ||
| return &runtime.Exit{ | ||
| Pid: taskPid, | ||
| Timestamp: response.ExitedAt, | ||
| Status: response.ExitStatus, | ||
| }, nil | ||
|
|
@@ -468,7 +465,7 @@ func (s *shim) Stats(ctx context.Context) (*ptypes.Any, error) { | |
| return response.Stats, nil | ||
| } | ||
|
|
||
| func (s *shim) Process(ctx context.Context, id string) (runtime.ExecProcess, error) { | ||
| p := &process{ | ||
| id: id, | ||
| shim: s, | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
_ctxThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, context is not used in v1 runtime, but needs to be passed in order to implement the interface. So it's marked with
_prefix to be ignored by linters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right; I usually just use
_(no variable name) for thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look like rust style. go use
_