Refactor ctr restore to allow for tty allocation#7673
Refactor ctr restore to allow for tty allocation#7673AkihiroSuda merged 1 commit intocontainerd:mainfrom
Conversation
|
Hi @turan18. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Your PR description is great. Please copy some to the commit message itself. |
| } | ||
|
|
||
| // Check if the Spec requires a terminal to be allocated -> { terminal: true or false } | ||
| func CheckSpecTTY(ctx context.Context, client *Client, desc ocispec.Descriptor) (*bool, error) { |
There was a problem hiding this comment.
While the intention of your change as a whole was to use this to check if a restored container needs a tty, this file is solely for Restore related opts functions as of now (WithFooBar() NewContainerOpts). Although maybe there's another way to get the info you're after without this altogether
| } | ||
|
|
||
| task, err := ctr.NewTask(ctx, cio.NewCreator(cio.WithStdio), topts...) | ||
| useTTY, err := containerd.CheckSpecTTY(ctx, client, checkpoint.Target()) |
There was a problem hiding this comment.
Never used the criu support here, but does the container object returned from client.Restore() have its spec filled in as normal? If so could we just check the spec returned from ctr.Spec() and see if terminal is set?
There was a problem hiding this comment.
Ugh github didn't select all of the lines I did -_-. I had lines 83-93 selected if that makes the comment easier to parse.
There was a problem hiding this comment.
You are correct. I have removed the unnecessary call to checkSpecTTY().
Got it. Done |
…al:true} within Spec Signed-off-by: Yasin Turan <[email protected]>
6 participants
ctr container restorerestores a container from a specified checkpoint. The newly restored container will have the same Spec as that of the parent container(container which was checkpointed ). If the parent container was started with a new terminal allocated (eg:ctr run -t) its Spec will have{terminal : true}, letting runc know to create a new set of file descriptors for a new pseudoterminal.The IO for the new container that will be restored is specified via the cio.NewCreator() function which takes in cio.WithStdio as an argument, effectively setting the IO of the new container to be the default OS stdio. The issue happens when attempting to restore a container whom Spec has
{terminal: true}. Since no terminal has been allocated to the container (restore only uses os stdio), runc does not have a place to send the file descriptors it creates for the pseudoterminal, thowing the error =>cannot allocate tty if runc will detach without setting console socket: unknownI have refactored
ctr container restoreto account for tty allocation in the case where the Spec requires a terminal to be set. A lot of the changes mirror what has already been done withinctr runFixes: #7672
Signed-off-by: Yasin Turan [email protected]