[Refactor] Refactor the (container) create command flagging process #1970

Laitr0n · 2023-02-03T02:58:38Z

Checklist:

Create a file in pkg/api/types/container_types.go, and define the CommandOption for this command

Signed-off-by: Laitron meetlq@outlook.com

cmd/nerdctl/container_create.go

pkg/api/types/container_types.go

cmd/nerdctl/container_create.go

cmd/nerdctl/container_run_linux.go

Laitr0n · 2023-04-03T04:54:11Z

@djdongjin Sorry for delay.
PTAL. If there are any problems, please let me know. I will make the changes as soon as possible.

cmd/nerdctl/container_create.go

pkg/containerutil/containerutil.go

pkg/cmd/container/run.go

pkg/containerutil/mount_options.go

pkg/containerutil/labels.go

djdongjin · 2023-04-03T15:46:30Z

pkg/containerutil/gc.go

+)
+
+// GenerateGcFunc returns a function that can be used to garbage collect a container
+func GenerateGcFunc(ctx context.Context, container containerd.Container, ns, id, name, dataStore string, containerErr error, containerNameStore namestore.NameStore, netManager NetworkOptionsManager, internalLabels InternalLabels) func() {


where is this func used?

When an error occurs during creation client.NewContainer(ctx, id, cOpts...) or due to a network error netManager.SetupNetworking(ctx, id), GenerateGcFunc uses to generate gc func.

djdongjin · 2023-04-03T15:53:15Z

I haven't looked into if the newly created files make sense (i.e. if they're created in the correct pkg/location etc). Will take a look later.

Laitr0n · 2023-04-03T16:05:40Z

I haven't looked into if the newly created files make sense (i.e. if they're created in the correct pkg/location etc). Will take a look later.

Thanks! I got it and I will deal above comments firstly.

djdongjin

@Laitr0n thanks for the change.

An overall suggestion is to first make the file name changes more consistent with the existing pattern, instead of creating new packages (cgrouputil) or moving to other packages (containerutil) unless the code is reused by other functions/commands other than run.

E.g., move cmd/nerdctl/container_run_xxx_linux.go to pkg/cmd/container/run_xxx_linux.go.

This will be quite helpful to identify where a new file/code come from.

Thanks!

pkg/containerutil/restart_options.go

pkg/containerutil/platform_options_linux.go

pkg/cgrouputil/cgroup_linux.go

Laitr0n · 2023-04-25T15:44:26Z

@Laitr0n thanks for the change.

An overall suggestion is to first make the file name changes more consistent with the existing pattern, instead of creating new packages (cgrouputil) or moving to other packages (containerutil) unless the code is reused by other functions/commands other than run.

E.g., move cmd/nerdctl/container_run_xxx_linux.go to pkg/cmd/container/run_xxx_linux.go.

This will be quite helpful to identify where a new file/code come from.

Thanks!

Got what you mean. I'll make the update for these changes.

AkihiroSuda · 2023-05-03T16:15:16Z

@Zheaoli @djdongjin LGTY? 🙏

djdongjin · 2023-05-03T16:23:18Z

@AkihiroSuda sorry I haven't gotten a chance to take a look at the latest change. Will take a look today.

djdongjin

Overall LGTM, I left some minor suggestions. Thanks!

Since it's a large PR it might be good to have another review/approval.

pkg/cmd/container/run_runtime.go

djdongjin · 2023-05-04T12:32:22Z

pkg/cmd/container/run_cgroup_linux.go

