merge nested warden into master #27

Merged
merged 20 commits into from Sep 12, 2013

Projects

None yet

6 participants

@syslxg
Member
syslxg commented Aug 15, 2013

merge nested warden into master

David Sabeti... and others added some commits Apr 5, 2013
David Sabeti and Pieter Noordhuis Allow nesting via `allow_nested_warden` config flag
Setting this flag will create a number of bind mounts into /tmp/warden
inside the container such that warden can run inside of a container.
c8446d7
David Sabeti and Pieter Noordhuis Remove cleanup of old cgroup mount e932f25
David Sabeti and Pieter Noordhuis Explicitly allow inbound traffic on FORWARD chain
Nested warden forwards both inbound and outbound traffic on interfaces
that match the "w-+" wildcard. This means that inbound traffic is
dropped by default. This change makes sure that traffic that comes in
via the default outbound interface is always allowed.
b1cb5f3
David Sabeti and Pieter Noordhuis Only setup cgroups if the cgroup path does not exist 6e1842c
David Sabeti and Pieter Noordhuis Never try to disable quota e833b9b
David Sabeti and Pieter Noordhuis Allow access to all devices for nested warden be58d88
David Sabeti and Pieter Noordhuis Add config for warden-cpi 9712451
David Sabeti and Tony Hansmann Remove grace timer from warden cpi config f664755
@Kaixiang Kaixiang make /var rw when setting fs for non-ubuntu linux 105df82
Gary Liu complete release deplay support for network pool 0781da5
@Kaixiang Kaixiang add default value when no ip_local_port_range found in /proc 0fa1f98
Gary Liu Merge branch 'master' into nested 2169405
Gary Liu delete warden/config/warden-cpi-linux.yml 4766176
Member
syslxg commented Aug 21, 2013

built a cf-release using this branch. yeti passed:

76 examples, 2 failures, 13 pendings

the two failures are about loggregator, not related with warden. maybe it is because of missing port forwarding rules on ELB.

@ryantang ryantang commented on the diff Aug 23, 2013
warden/lib/warden/config.rb
@@ -106,7 +110,12 @@ def self.network_schema
end
def self.ip_local_port_range
- File.read("/proc/sys/net/ipv4/ip_local_port_range").split.map(&:to_i)
+ # if no ip_local_port_range found, make some guess"
+ if File.exist?("/proc/sys/net/ipv4/ip_local_port_range")
+ File.read("/proc/sys/net/ipv4/ip_local_port_range").split.map(&:to_i)
+ else
+ return 32768, 61000
ryantang
ryantang Aug 23, 2013

Why this is required for nested warden functionality?

@mariash @ryantang

Kaixiang
Kaixiang Aug 27, 2013 Member

in newer kernel , /proc/sys/net/ipv4/ip_local_port_range is not exported inside container anymore.
so we should check whether it exist before provide some default values.

cf-frameworks
cf-frameworks Sep 11, 2013 Contributor

OK, makes sense.

@vito & @mariash

@ryantang ryantang and 1 other commented on an outdated diff Aug 23, 2013
warden/lib/warden/container/linux.rb
file.puts
file.puts
- request.bind_mounts.each do |bind_mount|
- src_path = bind_mount.src_path
- dst_path = bind_mount.dst_path
-
- # Check that the source path exists
- stat = File.stat(src_path) rescue nil
- if stat.nil?
- raise WardenError.new("Source path for bind mount does not exist: #{src_path}")
- end
-
- # Fix up destination path to be an absolute path inside the union
+ add_bind_mount = lambda do |src_path, dst_path, mode|
ryantang
ryantang Aug 23, 2013

Can we move this to separate function?

@mariash @ryantang

syslxg
syslxg Aug 27, 2013 Member

change the lambda to method in this commit
02d6a29

