Skip to content

lcow: Allow the client to adjust capabilities and device cgroup rules#37294

Merged
thaJeztah merged 2 commits intomoby:masterfrom
jstarks:lcow_caps
Jun 20, 2018
Merged

lcow: Allow the client to adjust capabilities and device cgroup rules#37294
thaJeztah merged 2 commits intomoby:masterfrom
jstarks:lcow_caps

Conversation

@jstarks
Copy link
Contributor

@jstarks jstarks commented Jun 15, 2018

- What I did
Allowed LCOW clients to pass --cap-add, --device-group-rule etc. to customize the capabilities and device cgroup rules for their containers.
- How I did it
Moved the capabilities- and device cgroup-related OCI spec creation code from Linux-specific code to a common area.
- How to verify it
docker run --cap-add=SYSLOG ubuntu dmesg now succeeds on LCOW.
- Description for the changelog
Allow the client to customize capabilities and device cgroup rules for LCOW containers

- A picture of a cute animal (not mandatory but encouraged)

@jstarks
Copy link
Contributor Author

jstarks commented Jun 15, 2018

@jhowardmsft PTAL

Signed-off-by: John Starks <jostarks@microsoft.com>
Signed-off-by: John Starks <jostarks@microsoft.com>
@jstarks jstarks changed the title lcow: Allow the client to add or remove capabilities lcow: Allow the client to adjust capabilities and device cgroup rules Jun 15, 2018
Copy link
Member

@lowenna lowenna left a comment

Choose a reason for hiding this comment

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

Nice. LGTM.

@johnstep PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Two minor nits, but overall looks okay

s.Root.Path = "rootfs"
s.Root.Readonly = c.HostConfig.ReadonlyRootfs
if err := setCapabilities(s, c); err != nil {
return fmt.Errorf("linux spec capabilities: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "failed to set ....." ?

Also may want to use errors.Wrap(err, "failed to set ...") to preserve the original error

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 copied these errors from the Linux use of the functions. Do you want me to update both places or leave it as is?

(Maybe better to avoid duplication altogether but doing that in this change seemed to be more trouble than its worth right now.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Didn't check be Linux equivalent, but now recall I saw those at some point and wanted to update them.

Let's keep them as-is for now to be consistent, and keep it for a separate PR to improve if we want to

}
devPermissions, err := appendDevicePermissionsFromCgroupRules(nil, c.HostConfig.DeviceCgroupRules)
if err != nil {
return fmt.Errorf("linux runtime spec devices: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Same here (errors.Wrap()), and perhaps failed to .... (or something along that line)

Copy link
Member

@johnstep johnstep left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@yongtang
Copy link
Member

yongtang commented Jun 17, 2018

The only failure is:

FAIL: docker_cli_daemon_test.go:1800: DockerDaemonSuite.TestDaemonNoSpaceLeftOnDeviceError

which I think is unrelated.

@thaJeztah
Copy link
Member

That failure will be addressed through #37315

@thaJeztah
Copy link
Member

same test failed again; but it's unrelated, so merging

@thaJeztah thaJeztah merged commit e259323 into moby:master Jun 20, 2018
@thaJeztah thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018
@itsgk92
Copy link

itsgk92 commented Jan 24, 2019

Is there a edge release available for this yet?

@thaJeztah
Copy link
Member

@itsgk92 this was merged un June last year, and is part of Docker 18.06 and up

@itsgk92
Copy link

itsgk92 commented Jan 24, 2019

I couldn't find it working. #38631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants