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

Merged
merged 1 commit into from
Nov 19, 2024
Merged

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 mxpv requested a review from Burning1020 September 23, 2024 17:04
@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
}

@jokemanfire jokemanfire force-pushed the dev4 branch 2 times, most recently from 05be826 to 41fafc9 Compare November 15, 2024 04:19
Compatible with goshim

Signed-off-by: jokemanfire <[email protected]>
@mxpv mxpv added this pull request to the merge queue Nov 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2024
@mxpv mxpv added this pull request to the merge queue Nov 19, 2024
Merged via the queue into containerd:main with commit 6d496ef Nov 19, 2024
18 checks passed
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