Hi Andrey,

I just have seen your http://www.dreyou.org/ovirt/vdsm.patch for
ovirt-3.2. I would like to thank you for your packaging and engineering
work. I'd like to incorporate as much of it into vdsm's upstream so that
it runs properly on any EL6 out-of-the-box.

Could you help me understand some of your changes?

I've posted http://gerrit.ovirt.org/#/c/10893/ which is taken from a
former patchset of yours - is it intentionally missing form the 3.2
version of your patchset?

> From adf7dc96767783ab81993504267c3cfd65b4c1bb Mon Sep 17 00:00:00 2001
> From: Andrey Gordeev <dre...@gmail.com>
> Date: Fri, 7 Dec 2012 13:12:19 +0400
> Subject: [PATCH 1/2] CentOS 6.2 changes
> 
> ---
>  build-aux/pkg-version |    6 +++++-
>  vds_bootstrap/setup   |    2 +-
>  vdsm.spec.in          |   30 ++++++++++++++++++++++++++----
>  vdsm/configNetwork.py |    5 +++++
>  vdsm/storage/misc.py  |    2 +-
>  vdsm/vdsmd.init.in    |    3 ++-
>  6 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/build-aux/pkg-version b/build-aux/pkg-version
> index 346ad23..9c410ff 100755
> --- a/build-aux/pkg-version
> +++ b/build-aux/pkg-version
> @@ -32,7 +32,11 @@ if test "x$1" = "x--full"; then
>  elif test "x$1" = "x--version"; then
>      echo $PKG_VERSION | awk "$AWK_VERSION" | tr -cd '[:alnum:].'
>  elif test "x$1" = "x--release"; then
> -    echo $PKG_VERSION | awk "$AWK_RELEASE" | tr -cd '[:alnum:].'
> +    if [ $LOCAL_BUILDID ]; then
> +       echo $PKG_VERSION | awk "$AWK_RELEASE" | sed 
> "s/git.*$/{$LOCAL_BUILDID}/" | tr -cd '[:alnum:].'
> +    else
> +       echo $PKG_VERSION | awk "$AWK_RELEASE" | tr -cd '[:alnum:].'
> +    fi
>  else
>      echo "usage: $0 [--full|--version|--release]"
>      exit 1
> diff --git a/vds_bootstrap/setup b/vds_bootstrap/setup
> index 01306bd..177e089 100755
> --- a/vds_bootstrap/setup
> +++ b/vds_bootstrap/setup
> @@ -29,7 +29,7 @@ import re
>  import tempfile
>  from time import strftime
>  
> -SUPPORTED_PLATFORMS = [ "RedHatEnterpriseServer", "Fedora" ]
> +SUPPORTED_PLATFORMS = [ "RedHatEnterpriseServer", "Fedora", "CentOS", 
> "Scientific", "Oracle" ]

I see no reason not to take this upstream

>  HYPERVISOR_PLATFORMS = [ "RedHatEnterpriseVirtualizationHypervisor", 
> "RedHatEnterpriseHypervisor", "oVirtNodeHypervisor" ]
>  HYPERVISOR_RELEASE_FILE = '/etc/rhev-hypervisor-release'
>  REDHAT_RELEASE_FILE = '/etc/redhat-release'
> diff --git a/vdsm.spec.in b/vdsm.spec.in
> index 465d548..a9de23d 100644
> --- a/vdsm.spec.in
> +++ b/vdsm.spec.in
> @@ -102,8 +102,10 @@ Requires: iscsi-initiator-utils >= 6.2.0.872-15
>  Requires: device-mapper-multipath >= 0.4.9-52
>  Requires: e2fsprogs >= 1.41.12-11
>  Requires: kernel >= 2.6.32-279.9.1
> -Requires: sanlock >= 2.3-4, sanlock-python
> -Requires: initscripts >= 9.03.31-2.el6_3.1
> +#Requires: sanlock >= 2.3-4, sanlock-python
> +#Requires: initscripts >= 9.03.31-2.el6_3.1

These requirements are for real bugs. Better include them into your EL6 flavor
or build yourself.

> +Requires: sanlock >= 2.3-1, sanlock-python
> +Requires: initscripts >= 9.03.31-2
>  Requires: mom >= 0.3.0
>  Requires: selinux-policy-targeted >= 3.7.19-80
>  Requires: lvm2 >= 2.02.95-10.el6_3.2
> @@ -431,6 +433,8 @@ install -Dm 0644 vdsm/limits.conf \
>  # Install the SysV init scripts
>  install -Dm 0755 vdsm/vdsmd.init %{buildroot}%{_initrddir}/vdsmd
>  install -Dm 0755 vdsm_reg/vdsm-reg.init %{buildroot}%{_initrddir}/vdsm-reg
> +install -Dm 0644 vdsm/vdsm.conf.sample \
> +                 %{buildroot}%{_sysconfdir}/vdsm/vdsm.conf

Federico, do you recall why don't we ship vdsm.conf on el?

