Skip to content

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

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

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

Conversation

@jstarks

@jstarks jstarks commented Jun 15, 2018

Copy link
Copy Markdown
Contributor

- 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

jstarks commented Jun 15, 2018

Copy link
Copy Markdown
Contributor Author

@jhowardmsft PTAL

@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

@lowenna lowenna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice. LGTM.

@johnstep PTAL

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

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

Comment thread daemon/oci_windows.go
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread daemon/oci_windows.go
}
devPermissions, err := appendDevicePermissionsFromCgroupRules(nil, c.HostConfig.DeviceCgroupRules)
if err != nil {
return fmt.Errorf("linux runtime spec devices: %v", err)

Copy link
Copy Markdown
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)

@johnstep johnstep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@yongtang

yongtang commented Jun 17, 2018

Copy link
Copy Markdown
Member

The only failure is:

FAIL: docker_cli_daemon_test.go:1800: DockerDaemonSuite.TestDaemonNoSpaceLeftOnDeviceError

which I think is unrelated.

@thaJeztah

Copy link
Copy Markdown
Member

That failure will be addressed through #37315

@thaJeztah

Copy link
Copy Markdown
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

itsgk92 commented Jan 24, 2019

Copy link
Copy Markdown

Is there a edge release available for this yet?

@thaJeztah

Copy link
Copy Markdown
Member

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

@itsgk92

itsgk92 commented Jan 24, 2019

Copy link
Copy Markdown

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