refactor: make spec created more reasonable #1154

Ace-Tang · 2018-04-17T15:19:59Z

Old spec related code is splited messed, lots of fields in spec
have relationship with each other, simple make them as general
funtion is wrong, I try to make spec files splited from runtime
spec struct, make it more reasonable and remove redundancy code.

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

allencloud · 2018-04-18T01:25:33Z

I like the spirit this pull request figures out. 👍

codecov-io · 2018-04-18T12:15:37Z

Codecov Report

Merging #1154 into master will increase coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   15.57%   15.62%   +0.05%     
==========================================
  Files         178      171       -7     
  Lines       10454    10420      -34     
==========================================
  Hits         1628     1628              
+ Misses       8706     8672      -34     
  Partials      120      120

Impacted Files	Coverage Δ
daemon/mgr/spec_process.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_annotations.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_linux.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_mount.go	`0% <0%> (ø)`
daemon/mgr/spec_hook.go	`0% <0%> (ø)`	⬆️

Ace-Tang · 2018-04-18T12:23:50Z

Please help to review the code, @allencloud , @rudyfly , @YaoZengzeng

rudyfly · 2018-04-18T13:22:51Z

daemon/mgr/spec.go

+func createSpec(ctx context.Context, c *ContainerMeta, specWrapper *SpecWrapper) error {
+	// new a default spec from containerd.
+	s, err := ctrd.NewDefaultSpec(ctx, c.ID)
+	if err != nil {


I don't think it is a right way to modify, it decreases the scalability of these code. When I write a part of spec, I just focus on it, don't care others. I agree to merge some similar functions.

First I want to say, fields in specs have related with each other, you can not focus on just one but not care about other.
For eg. When you set namespace, you must be careful about you are use other container ns or use host , or just make a new ns . Because in different case, lots of spec field should be changed. If you set uts ns to host, you need modify hostname, if you set user ns, you should get correct values for s.Linux.UIDMappings and s.Linux.GIDMappings.
How can you say you can just care one part. Or you can say it can be fixed by set setup functions to a good order, but I think it bad. When we read code at first time, we just think they
are general functions, these functions do not care about order since they are independent from each other. Unfortunately, they do have order, and if they execute in the wrong order, things will go wrong way.

On the other hand, spec code is splited into different parts by specs.Spec.

type Spec struct { // Version of the Open Container Runtime Specification with which the bundle complies. Version string `json:"ociVersion"` // Platform specifies the configuration's target platform. Platform Platform `json:"platform"` // Process configures the container process. Process Process `json:"process"` // Root configures the container's root filesystem. Root Root `json:"root"` // Hostname configures the container's hostname. Hostname string `json:"hostname,omitempty"` // Mounts configures additional mounts (on top of Root). Mounts []Mount `json:"mounts,omitempty"` // Hooks configures callbacks for container lifecycle events. Hooks Hooks `json:"hooks"` // Annotations contains arbitrary metadata for the container. Annotations map[string]string `json:"annotations,omitempty"` // Linux is platform specific configuration for Linux based containers. Linux *Linux `json:"linux,omitempty" platform:"linux"` // Solaris is platform specific configuration for Solaris containers. Solaris *Solaris `json:"solaris,omitempty" platform:"solaris"` // Windows is platform specific configuration for Windows based containers, including Hyper-V containers. Windows *Windows `json:"windows,omitempty" platform:"windows"` }

If you want to change one part, you can modify the corresponding files, but you still need to care about that if the modification will influence other fields. As I said before, spec fields are not Independent from each other. That why I say the design before is not reasonable.
Maybe I am not express my meaning totally, we can continue to discuss, @rudyfly

To be honest, I am not on the side of the original way. This refactoring is my dish. For me, I cannot accept to couple specWrapper with several managers with spec. It breaks the simplicity of code. Although it seems to be quite scalable, but redundant so much.

We can achieve scalability by several ways. If ideal scalability is not included in this part, we can improve this.

Also, could we still add type SetupFunc func(ctx context.Context, m *ContainerMeta, s *SpecWrapper) error ? and aggregate all the original functions into several SetupFunc according the LinuxSpec's fields in runtime-spec. @rudyfly @Ace-Tang

allencloud · 2018-04-22T06:35:37Z

daemon/mgr/spec_linux.go

+		}
+	} else {
+		for _, deviceMapping := range c.HostConfig.Devices {
+			if !opts.ValidateDeviceMode(deviceMapping.CgroupPermissions) {


I am wondering if we have to do the validation checking here. Is there any possibility to put this ahead to validations in manager layer instead of spec layer.

Of course, we can consider this later.

allencloud · 2018-04-22T07:12:54Z

daemon/mgr/spec_mount.go

@@ -73,8 +72,6 @@ func setupMounts(ctx context.Context, c *ContainerMeta, spec *SpecWrapper) error
 				}
 			}
 		}
-		s.Linux.ReadonlyPaths = nil


I am wondering why to remove both two lines of code. Just curious. :)

I move them to Linux, since they are in spec.Linux field. And only depend on if container has privileged.

allencloud · 2018-04-22T07:19:03Z

daemon/mgr/spec_process.go

+	s.Process.Env = append(s.Process.Env, createEnvironment(c)...)
+	s.Process.Cwd = cwd
+	s.Process.Terminal = config.Tty
+	s.Process.NoNewPrivileges = c.NoNewPrivileges


I think this part is not compatible with the original code. Please help to take a look again. @Ace-Tang .
Here is my reason, in the original way, we have:

func setupNoNewPrivileges(ctx context.Context, meta *ContainerMeta, spec *SpecWrapper) error { if meta.HostConfig.Privileged { return nil } spec.s.Process.NoNewPrivileges = meta.NoNewPrivileges return nil }

So, if meta.HostConfig.Privileged is false, it will not be assigned value even if user has set one value for this.

While now, no matter privileged is true or not, s.Process.NoNewPrivileges will always be assigned.

WDYT?

I check the NoNewPrivileges not depend on privileged. Maybe we need to find the one who write it to disscuse.

cc @YaoZengzeng

allencloud · 2018-04-22T07:36:42Z

I try to understand the refactoring, while I think we usually feel uneasy to code review just in one way. Let me add some point I may have:

Could we add the SetupFunc like the original way, but aggregate serveral functions according to field's in LinuxSpec.
I have not found the namespace dealing code within your code change, is it not necessary.

Hope to hear from you soon. Thanks.

Ace-Tang · 2018-04-25T05:57:00Z

@allencloud , I update the pr, PTAL, and about namespace dealing code , locate at daemon/mgr/spec_linux.go , the file is compressed.

Old spec related code is splited messed, lots of fields in spec have relationship with each other, simple make them as general funtion is wrong, I try to make spec files splited from runtime spec struct, make it more reasonable and remove redundancy code. Signed-off-by: Ace-Tang <[email protected]>

rudyfly · 2018-04-25T02:09:09Z

daemon/mgr/spec_hook.go

-		Args: args[1:],
-	}
+	// if rootFSQuota not set skip quota prestart set
+	rootFSQuota := quota.GetDefaultQuota(c.Config.DiskQuota)


change these into a function?

allencloud · 2018-04-25T09:41:06Z

LGTM

pouchrobot added kind/refactor size/XXL labels Apr 17, 2018

rudyfly reviewed Apr 18, 2018

View reviewed changes

allencloud reviewed Apr 22, 2018

View reviewed changes

This was referenced Apr 22, 2018

feature: set volumes to /etc/mtab #1170

Merged

feature: add run flag ulimit #1179

Merged

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Apr 24, 2018

rudyfly reviewed Apr 25, 2018

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 25, 2018

allencloud merged commit 30dd372 into AliyunContainerService:master Apr 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make spec created more reasonable #1154

refactor: make spec created more reasonable #1154

Ace-Tang commented Apr 17, 2018

allencloud commented Apr 18, 2018

codecov-io commented Apr 18, 2018 •

edited

Loading

Ace-Tang commented Apr 18, 2018

rudyfly Apr 18, 2018

Ace-Tang Apr 19, 2018

allencloud Apr 22, 2018 •

edited

Loading

allencloud Apr 22, 2018 •

edited

Loading

allencloud Apr 22, 2018

Ace-Tang Apr 25, 2018

allencloud Apr 22, 2018

Ace-Tang Apr 25, 2018

allencloud Apr 25, 2018

allencloud commented Apr 22, 2018

Ace-Tang commented Apr 25, 2018

rudyfly Apr 25, 2018

allencloud commented Apr 25, 2018

refactor: make spec created more reasonable #1154

refactor: make spec created more reasonable #1154

Conversation

Ace-Tang commented Apr 17, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

allencloud commented Apr 18, 2018

codecov-io commented Apr 18, 2018 • edited Loading

Codecov Report

Ace-Tang commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

allencloud Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Apr 22, 2018

Ace-Tang commented Apr 25, 2018

Choose a reason for hiding this comment

allencloud commented Apr 25, 2018

codecov-io commented Apr 18, 2018 •

edited

Loading

allencloud Apr 22, 2018 •

edited

Loading

allencloud Apr 22, 2018 •

edited

Loading