>  
>  # This is not commonplace, but we want /var/log/core to be a world-writable
>  # dropbox for core dumps
> @@ -475,6 +479,15 @@ export LC_ALL=C
>  /usr/sbin/usermod -a -G %{qemu_group},%{vdsm_group} %{snlk_user}
>  
>  %post
> +if [ -f /etc/sysconfig/modules/softdog.modules ]
> +then
> +    cp /etc/sysconfig/modules/softdog.modules 
> /etc/vdsm/softdog.modules.prevdsm
> +fi
> +echo -e '#!/bin/sh\nmodprobe softdog\nexit 0' > 
> /etc/sysconfig/modules/softdog.modules
> +chmod +x /etc/sysconfig/modules/softdog.modules
> +/sbin/chkconfig wdmd on
> +/sbin/chkconfig sanlock on
> +

Andrey/Federico: why is this hack required?

>  %{_bindir}/vdsm-tool sebool-config
>  # set the vdsm "secret" password for libvirt
>  %{_bindir}/vdsm-tool set-saslpasswd
> @@ -538,6 +551,15 @@ exit 0
>  %endif
>  
>  %postun
> +if [ -f /etc/sysconfig/modules/softdog.modules ]
> +then
> +    cp /etc/sysconfig/modules/softdog.modules 
> /etc/vdsm/softdog.modules.prevdsm
> +fi
> +echo -e '#!/bin/sh\nmodprobe softdog\nexit 0' > 
> /etc/sysconfig/modules/softdog.modules
> +chmod +x /etc/sysconfig/modules/softdog.modules
> +/sbin/chkconfig wdmd on
> +/sbin/chkconfig sanlock on
> +
>  %if 0%{?rhel}
>  if [ "$1" -ge 1 ]; then
>      /sbin/service vdsmd condrestart > /dev/null 2>&1
> @@ -762,9 +784,9 @@ exit 0
>  %files python
>  %defattr(-, root, root, -)
>  %{_bindir}/vdsm-tool
> -%if !0%{?rhel}
> +#%if !0%{?rhel}
>  %config(noreplace) %{_sysconfdir}/%{vdsm_name}/vdsm.conf
> -%endif
> +#%endif
>  %{python_sitearch}/%{vdsm_name}/__init__.py*
>  %{python_sitearch}/%{vdsm_name}/config.py*
>  %{python_sitearch}/%{vdsm_name}/constants.py*
> diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py
> index 78fd3af..7bf6af7 100755
> --- a/vdsm/configNetwork.py
> +++ b/vdsm/configNetwork.py
> @@ -890,6 +890,11 @@ def addNetwork(network, vlan=None, bonding=None, 
> nics=None, ipaddr=None,
>      _netinfo = netinfo.NetInfo()
>      bridged = utils.tobool(bridged)
>  
> +    # Hack here, netmask may be not defined, if this happen,
> +    # set netmask to 255.255.255.0
> +    if not netmask:
> +        netmask = "255.255.255.0"
> +

When is this needed? Could it be related to PREFIX instead of NETMASK
issue of http://gerrit.ovirt.org/9322 ?

>      if mtu:
>          mtu = int(mtu)
>  
> diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py
> index 17d38ee..ada3196 100644
> --- a/vdsm/storage/misc.py
> +++ b/vdsm/storage/misc.py
> @@ -748,7 +748,7 @@ class RollbackContext(object):
>  
>          # re-raise the earliest exception
>          if firstException is not None:
> -            raise firstException, None, traceback
> +            raise firstException(None, traceback)

I do not believe that it is correct - is it only to satisfy old pep8
tool not recognizing the peculiar "raise" syntax that is used here?
Could you ship
http://danken.fedorapeople.org/python-pep8-1.3.3-3.el6.noarch.rpm
instead?

>  
>      def defer(self, func, *args, **kwargs):
>          self._finally.append((func, args, kwargs))
> diff --git a/vdsm/vdsmd.init.in b/vdsm/vdsmd.init.in
> index 7c709a6..0fd206e 100755
> --- a/vdsm/vdsmd.init.in
> +++ b/vdsm/vdsmd.init.in
> @@ -136,7 +136,8 @@ shutdown_conflicting_srv() {
>  }
>  
>  libvirt_should_use_upstart() {
> -    [[ -x /sbin/initctl ]]
> +#    [[ -x /sbin/initctl ]]
> +    [[ -x /sbin/foo ]]

Why is that? Don't you want to use upstart's libvirtd autostart after
crash?

>  }
>  
>  start_needed_srv() {
> -- 
> 1.7.1
> 
> 
> From 6f7ea3241047f06a196c90549960460c174362fd Mon Sep 17 00:00:00 2001
> From: Andrey Gordeev <dre...@gmail.com>
> Date: Tue, 11 Dec 2012 11:47:33 +0400
> Subject: [PATCH 2/2] Reset SecureXMLRPCServer.py due to M2Crypto errors

This worries me quite a bit. Which M2Crypto errors are you referring to?
We may need to revert http://gerrit.ovirt.org/8123 - but it has
performance improvement that is a shame to miss.

> 
> ---
>  vdsm/SecureXMLRPCServer.py |   92 
> +++++++-------------------------------------
>  1 files changed, 14 insertions(+), 78 deletions(-)
> 
> diff --git a/vdsm/SecureXMLRPCServer.py b/vdsm/SecureXMLRPCServer.py
> index 2de1cf7..9396a28 100644
> --- a/vdsm/SecureXMLRPCServer.py
> +++ b/vdsm/SecureXMLRPCServer.py
> @@ -34,86 +34,10 @@ import ssl
>  import httplib
>  import socket
>  import SocketServer
> -import logging
> -
> -from M2Crypto import SSL, X509
>  
>  SecureXMLRPCRequestHandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler
>  
>  
> -class SSLSocket(object):
> -    """SSL decorator for sockets.
> -
> -    This class wraps a socket returned by the accept method of a
> -    server socket providing the SSL socket methods that are missing in
> -    the connection class. The rest of the methods are just delegated.
> -    """
> -
> -    def __init__(self, connection):
> -        # Save the reference to the connection so that we can delegate
> -        # calls to it later:
> -        self.connection = connection
> -
> -    def gettimeout(self):
> -        return self.connection.socket.gettimeout()
> -
> -    def __getattr__(self, name):
> -        # This is how we delegate all the rest of the methods to the
> -        # underlying SSL connection:
> -        return getattr(self.connection, name)
> -
> -
> -class SSLServerSocket(SSLSocket):
> -    """SSL decorator for server sockets.
> -
> -    This class wraps a normal socket so that when the accept method is
> -    called the accepted socket is also decorated.
> -    """
> -
> -    def __init__(self, raw, certfile=None, keyfile=None, ca_certs=None,
> -                 session_id="vdsm", protocol="sslv23"):
> -        # Create the SSL context:
> -        self.context = SSL.Context(protocol)
> -        self.context.set_session_id_ctx(session_id)
> -
> -        # Load the server certificate and key files:
> -        if certfile and keyfile:
> -            self.context.load_cert_chain(certfile, keyfile)
> -
> -        def verify(context, certificate, error, depth, result):
> -            # The validation of the client certificate has already been
> -            # performed by the OpenSSL library and the handhake already 
> aborted
> -            # if it fails as we use the verify_fail_if_no_peer_cert mode. We
> -            # are not doing any additional validation, so we just need to log
> -            # it and return the same result.
> -            if not result:
> -                certificate = X509.X509(certificate)
> -                logging.error(
> -                    "invalid client certificate with subject \"%s\"",
> -                    certificate.get_subject())
> -            return result
> -
> -        # Load the certificates of the CAs used to verify client
> -        # connections:
> -        if ca_certs:
> -            self.context.load_verify_locations(ca_certs)
> -            self.context.set_verify(
> -                mode=SSL.verify_peer | SSL.verify_fail_if_no_peer_cert,
> -                depth=10,
> -                callback=verify)
> -
> -        # Create the SSL connection:
> -        self.connection = SSL.Connection(self.context, sock=raw)
> -
> -    def accept(self):
> -        # The SSL connection already returns a SSL prepared socket, but it
> -        # misses some of the methods that the XML PRC server uses, so we need
> -        # to wrap it as well:
> -        client, address = self.connection.accept()
> -        client = SSLSocket(client)
> -        return client, address
> -
> -
>  class SecureXMLRPCServer(SimpleXMLRPCServer.SimpleXMLRPCServer):
>      def __init__(self, addr,
>                   
> requestHandler=SimpleXMLRPCServer.SimpleXMLRPCRequestHandler,
> @@ -129,15 +53,27 @@ class 
> SecureXMLRPCServer(SimpleXMLRPCServer.SimpleXMLRPCServer):
>                   requestHandler,
>                   logRequests, allow_none, encoding,
>                   bind_and_activate=False)
> -        self.socket = SSLServerSocket(raw=self.socket, certfile=certfile,
> -                                      keyfile=keyfile, ca_certs=ca_certs)
> +        self.socket = ssl.wrap_socket(self.socket,
> +                 keyfile=keyfile, certfile=certfile,
> +                 ca_certs=ca_certs, server_side=True,
> +                 cert_reqs=ssl.CERT_REQUIRED,
> +                 do_handshake_on_connect=False)
>          if timeout is not None:
>              self.socket.settimeout = timeout
>          if bind_and_activate:
>              self.server_bind()
>              self.server_activate()
>  
> +    def finish_request(self, request, client_address):
> +        request.do_handshake()
> +
> +        return SimpleXMLRPCServer.SimpleXMLRPCServer.finish_request(
> +                                                             self,
> +                                                             request,
> +                                                             client_address)
> +
>      def handle_error(self, request, client_address):
> +        import logging
>          logging.error('client %s', client_address, exc_info=True)
>  
>  
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel

Reply via email to