On 01/13/2013 02:39 PM, Dan Kenigsberg wrote:
> On Sun, Jan 13, 2013 at 04:40:11PM +0400, Andrey Gordeev wrote:
>> Hi Dan.
>>
>>
>> On Fri, Jan 11, 2013 at 12:42 AM, Dan Kenigsberg <dan...@redhat.com> wrote:
>>
>>> 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?
>>>
>>>
>> Glad to help you.
>>
>>
>>
>>> 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.
>>>
>>>
>> Agreed with sanlock, I can build newest version from rhev rpms repo,
>> but initscripts packages has the different names with the same version in
>> different distributions i.e. 9.03.31-2.el6_3.1 - rhel
>> , 9.03.31-2.el6.centos.1 - centos
> 
> Too bad about the naming convention. However, it is important that
> initscripts bug https://bugzilla.redhat.com/854852 is solved in that
> release.
> 
>>
>>
>>
>>>> +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?
>>>
>>>
>> Really don't know, but in one of my installation on Intel Blade System, i
>> got error:
>>
>> *"VM testVm is down. Exit message: internal error Failed to open socket to
>> sanlock daemon: No such file or directory.* "
>>
>> I.e. softdog module not loaded, then I add this hack.
>>
>>
>>
>>>>  %{_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 ?
>>>
>>
>> This may be needed if you manually writing yours ifcfg files and not set
>> NETMASK string, in this case default netmask is 255.255.255.0, and this
>> check must be changed to:
>>
>> if ipaddr and not netmask:
>>          netmask = "255.255.255.0"
>>
>> to correct work with none or dhcp protocol.
> 
> Poring at the initscripts code, I do not see that this as a valid
> default. As far as I see, a static IP with no NETMASK or PREFIX is not
> valid, and I do not think that upstream vdsm should out-guess the admin
> intentions.
> 
>>
>>
>>>
>>>>      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?
>>>
>>>
>> Yes, I hope.
> 
> Thanks. I still hope that someone rebases EPEL's pep8 to something more
> modern and without the "raise" bug. Maybe iweller could help!
> 
>>
>>
>>
>>>>
>>>>      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?
>>>
>>>
>> Because libvirtd service ships as systemv scripts in centos (and in rhel
>> 6.3), and its not restarted when you restart vdsmd service.
> 
> I libvirtd does not need to restart when vdsmd is restarted - it is the
> other way around. If libvirtd crashes, it should restart itself (with
> the help of upstart, that's why we disable sysv for libvirt), and when
> vdsm notices this, it should restart itself.
> 
> The solution is distorted, but in my experience it works fine these
> days, and I recommend to use it (or fix it properly upstream).
> 
>>
>>
>>>>  }
>>>>
>>>>  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.
>>>
>>
>> Sorry about this string about M2Crypto errors, this was happen because
>> I misunderstood one message in one of internet forums :-).
>>
>>
>> Original problem was in strange timeout error when vm trying to migrate
>> from one host to another. I'm consulted with Juan Hernandez, but I'm can't
>> fully resolve this problem. I found the solution, but I'm not sure in it. I
>> override the "close" method in SSLSocket clas (SecureXMLRPCServer.py), in
>> this case time out error was gone. Here is this method:
>>
>>     def close(self):
>>         import socket
>>         self.connection.shutdown(socket.SHUT_RDWR)
>>         self.connection.close()
> 
> dreyou, any chance you can post things like this to our upstream gerrit?
> 
> Juan, could you look into this? It may well be a cause of realy
> regressions in 3.2.

I couldn't reproduce this in my environment (Fedora 17), but I think
there is no harm in adding it, and Andrey already tested it.

Andrey, I submitted the patch with you as author:

http://gerrit.ovirt.org/10972

I will appreciate if you can test and verify it.

-- 
Dirección Comercial: C/Jose Bardasano Baos, 9, Edif. Gorbea 3, planta
3ºD, 28016 Madrid, Spain
Inscrita en el Reg. Mercantil de Madrid – C.I.F. B82657941 - Red Hat S.L.
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel

Reply via email to