[Devel] [PATCH 1/2] vzctl: synchronize CRIU with vzctl

[Kir Kolyshkin]

On 05/31/2013 02:56 AM, Andrey Vagin wrote:
> vzctl provides two descriptors signalfd and waitfd, it's used for apply
> host-side configuration after creating environment.
> A signal is send to signalfd after creating environment and an answer is
> recieved from waitfd.

Looks awesome overall. See some notes below.

> Signed-off-by: Andrey Vagin <avagin at openvz.org>
> ---
>   scripts/Makefile.am    |  3 ++-
>   scripts/vps-rst-env.in | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   scripts/vps-rst.in     |  4 ++++
>   src/lib/hooks_ct.c     | 33 +++++++++++++++++++++------------
>   4 files changed, 71 insertions(+), 13 deletions(-)
>   create mode 100755 scripts/vps-rst-env.in
>
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 05381f8..e77f19d 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -29,7 +29,8 @@ script_SCRIPTS = \
>   	vzevent-reboot \
>   	vps-pci	\
>   	vps-cpt \
> -	vps-rst
> +	vps-rst \
> +	vps-rst-env
>   
>   EXTRA_DIST = \
>   	$(script_SCRIPTS:%=%.in)
> diff --git a/scripts/vps-rst-env.in b/scripts/vps-rst-env.in
> new file mode 100755
> index 0000000..a0ec898
> --- /dev/null
> +++ b/scripts/vps-rst-env.in
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +#  Copyright (C) 2013, Parallels, Inc. All rights reserved.
> +#
> +#  This program is free software; you can redistribute it and/or modify
> +#  it under the terms of the GNU General Public License as published by
> +#  the Free Software Foundation; either version 2 of the License, or
> +#  (at your option) any later version.
> +#
> +#  This program is distributed in the hope that it will be useful,
> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#  GNU General Public License for more details.
> +#
> +#  You should have received a copy of the GNU General Public License
> +#  along with this program; if not, write to the Free Software
> +#  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +#
> +# This script is called by CRIU (http://criu.org) after creating namespaces.
> +#
> +# Parameters are passed in environment variables.
> +# Required parameters:
> +#   VZCTL_PID	  - pid of vzctl

whitespace at EOL

> +#   STATUSFD	  - file descriptor for sending signal to vzctl
> +#   WAITFD	  - file descriptor for receiving signal from vzctl
> +#   VE_STATE_FILE - file with PID of container init process
> +#   VE_NETNS_FILE - file, which used by ip netns

There's also CRTOOLS_SCRIPT_ACTION which is set by criu.

> +
> +[[ "setup-namespaces" == "$CRTOOLS_SCRIPT_ACTION" ]] || exit 0

bashism

it should be something like

[ "x$CRTOOLS_SCRIPT_ACTION" = "xsetup-namespaces" ] || exit 0

> +
> +exec 1>&2

why?

> +. @SCRIPTDIR@/vps-functions
> +
> +vzcheckvar VZCTL_PID
> +vzcheckvar STATUSFD
> +vzcheckvar WAITFD
> +vzcheckvar VE_NETNS_FILE
> +vzcheckvar VE_STATE_FILE
> +

maybe you should do 'set -e' here, since you are running some commands
below that may fail, and you don't check their exit codes.

> +pid=$(cat $VE_STATE_FILE)
> +ln -sf /proc/$pid/ns/net $VE_NETNS_FILE
> +
> +echo -ne '\0\0\0\0' > /proc/$VZCTL_PID/fd/$STATUSFD
> +ret=$(cat /proc/$VZCTL_PID/fd/$WAITFD | xxd -p -l 4)

Oh well, xxd comes from vim. I doubt we want to have vim as a dependency.

Maybe hexdump -e '"%d"'?
> +[ "$ret" = "00000000" ] && exit 0 || exit 1

You don't have to do that, just

	[ "$ret" = "00000000" ]
	exit

and "exit" is optional if it's last line of the file.


> diff --git a/scripts/vps-rst.in b/scripts/vps-rst.in
> index 91080b3..7cfe658 100755
> --- a/scripts/vps-rst.in
> +++ b/scripts/vps-rst.in
> @@ -26,6 +26,8 @@
>   #   VE_DUMP_DIR   - directory for saving dump files
>   #   VE_STATE_FILE - file to write CT init PID to
>   #   VE_VETH_DEVS  - pair of veth names (CT=HW\n)
> +#   VE_NS_SCRIPT  - script, which will be executed after creating namespaces

1 "script to execute after creating namespaces"

2 Can we just hardcode it to vps-rst-env?

> +#
>   
>   exec 1>&2
>   . @SCRIPTDIR@/vps-functions
> @@ -34,6 +36,7 @@ vzcheckvar VE_ROOT
>   vzcheckvar VE_STATE_FILE
>   vzcheckvar VE_DUMP_DIR
>   vzcheckvar VE_VETH_DEVS
> +vzcheckvar VE_NS_SCRIPT
>   
>   veth_args=""
>   for dev in $VE_VETH_DEVS; do
> @@ -46,6 +49,7 @@ criu restore	--file-locks		\
>   		--link-remap		\
>   		--root $VE_ROOT		\
>   		--restore-detached	\
> +		--action-script $VE_NS_SCRIPT \
>   		-D $VE_DUMP_DIR		\
>   		-o restore.log		\
>   		-vvvv			\
> diff --git a/src/lib/hooks_ct.c b/src/lib/hooks_ct.c
> index 18650e0..bf20779 100644
> --- a/src/lib/hooks_ct.c
> +++ b/src/lib/hooks_ct.c
> @@ -391,6 +391,7 @@ static int ct_env_create_real(struct arg_start *arg)
>   	int userns_p[2];
>   	int ret, fd;
>   	char pidpath[STR_SIZE];
> +	char ctpath[STR_SIZE];
>   
>   	stack_size = get_pagesize();
>   	if (stack_size < 0)
> @@ -478,13 +479,20 @@ static int ct_env_create_real(struct arg_start *arg)
>   		close(userns_p[1]);
>   	}
>   
> +	snprintf(ctpath, STR_SIZE, "%s/%d", NETNS_RUN_DIR, arg->veid);
> +	snprintf(pidpath, STR_SIZE, "/proc/%d/ns/net", ret);
> +	if (symlink(pidpath, ctpath)) {
> +		logger(-1, errno, "Can't symlink into netns file %s", ctpath);
> +		destroy_container(arg->veid);
> +		return -VZ_RESOURCE_ERROR;
> +	}
> +
>   	return ret;
>   }
>   
>   int ct_env_create(struct arg_start *arg)
>   {
>   	int ret;
> -	char procpath[STR_SIZE];
>   	char ctpath[STR_SIZE];
>   
>   	/* non-fatal */
> @@ -519,14 +527,6 @@ int ct_env_create(struct arg_start *arg)
>   	if (ret < 0)
>   		return -ret;
>   
> -	snprintf(procpath, STR_SIZE, "/proc/%d/ns/net", ret);
> -	ret = symlink(procpath, ctpath);
> -	if (ret) {
> -		logger(-1, errno, "Can't symlink into netns file %s", ctpath);
> -		destroy_container(arg->veid);
> -		return VZ_RESOURCE_ERROR;
> -	}
> -
>   	return 0;
>   }
>   
> @@ -924,7 +924,7 @@ static int ct_chkpnt(vps_handler *h, envid_t veid,
>   static int ct_restore_fn(vps_handler *h, envid_t veid, const vps_res *res,
>   			  int wait_p, int old_wait_p, int err_p, void *data)
>   {
> -	char *argv[2], *env[5];
> +	char *argv[2], *env[10];
>   	const char *dumpfile = NULL;
>   	const char *statefile = NULL;
>   	cpt_param *param = data;
> @@ -957,8 +957,17 @@ static int ct_restore_fn(vps_handler *h, envid_t veid, const vps_res *res,
>   				"%s=%s\n", veth->dev_name_ve, veth->dev_name);
>   	}
>   	env[3] = strdup(buf);
> -
> -	env[4] = NULL;
> +	snprintf(buf, sizeof(buf), "VE_NS_SCRIPT=%s", SCRIPTDIR "/vps-rst-env");
> +	env[4] = strdup(buf);
> +	snprintf(buf, sizeof(buf), "VZCTL_PID=%d", getpid());
> +	env[5] = strdup(buf);
> +	snprintf(buf, sizeof(buf), "STATUSFD=%d", STDIN_FILENO);
> +	env[6] = strdup(buf);
> +	snprintf(buf, sizeof(buf), "WAITFD=%d", wait_p);
> +	env[7] = strdup(buf);
> +	snprintf(buf, sizeof(buf), "VE_NETNS_FILE=%s/%d", NETNS_RUN_DIR, veid);
> +	env[8] = strdup(buf);
> +	env[9] = NULL;
>   
>   	ret = run_script(argv[0], argv, env, 0);
>   	free_arg(env);