Skip to content
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.

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

Process not found should not return ok #313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jokemanfire
Copy link
Contributor

if the process ID is not found, I think an error should be returned.

@github-actions github-actions bot added the C-runc-shim Runc shim label Sep 12, 2024
@mxpv
Copy link
Member

mxpv commented Oct 30, 2024

ping @Burning1020

@jsturtevant
Copy link
Contributor

if the process ID is not found, I think an error should be returned.

Could you expand on why? This is during a delete of the container. If there is no process then it should be a noop.

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Oct 31, 2024

Hello @jsturtevant I want to align it with Goshim . Return ok will ignore the Error::NotFoundError.
go shim code like this
https://github.com/containerd/containerd/blob/main/cmd/containerd-shim-runc-v2/runc/container.go

// Process returns the process by id
func (c *Container) Process(id string) (process.Process, error) {
	c.mu.Lock()
	defer c.mu.Unlock()
	if id == "" {
		if c.process == nil {
			return nil, fmt.Errorf("container must be created: %w", errdefs.ErrFailedPrecondition)
		}
		return c.process, nil
	}
	p, ok := c.processes[id]
	if !ok {
		return nil, fmt.Errorf("process does not exist %s: %w", id, errdefs.ErrNotFound)
	}
	return p, nil
}

Maintain consistency with Goshim

Signed-off-by: jokemanfire <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-runc-shim Runc shim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants