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.

Sign up for GitHub

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

Jump to bottom

Support --ipc=(shareable|container:<container>) flag #2757

Merged
merged 1 commit into from
Feb 4, 2024
Merged

Support --ipc=(shareable|container:<container>) flag #2757

merged 1 commit into from
Feb 4, 2024

Conversation

minuk-dev
Copy link
Contributor

#1293

(Late reviews are also okay. Please review carefully.)

  • Add ipcutil:
    • To support --ipc=shareable, a container mounts a path on host as an intermediary for /dev/shm.
    • For its persistency, the ipc info is saved in the containers label.
    • For backward compatibility, if ipcLabel is empty, ipc mode will be private.
  • When to remove container and stoping container, call ipcutil.CleanUp() to clean mounted /dev/shm.

minuk-dev
minuk-dev commented Jan 16, 2024
View reviewed changes
cmd/nerdctl/container_run_linux_test.go Outdated
Comment on lines 102 to 103
if !strings.Contains(combined, "no such container: abcd1234") &&
!strings.Contains(combined, "No such container: abcd1234") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q. For docker compatible, I added "No such container: abcd1234". But, in many cases, nerdctl prints it as lowercase. How can I deal with it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just use case-insensitive regexp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just change the string to lowercases before comparing it.
Thanks to reply.

AkihiroSuda
AkihiroSuda reviewed Jan 17, 2024
View reviewed changes
pkg/ipcutil/ipcutil.go Outdated

// ShmSize is only used when mode is private or shareable
// Devshm size in bytes
ShmSize string `json:"shm_size,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this IPC marshalized as JSON?
We do not use this_json_style in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it. I'm sorry.

@AkihiroSuda AkihiroSuda requested review from a team January 25, 2024 03:53
djdongjin
djdongjin reviewed Jan 25, 2024
View reviewed changes
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work. Left a few comments

pkg/ipcutil/ipcutil_windows.go Outdated Show resolved Hide resolved
pkg/ipcutil/ipcutil_windows.go Outdated Show resolved Hide resolved
pkg/ipcutil/ipcutil_linux.go Outdated Show resolved Hide resolved
pkg/ipcutil/ipcutil_linux.go Outdated
}
shmproperty += ",size=" + strconv.FormatInt(shmBytes, 10)
}
err := os.MkdirAll(shmPath, 0o700)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be 0o700 or 0700?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. Should I change it?
I think 0700 or 0755 can be. How do you think about it?

pkg/cmd/container/create.go Outdated Show resolved Hide resolved
pkg/containerutil/containerutil.go Outdated Show resolved Hide resolved
pkg/ipcutil/ipcutil.go
func DetectFlags(ctx context.Context, client *containerd.Client, stateDir string, ipc string, shmSize string) (IPC, error) {
var res IPC
res.ShmSize = shmSize
switch ipc {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cleaner to have a standalone func to convert string -> IPCMode? e.g.

func stringToIPCMode(s string) (IPCMode, error) {
	switch s {
	case "private", "host", "shareable", "container":
		return IPCMode(s), nil
	default:
		return "", errors.New("invalid IPCMode string")
	}
}

Then we can have other logic based on the converted IPCMode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find the better way to solve it.
Because, before searching the containers' status, we cannot know that the ipc string value is valid.
Can I get more detailed advice, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay. I'll take another look today.

pkg/ipcutil/ipcutil.go Show resolved Hide resolved
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Jan 31, 2024
djdongjin
djdongjin approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@AkihiroSuda
Copy link
Member

Looks good, but please squash commits

yankay
yankay reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@yankay yankay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @minuk-dev

Great Work. There are some comments for reference only.

cmd/nerdctl/container_restart_linux_test.go Show resolved Hide resolved
cmd/nerdctl/container_run_linux_test.go Outdated Show resolved Hide resolved
pkg/containerutil/containerutil.go Outdated Show resolved Hide resolved
@minuk-dev
Support --ipc=container:<id> option
b452c64 
Signed-off-by: Min Uk Lee <minuk.dev@gmail.com>
- support ipc namespace
- support /dev/shm
AkihiroSuda
AkihiroSuda approved these changes Feb 4, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit 8e9f510 into containerd:main Feb 4, 2024
22 checks passed
@minuk-dev minuk-dev mentioned this pull request Feb 5, 2024
Closed
@minuk-dev minuk-dev deleted the iss1293/ipc branch April 28, 2024 04:39
@github-actions github-actions bot mentioned this pull request Nov 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yankay yankay yankay left review comments

@djdongjin djdongjin djdongjin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@minuk-dev @AkihiroSuda @yankay @djdongjin