-	memSwap, err := cmd.Flags().GetString("memory-swap")
-	if err != nil {
-		return nil, err
+func generateCgroupPath(id string, cgroupManager, cgroupParent string) (string, error) {


nit: can we keep the argument order unchanged and remove the redundant string arg type (cgroupManager, cgroupParent, id string). Also if you move this func to after GenerateOptsand beforeParseDevice`, the diff will be much smaller.

Laitr0n · 2023-05-04T12:53:57Z

Overall LGTM, I left some minor suggestions. Thanks!

Since it's a large PR it might be good to have another review/approval.

LGTM! Thanks for your work! ❤️

@Zheaoli Could you take a look at this?

Signed-off-by: Laitron <meetlq@outlook.com>

AkihiroSuda

Thanks

Laitr0n mentioned this pull request Feb 3, 2023

Refactor the (container) run command flagging process #1954

Open

Laitr0n marked this pull request as ready for review February 3, 2023 03:00

Laitr0n marked this pull request as draft February 3, 2023 10:05

Laitr0n force-pushed the refactor_container_create branch 11 times, most recently from fa7578e to cde45a3 Compare February 4, 2023 16:39

djdongjin reviewed Feb 4, 2023

View reviewed changes

cmd/nerdctl/container_create.go Outdated Show resolved Hide resolved

Laitr0n force-pushed the refactor_container_create branch 2 times, most recently from 1cdf3bd to a2c6d43 Compare February 6, 2023 03:53

djdongjin reviewed Feb 6, 2023

View reviewed changes

cmd/nerdctl/container_create.go Outdated Show resolved Hide resolved

Laitr0n force-pushed the refactor_container_create branch 8 times, most recently from e14ae66 to dd82e4b Compare February 12, 2023 08:07

Laitr0n force-pushed the refactor_container_create branch 2 times, most recently from fc68d9d to 73f0bc3 Compare February 14, 2023 16:23

suyanhanx reviewed Feb 17, 2023

View reviewed changes

pkg/api/types/container_types.go Outdated Show resolved Hide resolved

cmd/nerdctl/container_create.go Outdated Show resolved Hide resolved

cmd/nerdctl/container_run_linux.go Outdated Show resolved Hide resolved

Laitr0n force-pushed the refactor_container_create branch from 4352fbf to 327fe3f Compare February 18, 2023 04:29

Laitr0n changed the title ~~[WIP][Refactor] Refactor the (container) create command flagging process~~ [Refactor] Refactor the (container) create command flagging process Mar 31, 2023

Laitr0n requested a review from djdongjin March 31, 2023 12:55

djdongjin reviewed Apr 3, 2023

View reviewed changes

Laitr0n force-pushed the refactor_container_create branch from 058c49e to 27ebb3a Compare April 4, 2023 13:36

djdongjin requested a review from Zheaoli April 4, 2023 13:41

Laitr0n force-pushed the refactor_container_create branch from 27ebb3a to 691e367 Compare April 6, 2023 02:04

AkihiroSuda modified the milestones: v1.3.1, v1.4.0 (tentative) Apr 13, 2023

Laitr0n force-pushed the refactor_container_create branch from 92d9896 to ee342bb Compare April 21, 2023 13:32

AkihiroSuda added the kind/refactor label Apr 25, 2023

AkihiroSuda requested a review from djdongjin April 25, 2023 13:14

djdongjin reviewed Apr 25, 2023

View reviewed changes

pkg/containerutil/restart_options.go Outdated Show resolved Hide resolved

pkg/containerutil/platform_options_linux.go Outdated Show resolved Hide resolved

pkg/cgrouputil/cgroup_linux.go Outdated Show resolved Hide resolved

Laitr0n force-pushed the refactor_container_create branch 4 times, most recently from a44450c to 09cc240 Compare April 30, 2023 06:56

Laitr0n requested a review from djdongjin April 30, 2023 07:29

djdongjin approved these changes May 4, 2023

View reviewed changes

[Refactor] Refactor the run command in createContainer.

e7855d0

Signed-off-by: Laitron <meetlq@outlook.com>

Laitr0n force-pushed the refactor_container_create branch from 09cc240 to e7855d0 Compare May 4, 2023 13:50

AkihiroSuda approved these changes May 7, 2023

View reviewed changes

AkihiroSuda merged commit a28e41d into containerd:main May 7, 2023

Laitr0n deleted the refactor_container_create branch May 7, 2023 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Refactor the (container) create command flagging process #1970

[Refactor] Refactor the (container) create command flagging process #1970

Laitr0n commented Feb 3, 2023 •

edited

Loading

Laitr0n commented Apr 3, 2023

djdongjin Apr 3, 2023

Laitr0n Apr 4, 2023

djdongjin commented Apr 3, 2023

Laitr0n commented Apr 3, 2023

djdongjin left a comment

Laitr0n commented Apr 25, 2023

AkihiroSuda commented May 3, 2023

djdongjin commented May 3, 2023

djdongjin left a comment

djdongjin May 4, 2023

Laitr0n commented May 4, 2023

AkihiroSuda left a comment

[Refactor] Refactor the (container) create command flagging process #1970

[Refactor] Refactor the (container) create command flagging process #1970

Conversation

Laitr0n commented Feb 3, 2023 • edited Loading

Laitr0n commented Apr 3, 2023

djdongjin Apr 3, 2023

Choose a reason for hiding this comment

Laitr0n Apr 4, 2023

Choose a reason for hiding this comment

djdongjin commented Apr 3, 2023

Laitr0n commented Apr 3, 2023

djdongjin left a comment

Choose a reason for hiding this comment

Laitr0n commented Apr 25, 2023

AkihiroSuda commented May 3, 2023

djdongjin commented May 3, 2023

djdongjin left a comment

Choose a reason for hiding this comment

djdongjin May 4, 2023

Choose a reason for hiding this comment

Laitr0n commented May 4, 2023

AkihiroSuda left a comment

Choose a reason for hiding this comment

Laitr0n commented Feb 3, 2023 •

edited

Loading