Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update moby to containerd and runc 1.0 final rc #33007

Merged
merged 1 commit into from May 8, 2017

Conversation

crosbymichael
Copy link
Contributor

Getting this up for review, I'll have to change the repo paths one last time before we can merge but CI takes so long to run I wanted to open this now.

This is the last rc for the runtime spec and runc before a 1.0 release.

@vieux
Copy link
Contributor

vieux commented May 4, 2017

@crosbymichael panic in the unit tests

@AkihiroSuda AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label May 4, 2017
@crosbymichael
Copy link
Contributor Author

crosbymichael commented May 4, 2017

@LK4D4 do you know why vndr is freaking out about my dep?

it was saying

18:36:29 2017/05/02 18:36:30 WARNING: packages 'github.com/opencontainers/selinux, github.com/opencontainers/selinux' has same root import github.com/opencontainers/selinux
18:36:29 2017/05/02 18:36:30 WARNING: suggested vendor.conf is written to vendor.conf.tmp, use diff and common sense before using it
18:36:29 2017/05/02 18:36:30 There were some validation errors

@@ -12,6 +12,25 @@ func iPtr(i int64) *int64 { return &i }
func u32Ptr(i int64) *uint32 { u := uint32(i); return &u }
Copy link
Member

Choose a reason for hiding this comment

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

sPtr should be unused now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

result = icmd.RunCmd(cmd)
// for some reason there is carriage return in the output. i think this is
// just expected.
c.Assert(result, icmd.Matches, icmd.Expected{Out: "out\r\nerr\r\n"})
Copy link
Member

Choose a reason for hiding this comment

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

The test should still be meaningful. Just remove carriage return.

Copy link
Member

Choose a reason for hiding this comment

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

could remove the comment as well

@crosbymichael crosbymichael removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 5, 2017
@vieux
Copy link
Contributor

vieux commented May 5, 2017

@crosbymichael can you rebase ?

github.com/opencontainers/selinux ba1aefe8057f1d0cfb8e88d0ec1dc85925ef987d is already in master (last line of the vendor.conf)

That's why you end up with selinux errors with vndr.

@crosbymichael
Copy link
Contributor Author

@vieux thanks

@crosbymichael
Copy link
Contributor Author

I'm having to update and sync the deps with runc because some of them are out of date and causing the validation to fail. The others will be a bump for docker's versions where runc depends on a newer version.

@crosbymichael
Copy link
Contributor Author

Can we turn off this new vendor comparison because runc is a binary dep I don't know why its diffing the vendor files.

@vdemeester
Copy link
Member

vdemeester commented May 5, 2017

@crosbymichael as long as vendor.conf or vendor/* files are changed, the vendor check is run

16:22:02 ?? vendor/github.com/opencontainers/runc/vendor.conf

This file seems to be missing in the commit but is while using vndr.

@crosbymichael
Copy link
Contributor Author

@vdemeester its comparing things it shouldn't because runc has a vendor.conf that is not used here because its a binary dep and shouldn't matter.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@thaJeztah
Copy link
Member

All green now @tonistiigi @mlaventure PTAL!

@@ -77,7 +77,7 @@ func getMemoryResources(config containertypes.Resources) *specs.Memory {
memory.Reservation = &reservation
}

if config.MemorySwap != 0 {
if config.MemorySwap > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This one has been tricky, IIRC.

Seems -1 (previously unlimited) and 0 (unset) are now treated the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

This routine seems to only be used when creating the container spec initially (not when updating), in which case it's correct. The default value for that limit being unlimited.

Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

(one small nit)


if config.CPUShares != 0 {
if config.CPUShares < 0 {
return nil, fmt.Errorf("shares: invalid argument")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: cpu shares: ...

@tonistiigi
Copy link
Member

LGTM

@mlaventure mlaventure merged commit 7238cca into moby:master May 8, 2017
@crosbymichael crosbymichael deleted the containerd-rc5 branch May 8, 2017 16:23
@thaJeztah
Copy link
Member

@mlaventure can you verify if #32667 and #32699 should be resolved by this?

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

Successfully merging this pull request may close these issues.

None yet

9 participants