@ryantang ryantang and 1 other commented on an outdated diff Aug 23, 2013
warden/lib/warden/container/linux.rb
+ when Protocol::CreateRequest::BindMount::Mode::RW
+ "rw"
+ else
+ raise "Unknown mode"
+ end
+
+ add_bind_mount.call(src_path, dst_path, mode)
+ end
+ end
+
+ if Server.config.allow_nested_warden?
+ tmp_warden = File.join(container_path, "tmp", "warden")
+ FileUtils.mkdir_p(tmp_warden)
+
+ # Bind-mount containers
+ add_bind_mount.call(tmp_warden, "/tmp/warden", "rw")
ryantang
ryantang Aug 23, 2013

Seems like these directories should be configurable.

@mariash @ryantang

syslxg
syslxg Aug 27, 2013 Member

/tmp/warden/cgroup/* is hardcoded
/tmp/warden/rootfs can be configurable. it is for sharing rootfs between parent warden and child warden. But we think it is better to skip it for now. because warden-cpi don't need this feature.

syslxg
syslxg Aug 27, 2013 Member

this commit remove the rootfs sharing: 02d6a29

@ryantang ryantang and 1 other commented on an outdated diff Aug 23, 2013
warden/root/linux/setup.sh
@@ -7,43 +7,29 @@ shopt -s nullglob
cd $(dirname "${0}")
-# Check if the old mount point exists, and if so clean it up
ryantang
ryantang Aug 23, 2013

Why is the cleaning up of old mounts removed?

@mariash @ryantang

cf-frameworks
cf-frameworks Sep 11, 2013 Contributor

This may be OK. It looks like /dev/cgroup isn't referenced anywhere else in Warden, but maybe it's trying to clean up after some initial OS configuration for it?

@vito & @mariash

@ryantang ryantang and 1 other commented on an outdated diff Aug 23, 2013
warden/root/linux/setup.sh
- if ! grep -q "${cgroup_path}/$subsystem " /proc/mounts
+ # Mount tmpfs
+ if ! grep "${cgroup_path} " /proc/mounts | cut -d' ' -f3 | grep -q tmpfs
ryantang
ryantang Aug 23, 2013

This block of code is never executed if the $cgroup_path directory existed prior to line 12. What if the $cgroup directory exists and not mounted to /tmpfs? In this case, we'll be writing on the parent FS.

@mariash @ryantang

syslxg
syslxg Aug 27, 2013 Member

writing on the parent cgroup FS is exactly the solution for nested warden: sharing cgroup fs since we share one kernel among host & containers. it is possible to setup separate cgroup fs for each warden server (parent and child), but their content will be the same. so NO points to do so.

and, like nested resource groups in vCenter, child containers' cgroups should under parent containers' groups, like this: /tmp/warden/cgroup/cpu/parent_container/child_container.

@ryantang ryantang and 1 other commented on an outdated diff Aug 23, 2013
warden/root/linux/setup.sh
then
mount -o remount,usrjquota=aquota.user,grpjquota=aquota.group,jqfmt=vfsv0 $CONTAINER_DEPOT_MOUNT_POINT_PATH
quotacheck -ugmb -F vfsv0 $CONTAINER_DEPOT_MOUNT_POINT_PATH
quotaon $CONTAINER_DEPOT_MOUNT_POINT_PATH
-elif [ "$DISK_QUOTA_ENABLED" = "false" ] && ! quotaon -p $CONTAINER_DEPOT_MOUNT_POINT_PATH > /dev/null
-then
- quotaoff $CONTAINER_DEPOT_MOUNT_POINT_PATH
ryantang
ryantang Aug 23, 2013

In case if we enable and then disable we don't do quotaoff. Why was this removed?

@mariash @ryantang

Kaixiang
Kaixiang Aug 27, 2013 Member

don't know what was the original scenario for this commit.
but warden-cpi don't rely on this change.

@ryantang ryantang commented on the diff Aug 23, 2013
...en/root/linux/skeleton/lib/hook-parent-after-clone.sh
- # /dev/zero
- echo "c 1:5 rw" > $instance_path/devices.allow
- # /dev/random
- echo "c 1:8 rw" > $instance_path/devices.allow
- # /dev/urandom
- echo "c 1:9 rw" > $instance_path/devices.allow
- # /dev/tty
- echo "c 5:0 rw" > $instance_path/devices.allow
- # /dev/ptmx
- echo "c 5:2 rw" > $instance_path/devices.allow
- # /dev/pts/*
- echo "c 136:* rw" > $instance_path/devices.allow
+ if [ $allow_nested_warden == "true" ]
+ then
+ # Allow everything
+ echo "a *:* rw" > $instance_path/devices.allow
ryantang
ryantang Aug 23, 2013

Can you explain why you need everything? It seems like the right thing to do is to just add the specific devices you need.

@mariash @ryantang

Kaixiang
Kaixiang Aug 27, 2013 Member

at least one thing fail as I observed, when untar rootfs package for dea job, a lot of device node need mknod.
otherwise we will fail because of Operation not permitted. (and we want share the same package from cf-realase master)

and since warden-cpi are the only consumer of nested warden, we see no requirement to restrict devices access for it right now.

cf-frameworks
cf-frameworks Sep 11, 2013 Contributor

Ok, makes sense.

@ryantang ryantang commented on the diff Aug 23, 2013
warden/root/linux/skeleton/net.sh
@@ -38,7 +38,7 @@ function setup_filter() {
--goto ${filter_default_chain}
# Bind instance chain to forward chain
- iptables -I ${filter_forward_chain} \
+ iptables -I ${filter_forward_chain} 2 \
ryantang
ryantang Aug 23, 2013

Why do you need this change? Can you add a test to showcase the desired behavior?

@mariash @ryantang

cf-frameworks
cf-frameworks Sep 11, 2013 Contributor

Was this added by you guys or Pieter & David?

@vito & @mariash

@Kaixiang @syslxg — Please add tests with this commit. @mariash and I commented out lines 229 through 242, starting with if Server.config.allow_nested_warden? from linux.rb of this commit. We ran the tests, and everything still passed. We should see at least one failing test as a result of this exercise, telling us that an important feature is broken.

We'd be happy to help you out on this. Please let us know. Thanks.

@Kaixiang @mkocher Any updates on this one ?

Member
syslxg commented Sep 9, 2013

@ryantang @AndreasMaier @mariash we added tests for nested-warden, and answered your comments above.
Please review this pr.
let us know if you have any questions. @Kaixiang @mkocher

@cf-frameworks cf-frameworks commented on the diff Sep 11, 2013
warden/lib/warden/container/linux.rb
@@ -194,38 +195,49 @@ def perform_rsync(src_path, dst_path)
sh *args
end
- def write_bind_mount_commands(request)
- return if request.bind_mounts.nil? || request.bind_mounts.empty?
+ def add_bind_mount(file, src_path, dst_path, mode)
cf-frameworks
cf-frameworks Sep 11, 2013 Contributor

Will this blow up if src doesn't exist? Previously we used to File.stat it and raise a Warden error if it didn't exist.

@vito & @mariash

Member

After pairing review the PR this morning with @dsabeti @syslxg , we all agree to merge the pull request in the review. as far as we fix the src_path check and revert some unused commit, and add an outbound traffic test for nested warden. all done and the tests pass. so we merge it now.

contact @dsabeti if you have seen some issue when it's out of our time

@syslxg syslxg merged commit f25ec67 into master Sep 12, 2013
@drnic
Member
drnic commented on 0fa1f98 Sep 17, 2013

@Kaixiang @mkocher why this month-old patch isn't in cf-141? did it only recently get merged into warden master branch?

Member

I noticed its absence in cloudfoundry/cf-release#179

Member

because it is in nested branch instead of warden-master. the nested branch was just merged into warden-master recently, and src/warden in cf-release was bumped to pickup the nested warden feature on Sep 13.

Member
@fraenkel fraenkel deleted the nested-rebase branch Jan 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment