AMBARI-11419. Checks for package existence are too slow. (aonishuk)

ambari-agent/src/test/python/resource_management/TestPackageResource.py

@@ -16,6 +16,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 '''
 
+import sys
 from unittest import TestCase
 from mock.mock import patch, MagicMock, call
 
@@ -35,9 +36,9 @@ class TestPackageResource(TestCase):
     with Environment('/') as env:
       Package("some_package",
       )
-    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep '^some-package$'"),
-                                call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', '--allow-unauthenticated', '--assume-yes', 'install', 'some-package'], logoutput=False, sudo=True, env={'DEBIAN_FRONTEND': 'noninteractive'}),
-                                call(['/usr/bin/apt-get', 'update', '-qq'], logoutput=False, sudo=True)])
+    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep ^some-package$"),
+ call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', '--allow-unauthenticated', '--assume-yes', 'install', 'some-package'], logoutput=False, sudo=True, env={'DEBIAN_FRONTEND': 'noninteractive'}),
+ call(['/usr/bin/apt-get', 'update', '-qq'], logoutput=False, sudo=True)])
     
     shell_mock.assert_has_calls([call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', 
                                        '--allow-unauthenticated', '--assume-yes', 'install', 'some-package'], logoutput=False, sudo=True)])
@@ -50,8 +51,9 @@ class TestPackageResource(TestCase):
     with Environment('/') as env:
       Package("some_package",
       )
-    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep '^some-package$'"),
-                                call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', '--allow-unauthenticated', '--assume-yes', 'install', 'some-package'], logoutput=False, sudo=True, env={'DEBIAN_FRONTEND': 'noninteractive'})])
+    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep ^some-package$"),
+ call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', '--allow-unauthenticated', '--assume-yes', 'install', 'some-package'], logoutput=False, sudo=True, env={'DEBIAN_FRONTEND': 'noninteractive'})])
+
     
     self.assertEqual(shell_mock.call_count, 0, "shell.checked_call shouldn't be called")
 
@@ -59,19 +61,11 @@ class TestPackageResource(TestCase):
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'ubuntu')
   def test_action_install_regex_ubuntu(self, shell_mock, call_mock):
-    call_mock.side_effect = [(0, None),
-                             (0, "some-package1\nsome-package2"),
-                             (0, "Some text.\nStatus: install ok installed\nSome text"),
-                             (0, "Some text.\nStatus: not installed\nSome text"),
-                             (0, None)]
+    call_mock.side_effect = [(0, None)]
     with Environment('/') as env:
       Package("some_package.*",
       )
-    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep '^some-package.*$'"),
-                                call("apt-cache --names-only search '^some-package.*$' | awk '{print $1}'"),
-                                call("dpkg --status 'some-package1'"),
-                                call("dpkg --status 'some-package2'"),
-                                call(['/usr/bin/apt-get', '-q', '-o', 'Dpkg::Options::=--force-confdef', '--allow-unauthenticated', '--assume-yes', 'install', 'some-package.*'], logoutput=False, sudo=True, env={'DEBIAN_FRONTEND': 'noninteractive'})])
+    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep ^some-package.*$")])
     self.assertEqual(shell_mock.call_count, 0, "shell.checked_call shouldn't be called")
 
   @patch.object(shell, "call")
@@ -86,57 +80,56 @@ class TestPackageResource(TestCase):
     with Environment('/') as env:
       Package("some_package.*",
               )
-    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep '^some-package.*$'"),
-                                call("apt-cache --names-only search '^some-package.*$' | awk '{print $1}'"),
-                                call("dpkg --status 'some-package1'"),
-                                call("dpkg --status 'some-package2'")])
-    self.assertEqual(call_mock.call_count, 4, "Package should not be installed")
+    call_mock.assert_has_calls([call("dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep ^some-package.*$")])
+    self.assertEqual(call_mock.call_count, 1, "Package should not be installed")
     self.assertEqual(shell_mock.call_count, 0, "shell.checked_call shouldn't be called")
 
-  @patch.object(shell, "call")
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'redhat')
-  def test_action_install_rhel(self, shell_mock, call_mock):
-    call_mock.return_value= (1, None)
+  def test_action_install_rhel(self, shell_mock):
+    sys.modules['yum'] = MagicMock()
+    sys.modules['yum'].YumBase.return_value = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.return_value = [('some_packag',)]
     with Environment('/') as env:
       Package("some_package",
       )
-    call_mock.assert_called_with("installed_pkgs=`rpm -qa 'some_package'` ; [ ! -z \"$installed_pkgs\" ]")
+    self.assertTrue(sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.called)
     shell_mock.assert_called_with(['/usr/bin/yum', '-d', '0', '-e', '0', '-y', 'install', 'some_package'], logoutput=False, sudo=True)
 
-  @patch.object(shell, "call")
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'redhat')
-  def test_action_install_pattern_rhel(self, shell_mock, call_mock):
-    call_mock.side_effect=[(0, None), (1, "Some text")]
+  def test_action_install_pattern_rhel(self, shell_mock):
+    sys.modules['yum'] = MagicMock()
+    sys.modules['yum'].YumBase.return_value = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.return_value = [('some_packag',)]
     with Environment('/') as env:
       Package("some_package*",
       )
-    call_mock.assert_has_calls([call("installed_pkgs=`rpm -qa 'some_package*'` ; [ ! -z \"$installed_pkgs\" ]"),
-                                call("! yum list available 'some_package*'")])
     shell_mock.assert_called_with(['/usr/bin/yum', '-d', '0', '-e', '0', '-y', 'install', 'some_package*'], logoutput=False, sudo=True)
 
-  @patch.object(shell, "call")
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'redhat')
-  def test_action_install_pattern_installed_rhel(self, shell_mock, call_mock):
-    call_mock.side_effect=[(0, None), (0, "Some text")]
+  def test_action_install_pattern_installed_rhel(self, shell_mock):
+    sys.modules['yum'] = MagicMock()
+    sys.modules['yum'].YumBase.return_value = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.return_value = [('some_package_1_2_3',)]
     with Environment('/') as env:
       Package("some_package*",
       )
-    call_mock.assert_has_calls([call("installed_pkgs=`rpm -qa 'some_package*'` ; [ ! -z \"$installed_pkgs\" ]"),
-                                call("! yum list available 'some_package*'")])
     self.assertEqual(shell_mock.call_count, 0, "shell.checked_call shouldn't be called")
 
-  @patch.object(shell, "call")
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'suse')
-  def test_action_install_suse(self, shell_mock, call_mock):
-    call_mock.return_value= (1, None)
+  def test_action_install_suse(self, shell_mock):
+    sys.modules['rpm'] = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value.dbMatch.return_value = [{'name':'some_packages'}]
     with Environment('/') as env:
       Package("some_package",
       )
-    call_mock.assert_called_with("installed_pkgs=`rpm -qa 'some_package'` ; [ ! -z \"$installed_pkgs\" ]")
     shell_mock.assert_called_with(['/usr/bin/zypper', '--quiet', 'install', '--auto-agree-with-licenses', '--no-confirm', 'some_package'], logoutput=False, sudo=True)
 
   @patch.object(shell, "call")
@@ -151,22 +144,25 @@ class TestPackageResource(TestCase):
                                 call("zypper --non-interactive search --type package --uninstalled-only --match-exact 'some_package*'")])
     shell_mock.assert_called_with(['/usr/bin/zypper', '--quiet', 'install', '--auto-agree-with-licenses', '--no-confirm', 'some_package*'], logoutput=False, sudo=True)
 
-  @patch.object(shell, "call")
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'suse')
-  def test_action_install_pattern_suse(self, shell_mock, call_mock):
-    call_mock.side_effect=[(0, None), (0, "Loading repository data...\nReading installed packages...\nNo packages found.\n")]
+  def test_action_install_pattern_suse(self, shell_mock):
+    sys.modules['rpm'] = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value.dbMatch.return_value = [{'name':'some_packagetest'}]
     with Environment('/') as env:
       Package("some_package*",
               )
-    call_mock.assert_has_calls([call("installed_pkgs=`rpm -qa 'some_package*'` ; [ ! -z \"$installed_pkgs\" ]"),
-                                call("zypper --non-interactive search --type package --uninstalled-only --match-exact 'some_package*'")])
     self.assertEqual(shell_mock.call_count, 0, "shell.checked_call shouldn't be called")
 
   @patch.object(shell, "call", new = MagicMock(return_value=(0, None)))
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'redhat')
   def test_action_install_existent_rhel(self, shell_mock):
+    sys.modules['yum'] = MagicMock()
+    sys.modules['yum'].YumBase.return_value = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.return_value = [('some_package',)]
     with Environment('/') as env:
       Package("some_package",
               )
@@ -188,6 +184,9 @@ class TestPackageResource(TestCase):
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'suse')
   def test_action_install_existent_suse(self, shell_mock):
+    sys.modules['rpm'] = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value.dbMatch.return_value = [{'name':'some_package'}]
     with Environment('/') as env:
       Package("some_package",
               )
@@ -197,6 +196,10 @@ class TestPackageResource(TestCase):
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'redhat')
   def test_action_remove_rhel(self, shell_mock):
+    sys.modules['yum'] = MagicMock()
+    sys.modules['yum'].YumBase.return_value = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb = MagicMock()
+    sys.modules['yum'].YumBase.return_value.rpmdb.simplePkgList.return_value = [('some_package',)]
     with Environment('/') as env:
       Package("some_package",
               action = "remove"
@@ -207,6 +210,9 @@ class TestPackageResource(TestCase):
   @patch.object(shell, "checked_call")
   @patch.object(System, "os_family", new = 'suse')
   def test_action_remove_suse(self, shell_mock):
+    sys.modules['rpm'] = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value = MagicMock()
+    sys.modules['rpm'].TransactionSet.return_value.dbMatch.return_value = [{'name':'some_package'}]
     with Environment('/') as env:
       Package("some_package",
               action = "remove"

ambari-common/src/main/python/resource_management/core/providers/package/apt.py

@@ -22,6 +22,7 @@ Ambari Agent
 import os
 import tempfile
 import shutil
+import re
 
 from resource_management.core.providers.package import PackageProvider
 from resource_management.core import shell
@@ -39,15 +40,10 @@ REMOVE_CMD = {
 }
 REPO_UPDATE_CMD = ['/usr/bin/apt-get', 'update','-qq']
 
-CHECK_EXISTENCE_CMD = "dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep '^%s$'"
-GET_PACKAGES_BY_PATTERN_CMD = "apt-cache --names-only search '^%s$' | awk '{print $1}'"
-GET_PACKAGE_STATUS_CMD = "dpkg --status '%s'"
-
-PACKAGE_INSTALLED_STATUS = 'Status: install ok installed'
-
-EMPTY_FILE = "/dev/null"
 APT_SOURCES_LIST_DIR = "/etc/apt/sources.list.d"
 
+CHECK_CMD = "dpkg --get-selections | grep -v deinstall | awk '{print $1}' | grep ^%s$"
+
 def replace_underscores(function_to_decorate):
   def wrapper(*args):
     self = args[0]
@@ -60,7 +56,7 @@ class AptProvider(PackageProvider):
 
   @replace_underscores
   def install_package(self, name, use_repos=[]):
-    if not self._check_existence(name) or use_repos:
+    if use_repos or not self._check_existence(name):
       cmd = INSTALL_CMD[self.get_logoutput()]
       copied_sources_files = []
       is_tmp_dir_created = False
@@ -117,16 +113,24 @@ class AptProvider(PackageProvider):
       Logger.info("Skipping removal of non-existing package %s" % (name))
 
   @replace_underscores
-  def _check_existence(self, name):
-    code, out = shell.call(CHECK_EXISTENCE_CMD % name)
-    if bool(code):
-      return False
-    elif '*' in name or '.' in name:  # Check if all packages matching regexp are installed
-      code1, out1 = shell.call(GET_PACKAGES_BY_PATTERN_CMD % name)
-      for package_name in out1.splitlines():
-        code2, out2 = shell.call(GET_PACKAGE_STATUS_CMD % package_name)
-        if PACKAGE_INSTALLED_STATUS not in out2.splitlines():
-          return False
-      return True
-    else:
-      return True
+  def _check_existence(self, name): 
+    """
+    For regexp names:
+    If only part of packages were installed during early canceling.
+    Let's say:
+    1. install hbase-2-3-.*
+    2. Only hbase-2-3-1234 is installed, but is not hbase-2-3-1234-regionserver yet.
+    3. We cancel the apt-get
+    
+    In that case this is bug of packages we require.
+    And hbase-2-3-*-regionserver should be added to metainfo.xml.
+    
+    Checking existence should never fail in such a case for hbase-2-3-.*, otherwise it
+    gonna break things like removing packages and some other things.
+    
+    Note: this method SHOULD NOT use apt-get (apt.cache is using dpkg not apt). Because a lot of issues we have, when customer have
+    apt-get in inconsistant state (locked, used, having invalid repo). Once packages are installed
+    we should not rely on that.
+    """
+    code, out = shell.call(CHECK_CMD % name)
+    return not bool(code)

ambari-common/src/main/python/resource_management/core/providers/package/yumrpm.py

@@ -24,7 +24,9 @@ from resource_management.core.providers.package import PackageProvider
 from resource_management.core import shell
 from resource_management.core.shell import string_cmd_from_args_list
 from resource_management.core.logger import Logger
+from resource_management.core.utils import suppress_stdout
 import os
+import re
 
 INSTALL_CMD = {
   True: ['/usr/bin/yum', '-y', 'install'],
@@ -36,12 +38,9 @@ REMOVE_CMD = {
   False: ['/usr/bin/yum', '-d', '0', '-e', '0', '-y', 'erase'],
 }
 
-CHECK_CMD = "installed_pkgs=`rpm -qa '%s'` ; [ ! -z \"$installed_pkgs\" ]"
-CHECK_AVAILABLE_PACKAGES_CMD = "! yum list available '%s'"
-
 class YumProvider(PackageProvider):
   def install_package(self, name, use_repos=[]):
-    if not self._check_existence(name) or use_repos:
+    if use_repos or not self._check_existence(name):
       cmd = INSTALL_CMD[self.get_logoutput()]
       if use_repos:
         enable_repo_option = '--enablerepo=' + ",".join(use_repos)
@@ -64,13 +63,34 @@ class YumProvider(PackageProvider):
       Logger.info("Skipping removal of non-existing package %s" % (name))
 
   def _check_existence(self, name):
-    if '.' in name:  # To work with names like 'zookeeper_2_2_1_0_2072.noarch'
-      name = os.path.splitext(name)[0]
-    code, out = shell.call(CHECK_CMD % name)
-    if bool(code):
-      return False
-    elif '*' in name or '?' in name:  # Check if all packages matching pattern are installed
-      code1, out1 = shell.call(CHECK_AVAILABLE_PACKAGES_CMD % name)
-      return not bool(code1)
-    else:
-      return True
+    """
+    For regexp names:
+    If only part of packages were installed during early canceling.
+    Let's say:
+    1. install hbase_2_3_*
+    2. Only hbase_2_3_1234 is installed, but is not hbase_2_3_1234_regionserver yet.
+    3. We cancel the yum
+    
+    In that case this is bug of packages we require.
+    And hbase_2_3_*_regionserver should be added to metainfo.xml.
+    
+    Checking existence should never fail in such a case for hbase_2_3_*, otherwise it
+    gonna break things like removing packages and some others.
+    
+    Note: this method SHOULD NOT use yum directly (yum.rpmdb doesn't use it). Because a lot of issues we have, when customer have
+    yum in inconsistant state (locked, used, having invalid repo). Once packages are installed
+    we should not rely on that.
+    """
+    import yum # Python Yum API is much faster then other check methods. (even then "import rpm")
+    yb = yum.YumBase()
+    name_regex = re.escape(name).replace("\\?", ".").replace("\\*", ".*") + '$'
+    regex = re.compile(name_regex)
+    
+    with suppress_stdout():
+      package_list = yb.rpmdb.simplePkgList()
+    
+    for package in package_list:
+      if regex.match(package[0]):
+        return True
+    
+    return False

ambari-common/src/main/python/resource_management/core/providers/package/zypper.py

@@ -24,6 +24,9 @@ from resource_management.core.providers.package import PackageProvider
 from resource_management.core import shell
 from resource_management.core.shell import string_cmd_from_args_list
 from resource_management.core.logger import Logger
+from resource_management.core.utils import suppress_stdout
+
+import re
 
 INSTALL_CMD = {
   True: ['/usr/bin/zypper', 'install', '--auto-agree-with-licenses', '--no-confirm'],
@@ -33,10 +36,6 @@ REMOVE_CMD = {
   True: ['/usr/bin/zypper', 'remove', '--no-confirm'],
   False: ['/usr/bin/zypper', '--quiet', 'remove', '--no-confirm'],
 }
-CHECK_CMD = "installed_pkgs=`rpm -qa '%s'` ; [ ! -z \"$installed_pkgs\" ]"
-GET_NOT_INSTALLED_CMD = "zypper --non-interactive search --type package --uninstalled-only --match-exact '%s'"
-
-NO_PACKAGES_FOUND_STATUS = 'No packages found.'
 
 LIST_ACTIVE_REPOS_CMD = ['/usr/bin/zypper', 'repos']
 
@@ -55,7 +54,7 @@ def get_active_base_repos():
 
 class ZypperProvider(PackageProvider):
   def install_package(self, name, use_repos=[]):
-    if not self._check_existence(name) or use_repos:
+    if use_repos or not self._check_existence(name):
       cmd = INSTALL_CMD[self.get_logoutput()]
       if use_repos:
         active_base_repos = get_active_base_repos()
@@ -85,11 +84,32 @@ class ZypperProvider(PackageProvider):
       Logger.info("Skipping removal of non-existing package %s" % (name))
 
   def _check_existence(self, name):
-    code, out = shell.call(CHECK_CMD % name)
-    if bool(code):
-      return False
-    elif '*' in name or '?' in name:  # Check if all packages matching pattern are installed
-      code1, out1 = shell.call(GET_NOT_INSTALLED_CMD % name)
-      return NO_PACKAGES_FOUND_STATUS in out1.splitlines()
-    else:
-      return True
+    """
+    For regexp names:
+    If only part of packages were installed during early canceling.
+    Let's say:
+    1. install hbase_2_3_*
+    2. Only hbase_2_3_1234 is installed, but is not hbase_2_3_1234_regionserver yet.
+    3. We cancel the zypper
+    
+    In that case this is bug of packages we require.
+    And hbase_2_3_*_regionserver should be added to metainfo.xml.
+    
+    Checking existence should never fail in such a case for hbase_2_3_*, otherwise it
+    gonna break things like removing packages and some other things.
+    
+    Note: this method SHOULD NOT use zypper. Because a lot of issues we have, when customer have
+    zypper in inconsistant state (locked, used, having invalid repo). Once packages are installed
+    we should not rely on that.
+    """
+    import rpm # this is faster then calling 'rpm'-binary externally.
+    ts = rpm.TransactionSet()
+    packages = ts.dbMatch()
+    
+    name_regex = re.escape(name).replace("\\?", ".").replace("\\*", ".*") + '$'
+    regex = re.compile(name_regex)
+    
+    for package in packages:
+      if regex.match(package['name']):
+        return True
+    return False

ambari-common/src/main/python/resource_management/core/shell.py

@@ -170,7 +170,7 @@ def _call(command, logoutput=None, throw_on_failure=True,
       if ready:
         line = os.read(master_fd, 512)
         if not line:
-            break
+          continue
           
         out += line
         try:

ambari-common/src/main/python/resource_management/core/utils.py

@@ -20,6 +20,9 @@ Ambari Agent
 
 """
 
+import contextlib
+import sys
+import cStringIO
 from resource_management.core.exceptions import Fail
 
 class AttributeDictionary(object):
@@ -106,3 +109,10 @@ def checked_unite(dict1, dict2):
   result.update(dict2)
   
   return result
+
+@contextlib.contextmanager
+def suppress_stdout():
+  save_stdout = sys.stdout
+  sys.stdout = cStringIO.StringIO()
+  yield
+  sys.stdout = save_stdout