Deprecate shim.Command#13319
Conversation
There was a problem hiding this comment.
Pull request overview
Deprecates the exported pkg/shim.Command API via GoDoc deprecation text to discourage external consumption of daemon-only shim launch internals.
Changes:
- Updates
shim.Commanddoc comment punctuation/wording. - Adds a
Deprecated:paragraph explaining intended scope (containerd daemon internals) and future relocation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the pkg/shim.Command Go doc comment to be recognized as deprecated, clarifying that it’s intended for internal containerd daemon shim lifecycle launches rather than external consumption.
Changes:
- Mark
pkg/shim.Commandas deprecated using the Go docDeprecated:convention and expand rationale in the comment. - Add
staticchecksuppression at internal call sites (runtime v2 shim start/delete) to avoid SA1019 warnings while retaining required internal usage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/shim/util.go | Adds a Go-recognized Deprecated: doc paragraph and clarifies intended/internal usage and future relocation. |
| core/runtime/v2/binary.go | Suppresses staticcheck SA1019 at internal client.Command call sites with context explaining why internal usage remains valid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // by the containerd daemon for the initial shim launches (e.g. shim -start) and makes | ||
| // no sense for shim implementations. | ||
| // Deprecated: this function is internal to the containerd daemon, which uses it to | ||
| // invoke the shim binary for "start" and "delete" actions during the shim lifecycle. |
There was a problem hiding this comment.
Since the shim is tied to the sandbox, we are supposed to run shim delete when the sandbox is terminating. However, if the shim is killed unexpectedly, how should we clean up the sidecar container?
There was a problem hiding this comment.
The shim delete is only used to cleanup that sandbox one. it doesn't include that sidecar containers.
It relies on shim is alive to do the cleanup. I think that original design for deletion is to make sure that resources can be cleanup. But right now, it's hard to guarantee.
There was a problem hiding this comment.
I guess I don't fully understand the question. I'm not proposing to delete this function. I'm proposing to remove it from public package and move it to containerd core because it's daemon-internal. pkg/shim meant to be used by external shim authors.
There was a problem hiding this comment.
Sorry, I misunderstand that comment. I will post issue about shim-delete one.
Deprecate
Commandfrompkg/shim. This function is used by containerd to launch shims and should not be exposed to external users. This PR updates the comment so Go can recognize it as deprecated. I think it worth doing this in 2.3