[libvirt] [RFC] Power Hypervisor Libvirt support
Eduardo Otubo
otubo at linux.vnet.ibm.com
Fri Mar 27 20:33:43 UTC 2009
Hello Daniel,
> > The protocol between the console and the partition (LPAR) is not
> > disclosed, therefore I propose the driver to execute commands remoetly
> > over an SSH connection to the consoles to manage IBM LPARs.
>
> That seems like a reasonably choice, unless the web server in the
> HMC/IVM provided a good formal API like xmlrpc.
There is no other way of having this done. No xmlrpc or connections
directly to the hypervisor. Executing commands over SSH is not
beautiful, but is the only way we have.
> > The patch attached is the first scratch of the driver that will interact
> > with HMC over a SSH connection. The URI model that is
> > used in virsh command line is:
> >
> > virsh --conect phyp://$user@$server
> >
> > Some known issues are:
> > * Next step is to make the URI like this: phyp://$user@
> > $HMC/@managed_system. Almost finished. What it takes now is
> > $server = $HMC = $managed_system.
> > * Next features in my TODO list are "resume", "stop" and "reboot" the
> > LPAR.
>
> So if I'm understanding what I read on the web correctly, the HMC is a
> machine that is typically separate from the host running virtualization.
> Thus a single HMC can manage multiple hosts.
Yes, that's right. HMC, the Hardware Management Console, is a separate
machine that maganes one or more servers with virtualized machines.
>
> For the URI scheme, I'd see two possible options
>
> Either use the path component of the URI for managed system name.
>
> phyp://$user@$HMC/$managedsystem
>
> Or use a query parameter
>
> phyp://$user@$HMC/?managedsystem=$managedsystem
>
> I reckon I'd favour the former.
Yes, the first model was already on my todo list and it's prettu fine to
me.
> > diff --git a/configure.in b/configure.in
> > index 6b2bb5e..201a7dc 100644
> > --- a/configure.in
> > +++ b/configure.in
> > @@ -182,6 +182,8 @@ AC_ARG_WITH([uml],
> > [ --with-uml add UML support (on)],[],[with_uml=yes])
> > AC_ARG_WITH([openvz],
> > [ --with-openvz add OpenVZ support (on)],[],[with_openvz=yes])
> > +AC_ARG_WITH([phyp],
> > +[ --with-phyp add IBM HMC/IVM support (on)],[],[with_phyp=yes])
> > AC_ARG_WITH([lxc],
> > [ --with-lxc add Linux Container support (on)],[],[with_lxc=yes])
> > AC_ARG_WITH([test],
> > @@ -714,6 +716,21 @@ AM_CONDITIONAL([HAVE_NUMACTL], [test "$with_numactl" != "no"])
> > AC_SUBST([NUMACTL_CFLAGS])
> > AC_SUBST([NUMACTL_LIBS])
> >
> > +if test "$with_phyp" = "yes"; then
> > + AC_CHECK_LIB([ssh],[ssh_new],[
> > + LIBSSH_LIBS="$LIBSSH_LIBS -lssh -L/usr/local/lib/"
> > + AC_SUBST([LIBSSH_LIBS])],[
> > + AC_MSG_ERROR([You must install the libssh to compile Phype driver.])])
> > +
> > + AC_CHECK_HEADERS([libssh/libssh.h],[
> > + LIBSSH_CFLAGS="-I/usr/local/include/libssh"
> > + AC_SUBST([LIBSSH_CFLAGS])],[
> > + AC_MSG_ERROR([Cannot find libssh headers.Is libssh installed ?])],[])
> > + AC_DEFINE_UNQUOTED([WITH_PHYP], 1,
> > + [whether IBM HMC / IVM driver is enabled])
> > +fi
> > +AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"])
>
> For this it is preferable to avoid using hardcoded paths in this way. If
> we are going to use libssh2 for phyp driver, then I reckon it woul dbe
> desirable to also use it for our existing remote RPC driver too (it currently
> just fork/exec's /usr/bin/ssh).
In fact I use libssh <http://0xbadc0de.be/wiki/libssh:libssh> and not
libssh2. I choosed libssh instead of libssh2 for some reasons: libssh2
doesn't handle server side ssh or sshv1. And I am more used to work with
its API then libssh2.
> Thus I'd recommend adding an explicit arg
> for --with-libssh2=$PREFIX. So if someone did
>
> --with-libssh2
>
> it'd just look for it in /usr. While, if they did
>
> --with-libssh2=/usr/local
>
> then it'd search the alternate path given.
>
> Then, when processing your '$with_phyp' arg I'd recommend the following
> logic
>
> - If no --with-phyp arg is given
> - If libssh2 was detected, enable phyp driver by default
> - else disable the phyp driver by default
> - If a --with-phyp arg is given
> - If libssh2 was detected, then use it
> - Else AC_MSG_ERROR() to tell use libssh was missing & is
> needed for phyp
> - If a --without-phyp arg is given
> - diable the phyp driver
Yes, I agree with this. I'll implement this way and post it back.
> > +
> > dnl virsh libraries
> > AC_CHECK_HEADERS([readline/readline.h])
> >
> > @@ -1345,6 +1362,7 @@ AC_MSG_NOTICE([ QEMU: $with_qemu])
> > AC_MSG_NOTICE([ UML: $with_uml])
> > AC_MSG_NOTICE([ OpenVZ: $with_openvz])
> > AC_MSG_NOTICE([ LXC: $with_lxc])
> > +AC_MSG_NOTICE([ IBM HMC/IVM: $with_phyp])
> > AC_MSG_NOTICE([ Test: $with_test])
> > AC_MSG_NOTICE([ Remote: $with_remote])
> > AC_MSG_NOTICE([ Network: $with_network])
>
> Can you either shortern this to just 'Phyp: $with_phyp', or
> fix the indentation for all the other existing lines here,
> so everything is aligned sensibly.
Sure, in fact, I think IBM HMC/IVM is too high level. The final use of
this feature does not need this details. Phyp is enough information we
will need.
>
> > diff --git a/src/phyp_conf.h b/src/phyp_conf.h
> > new file mode 100644
> > index 0000000..51ca65e
> > --- /dev/null
> > +++ b/src/phyp_conf.h
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright IBM Corp. 2009
> > + *
> > + * phyp_driver.c: ssh layer to access Power Hypervisors
> > + *
> > + * Authors:
> > + * Eduardo Otubo <otubo at linux.vnet.ibm.com>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library 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
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <config.h>
>
> This should typically be done in the .c file, rather than the .h files.
Ok, this minor fixes will be at next version of this patch.
> > +static virDrvOpenStatus
> > +phypOpen(virConnectPtr conn,
> > + virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> > + int flags ATTRIBUTE_UNUSED)
> > +{
> > + if (!conn || !conn->uri)
> > + return VIR_DRV_OPEN_DECLINED;
> > +
> > + if (conn->uri->scheme == NULL || conn->uri->server == NULL)
> > + return VIR_DRV_OPEN_DECLINED;
> > +
> > + int ssh_auth = 0, exit_status = 0;
> > + char *banner;
> > + char *password;
> > +
> > + SSH_SESSION *session;
> > + SSH_OPTIONS *opt;
>
> Our general coding style is to keep the variabl declarations at
> the start of the nearest enclosing {} block. So either at start
> of the function, or start of the while/if/for loop.
Ok, I am still getting used to your coding style. I read the
HACKING file and followed all steps and details strictly, but some gone
missing like this. Will fix and also will be in the next version.
>
> > +
> > + session = ssh_new();
> > + opt = ssh_options_new();
> > +
> > + if (!conn->uri->port)
> > + conn->uri->port = 22;
> > +
> > + /*setting some ssh options */
> > + ssh_options_set_host(opt, conn->uri->server);
> > + ssh_options_set_port(opt, conn->uri->port);
> > + ssh_options_set_username(opt, conn->uri->user);
> > + ssh_set_options(session, opt);
> > +
> > + /*starting ssh connection */
> > + if (ssh_connect(session)) {
> > + fprintf(stderr, "Connection failed : %s\n",
> > + ssh_get_error(session));
> > + ssh_disconnect(session);
> > + ssh_finalize();
> > + exit_status = SSH_CONN_ERR;
> > + return VIR_DRV_OPEN_DECLINED;
> > + }
> > +
> > + /*trying to use pub key */
> > + if ((ssh_auth =
> > + ssh_userauth_autopubkey(session, NULL)) == SSH_AUTH_ERROR) {
> > + fprintf(stderr, "Authenticating with pubkey: %s\n",
> > + ssh_get_error(session));
> > + exit_status = SSH_AUTH_PUBKEY_ERR;
> > + return VIR_DRV_OPEN_DECLINED;
> > + }
> > +
> > + if ((banner = ssh_get_issue_banner(session))) {
> > + printf("%s\n", banner);
> > + VIR_FREE(banner);
> > + }
> > +
> > + if (ssh_auth != SSH_AUTH_SUCCESS) {
> > + password = getpass("Password: ");
>
> Rather than calling 'getpass', you should check to see if the caller
> has suplied an authentication callback - eg the
>
> 'virConnectAuthPtr auth'
>
> parameter which is passed in to this open method. 'getpass' will
> prompt on the command line, which isn't suitable for graphical
> apps, so you need to use the callback for collecting credentials.
>
> virsh has a default callback implementation that knows how to
> prompt for both username and password if requested by then open
> method,
>
> You could also use this to prompt for a passphrase if the SSH
> private key is not unlocked.
This improve for the authentication method is in my todo list too. I
have in mind that virsh won't be the only application that will handle
this, I may have many others. But thanks for the comment.
>
> > + if (ssh_userauth_password(session, NULL, password) !=
> > + SSH_AUTH_SUCCESS) {
> > + fprintf(stderr, "Authentication failed: %s\n",
> > + ssh_get_error(session));
> > + ssh_disconnect(session);
> > + memset(password, 0, strlen(password));
> > + exit_status = SSH_AUTH_ERR;
> > + return VIR_DRV_OPEN_DECLINED;
> > + } else
> > + goto exec;
> > + } else
> > + goto exec;
> > +
> > + exec:
> > + conn->privateData = session;
> > + return VIR_DRV_OPEN_SUCCESS;
> > +}
> > +
> > +static int
> > +phypClose(virConnectPtr conn)
> > +{
> > + SSH_SESSION *ssh_session = conn->privateData;
> > +
> > + ssh_disconnect(ssh_session);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +__inner_exec_command(SSH_SESSION * session, char *cmd,
> > + char *textual_return)
> > +{
> > + CHANNEL *channel = channel_new(session);
> > + BUFFER *readbuf = buffer_new();
> > +
> > + int exit_status = 0, temp_return = 0, buffer_size = 0;
> > +
> > + memset(textual_return, 0, sizeof(textual_return));
> > +
> > + if (channel_open_session(channel) == SSH_ERROR)
> > + return SSH_CHANNEL_OPEN_ERR;
> > +
> > + if (channel_request_exec(channel, cmd) == SSH_ERROR)
> > + return SSH_CHANNEL_EXEC_ERR;
> > +
> > + while (channel && channel_is_open(channel)) {
> > + temp_return = channel_read(channel, readbuf, 0, 0);
> > +
> > + strncat(textual_return, buffer_get(readbuf),
> > + buffer_get_len(readbuf));
> > + buffer_size += buffer_size + buffer_get_len(readbuf);
> > +
> > + if (temp_return == 0) {
> > + textual_return[buffer_size + 1] = 0;
> > + exit_status = channel_get_exit_status(channel);
> > + break;
> > + }
> > + }
> > +
> > + if (channel_send_eof(channel) == SSH_ERROR)
> > + return SSH_CHANNEL_SEND_EOF_ERR;
> > +
> > + if (channel_close(channel) == SSH_ERROR)
> > + return SSH_CHANNEL_CLOSE_ERR;
> > +
> > + return exit_status;
> > +}
> > +
> > +static int
> > +phypGetLparID(SSH_SESSION * ssh_session, const char *managed_system,
> > + const char *name)
> > +{
> > + char id_c[10];
> > + unsigned int i = 0;
> > + int ret = 0;
> > + char *cmd;
> > + char *textual_return;
> > +
> > + if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(name)+sizeof(managed_system))) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + memset(id_c, 0, sizeof(id_c));
> > +
> > + sprintf(cmd,
> > + "lssyscfg -r lpar -m %s --filter lpar_names=%s -F lpar_id",
> > + managed_system, name);
>
> In this case you'll find it simpler to use the virAsprintf() function
> which is available from src/util.h. This will automatically allocate an
> char * buffer large enough for the data your printf'ing. I think you
> probably also want to make sure you quote the string args in yuour
> SSH commands, and possibly also worry about escaping shell magic
> characters.
>
> > + ret = __inner_exec_command(ssh_session, cmd, textual_return);
>
> I'd also recommend against passing in a fixed allocated buffer
> 'char *textual_return' here. Instead, use the 'virBuffer' object
> (from src/buf.h) which implements grow-on-demand buffer with
> strong error checking for OOM.
And this was a problem I was going to ask your comments. I really need
to fix this. Thanks for the suggestion. This should be fine this way.
>
> > +
> > + if (ret) {
> > + CLEANUP;
> > + return LPAR_EXEC_ERR;
> > + }
> > +
> > + for (i = 0; textual_return[i] != '\n'; i++)
> > + id_c[i] = textual_return[i];
> > + id_c[i] = '\0';
> > +
> > + CLEANUP;
> > + return atoi(id_c);
> > +}
> > +
> > +static int
> > +phypGetLparNAME(SSH_SESSION * ssh_session, const char *managed_system,
> > + unsigned int lpar_id, char *lpar_name)
> > +{
> > + unsigned int i = 0;
> > + int ret = 0;
> > + char *cmd;
> > + char *textual_return;
> > +
> > + if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(managed_system))) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + sprintf(cmd,
> > + "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F name",
> > + managed_system, lpar_id);
> > + ret = __inner_exec_command(ssh_session, cmd, textual_return);
> > +
> > + if (ret) {
> > + CLEANUP;
> > + lpar_name = NULL;
> > + return LPAR_EXEC_ERR;
> > + }
> > +
> > + for (i = 0; textual_return[i] != '\n'; i++)
> > + lpar_name[i] = textual_return[i];
> > + lpar_name[i] = '\0';
> > +
> > + CLEANUP;
> > + return ret;
> > +}
> > +
> > +static int
> > +phypGetLparUUID(SSH_SESSION * ssh_session, const char *managed_system,
> > + unsigned int lpar_id, unsigned char *lpar_uuid)
> > +{
> > + unsigned int i = 0;
> > + int ret = 0;
> > + char *cmd;
> > + char *textual_return;
> > +
> > + if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(managed_system))) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + sprintf(cmd,
> > + "lssyscfg -r lpar -m %s --filter lpar_ids=%d -F logical_serial_num",
> > + managed_system, lpar_id);
> > + ret = __inner_exec_command(ssh_session, cmd, textual_return);
> > +
> > + if (ret) {
> > + CLEANUP;
> > + lpar_uuid = NULL;
> > + return LPAR_EXEC_ERR;
> > + }
> > +
> > + for (i = 0; textual_return[i] != '\n'; i++)
> > + lpar_uuid[i] = textual_return[i];
> > + lpar_uuid[i] = '\0';
> > +
> > + CLEANUP;
> > + return ret;
> > +}
> > +
> > +static int
> > +phypNumDomains(virConnectPtr conn)
> > +{
> > + SSH_SESSION *ssh_session = conn->privateData;
> > + const char *managed_system = conn->uri->server;
> > + int ret = 0;
> > + unsigned int i = 0, ndom = 1;
> > + char *cmd;
> > + char *textual_return;
> > +
> > + if (VIR_ALLOC_N(cmd, MAXSIZE+sizeof(managed_system))) {
> > + return PHYP_NO_MEM;
> > + }
> > +
> > + if (VIR_ALLOC_N(textual_return, MAXSIZE)) {
> > + return PHYP_NO_MEM;
> > + }
>
> For errors that occur in driver APIs, you can't return an error code
> in this way. See the 'src/libvirt.c' docs for description of the error
> code for method - it is usually '-1' or 'NULL' as appropriate.
>
> To give more detail to the caller, you can use one of the error reporting
> functions from src/virterror_internal.h
Understood, I'll check src/virterror_internal.h to see how I need to do
things in the right way.
I'll fix all those things and implement some more features I have in
mind. I'll post the next patch soon. Thanks everyone for the comments.
[]'s
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
otubo at linux.vnet.ibm.com
More information about the libvir-list
mailing list