Skip to content

oci: fix WithDevShmSize#5063

Merged
fuweid merged 1 commit intocontainerd:masterfrom
Iceber:fix-with-dev-shm-size
May 6, 2021
Merged

oci: fix WithDevShmSize#5063
fuweid merged 1 commit intocontainerd:masterfrom
Iceber:fix-with-dev-shm-size

Conversation

@Iceber
Copy link
Copy Markdown
Member

@Iceber Iceber commented Feb 23, 2021 •
edited
Loading

I think that as an exported function, it should be robust enough.

  • WithDevShmSize should only set the size of the /dev/shm
  • size options should be de-duplicated to prevent overwriting of the latter options
  • m is not a pointer，fix m.Options = append(m.Options, fmt.Sprintf("size=%dk", kb))

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Feb 23, 2021

Hi @Iceber. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 23, 2021

Build succeeded.

fuweid
fuweid reviewed Feb 23, 2021
View reviewed changes
Comment thread oci/spec_opts.go Outdated
Comment thread oci/spec_opts.go Outdated
Comment thread oci/spec_opts.go Outdated
@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from d20b88c to 693827a Compare February 23, 2021 08:40
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 23, 2021

Build succeeded.

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from 693827a to 8e8d26b Compare February 23, 2021 08:51
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 23, 2021

I added the unit test.
@fuweid PTAL. Thanks

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 23, 2021

Build succeeded.

@Iceber Iceber closed this Feb 24, 2021
@Iceber Iceber reopened this Feb 24, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 24, 2021

Build succeeded.

@fuweid fuweid self-requested a review February 24, 2021 05:42
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Feb 25, 2021

@crosbymichael PTAL. Thanks

fuweid
fuweid reviewed Mar 1, 2021
View reviewed changes
Comment thread oci/spec_opts_test.go Outdated
@Iceber Iceber requested a review from fuweid March 1, 2021 13:58
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 2, 2021

Build succeeded.

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from d6f8687 to 086e4f6 Compare March 2, 2021 10:46
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 2, 2021

@fuweid updated. Thank you for reviewing

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 2, 2021

Build succeeded.

fuweid
fuweid approved these changes Mar 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 9, 2021

@fuweid Hmm... Cloud the pr be merged?

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Mar 9, 2021

@fuweid Hmm... Cloud the pr be merged?

We need two LGTM~

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 9, 2021

@AkihiroSuda @mikebrow PTAL. Thanks

AkihiroSuda
AkihiroSuda reviewed Mar 9, 2021
View reviewed changes
Comment thread oci/spec_opts.go Outdated
@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from 086e4f6 to 43051bb Compare March 9, 2021 05:05
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 9, 2021

Build succeeded.

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from 43051bb to ad41240 Compare March 9, 2021 05:27
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 9, 2021

Build succeeded.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 9, 2021

move WithDevShmSize to spec_opts_unix.go ?

Yes, please

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from ad41240 to f023e3e Compare March 9, 2021 06:01
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 9, 2021

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch 3 times, most recently from df903ce to 4a7fa9d Compare March 9, 2021 06:16
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 9, 2021

Build succeeded.

@Iceber Iceber closed this Mar 9, 2021
@Iceber Iceber reopened this Mar 9, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 9, 2021

Build succeeded.

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 9, 2021

The WithDevShmSize has been moved to spec_opts_unix.go and spec_opts_linux.go. The TestWithDevShmSize has been moved to spec_opts_unix_test.go

The pr should not be related to the failed case

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from 4a7fa9d to e455b32 Compare March 10, 2021 02:25
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 10, 2021

Build succeeded.

@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from e455b32 to 5199813 Compare March 10, 2021 02:42
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 10, 2021 •
edited
Loading

The WithDevShmSize has been moved to spec_opts_unix.go and spec_opts_linux.go. The TestWithDevShmSize has been moved to spec_opts_unix_test.go

The pr should not be related to the failed case

I thought about it and it makes sense for Windows to return ErrNoShmMount, so I'm not going to move theWithDevShmSize and add the test case without the /dev/shm
@AkihiroSuda What do you think?

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 10, 2021

Build succeeded.

@Iceber Iceber requested a review from AkihiroSuda March 17, 2021 05:33
@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from 5199813 to a84da1c Compare March 17, 2021 05:40
@Iceber
oci: fix WithDevShmSize
b592a4c 
Signed-off-by: Iceber Gu <[email protected]>
@Iceber Iceber force-pushed the fix-with-dev-shm-size branch from a84da1c to b592a4c Compare March 17, 2021 05:44
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 17, 2021

Build succeeded.

@kzys
Copy link
Copy Markdown
Member

kzys commented Mar 17, 2021

/ok-to-test

@AkihiroSuda PTAL

@samuelkarp
Copy link
Copy Markdown
Member

samuelkarp commented Mar 20, 2021

/test pull-containerd-node-e2e

@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented May 6, 2021

@AkihiroSuda Does it still need to be modified？PTAL, Thanks

@AkihiroSuda AkihiroSuda requested a review from fuweid May 6, 2021 07:03
@fuweid fuweid merged commit ab963e1 into containerd:master May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Iceber @k8s-ci-robot @fuweid @AkihiroSuda @kzys @samuelkarp