Merge pull request #11486 from chef/lcg/yum-fix-blocking-and-flushing

5 files changed, 119 insertions, 105 deletions

diff --git a/cspell.json b/cspell.json
index 61c8ff5ec1..489a49e9a0 100644
--- a/cspell.json
+++ b/cspell.json
@@ -14,12 +14,6 @@
   "dictionaries": ["chef"],
   // words - list of words to be always considered correct
   "words": [
-    "aliyun",
-    "netbw",
-    "alibase",
-    "THREADSIZE",
-    "eipv",
-    "passwordprompt",
     "abcz",
     "Abdulin",
     "ABORTIFHUNG",
@@ -42,6 +36,8 @@
     "albertomurillo",
     "Albertson",
     "Algorta",
+    "alibase",
+    "aliyun",
     "Alloc",
     "allowlist",
     "allowlisted",
@@ -68,9 +64,8 @@
     "ARPHELPLINK",
     "ARPPRODUCTICON",
     "arry",
-    "Arțăriși",
     "artem",
-    "Ásgeirsson",
+    "Arțăriși",
     "Ashwini",
     "ASSIGNPRIMARYTOKEN",
     "astoltz",
@@ -108,8 +103,8 @@
     "BACKOFFICE",
     "backport",
     "backported",
-    "backporting",
     "Backporting",
+    "backporting",
     "backslashing",
     "backtrace",
     "backtraces",
@@ -156,8 +151,8 @@
     "bufptr",
     "bufsize",
     "bugfix",
-    "bugfixes",
     "Bugfixes",
+    "bugfixes",
     "bugfixing",
     "bugok",
     "Buildkite",
@@ -220,8 +215,8 @@
     "chgrpmem",
     "chilcote",
     "CHINESEBIG",
-    "chkconfig",
     "CHKCONFIG",
+    "chkconfig",
     "Chouhan",
     "ckbk",
     "cksum",
@@ -236,8 +231,8 @@
     "CLOEXEC",
     "Cloke",
     "cmdlets",
-    "cmds",
     "CMDS",
+    "cmds",
     "Cmnd",
     "CMPL",
     "cname",
@@ -256,8 +251,8 @@
     "computerlevel",
     "COMPUTERNAME",
     "computersystem",
-    "comspec",
     "COMSPEC",
+    "comspec",
     "concat",
     "Cond",
     "confd",
@@ -277,8 +272,8 @@
     "convertto",
     "cookbookname",
     "cookiecurse",
-    "cookstyle",
     "Cookstyle",
+    "cookstyle",
     "coookbook",
     "copypasta",
     "coreutils",
@@ -309,8 +304,8 @@
     "Cxxx",
     "dacl",
     "Daemonization",
-    "daemonizing",
     "Daemonizing",
+    "daemonizing",
     "databag",
     "databags",
     "Datacenter",
@@ -349,7 +344,6 @@
     "depsolved",
     "Depsolving",
     "derekgroh",
-    "Désarmes",
     "DESCR",
     "deserialization",
     "destdir",
@@ -378,8 +372,8 @@
     "disablerepo",
     "DISCARDABLE",
     "DISCARDNS",
-    "dism",
     "DISM",
+    "dism",
     "displayname",
     "distro",
     "distros",
@@ -391,8 +385,8 @@
     "dockerenv",
     "dockerignore",
     "dockerinit",
-    "dokken",
     "Dokken",
+    "dokken",
     "domainandname",
     "domainname",
     "domainuser",
@@ -423,6 +417,7 @@
     "DWORDLONG",
     "DYNALINK",
     "DYNLINK",
+    "Désarmes",
     "Eachern",
     "EASTEUROPE",
     "EBUSY",
@@ -430,6 +425,7 @@
     "ecparam",
     "edir",
     "egid",
+    "eipv",
     "elif",
     "ELOOP",
     "EMBEDDEDBIN",
@@ -440,8 +436,8 @@
     "enablement",
     "enablerepo",
     "encap",
-    "encryptor",
     "Encryptor",
+    "encryptor",
     "endlocal",
     "ENOENT",
     "entriesread",
@@ -463,10 +459,10 @@
     "ESRCH",
     "etag",
     "etags",
-    "etcd",
     "Etcd",
-    "ethtool",
+    "etcd",
     "ETHTOOL",
+    "ethtool",
     "ETIMEDOUT",
     "euca",
     "Eugen",
@@ -484,8 +480,8 @@
     "EXTGLOB",
     "extname",
     "extrastuff",
-    "exts",
     "Exts",
+    "exts",
     "FACs",
     "failburger",
     "FAILCRITICALERRORS",
@@ -501,9 +497,10 @@
     "FASTZIPDIR",
     "fatals",
     "fauxhai",
+    "fcntl",
     "featurename",
-    "fflags",
     "FFLAGS",
+    "fflags",
     "Fichter",
     "fieldname",
     "Fileexists",
@@ -545,8 +542,8 @@
     "FULLSCREEN",
     "fuzzifier",
     "fuzzify",
-    "fuzzyurl",
     "Fuzzyurl",
+    "fuzzyurl",
     "fzipi",
     "gaffneyc",
     "gecos",
@@ -557,6 +554,7 @@
     "getc",
     "getchef",
     "GETFD",
+    "GETFL",
     "getgrgid",
     "getgrnam",
     "gethostbyname",
@@ -652,13 +650,13 @@
     "HMODULE",
     "HMONITOR",
     "HOMEDRIVE",
-    "homepath",
     "HOMEPATH",
+    "homepath",
     "HOMESHARE",
     "Hongli",
     "hostnamectl",
-    "hostnames",
     "Hostnames",
+    "hostnames",
     "hostport",
     "hostspec",
     "hostspecific",
@@ -696,8 +694,8 @@
     "includedir",
     "includepkgs",
     "includer",
-    "indentable",
     "Indentable",
+    "indentable",
     "indentt",
     "inet",
     "infile",
@@ -730,12 +728,12 @@
     "invokereturnasis",
     "Ionuț",
     "IOPL",
-    "ipaddr",
     "IPADDR",
+    "ipaddr",
     "ipaddress",
     "Ippolito",
-    "ipscopes",
     "Ipscopes",
+    "ipscopes",
     "ipsec",
     "iptables",
     "Ireton",
@@ -771,8 +769,8 @@
     "jxvf",
     "katello",
     "Kauppila",
-    "kaustubh",
     "Kaustubh",
+    "kaustubh",
     "Keane",
     "keepalive",
     "keepalives",
@@ -832,8 +830,8 @@
     "LIBPATH",
     "libtool",
     "libvirt",
-    "libxml",
     "Libxml",
+    "libxml",
     "libxslt",
     "libyaml",
     "lifecycle",
@@ -914,8 +912,8 @@
     "Madsen",
     "magick",
     "mailservers",
-    "mailslot",
     "MAILSLOT",
+    "mailslot",
     "mailto",
     "mainloop",
     "Mainmodule",
@@ -947,8 +945,8 @@
     "merlinjim",
     "MESSAGEDEST",
     "MESSAGENAME",
-    "metafile",
     "METAFILE",
+    "metafile",
     "metalink",
     "metasearch",
     "Miah",
@@ -966,14 +964,14 @@
     "mirrorlist",
     "mirrorlists",
     "MITM",
-    "mixins",
     "Mixins",
+    "mixins",
     "mixlib",
     "mkdir",
     "mkgroup",
     "mkmf",
-    "mktemp",
     "MKTEMP",
+    "mktemp",
     "mktmpdir",
     "mname",
     "mntfs",
@@ -1005,13 +1003,13 @@
     "msys",
     "MSYSTEM",
     "Mtyj",
-    "Multicast",
     "MULTICAST",
+    "Multicast",
     "multiline",
     "multipackage",
     "multiplatform",
-    "multiresource",
     "Multiresource",
+    "multiresource",
     "multitenant",
     "multithreaded",
     "Multiuser",
@@ -1024,8 +1022,8 @@
     "Mware",
     "myapp",
     "mycert",
-    "mycook",
     "Mycook",
+    "mycook",
     "mycorp",
     "mydirectory",
     "myecrequest",
@@ -1059,6 +1057,7 @@
     "NETBINDDISABLE",
     "NETBINDENABLE",
     "NETBINDREMOVE",
+    "netbw",
     "NETCARD",
     "netdom",
     "NETLOGON",
@@ -1111,8 +1110,8 @@
     "nopasswd",
     "noprofile",
     "noprogressbar",
-    "norestart",
     "NORESTART",
+    "norestart",
     "nospace",
     "nospinner",
     "notapplicable",
@@ -1134,8 +1133,8 @@
     "objfs",
     "objs",
     "OEMCP",
-    "ohai",
     "Ohai",
+    "ohai",
     "Ojeda",
     "oldguard",
     "OLDLOGLOCATION",
@@ -1147,8 +1146,8 @@
     "oneshot",
     "onidle",
     "onlogon",
-    "onparent",
     "ONPARENT",
+    "onparent",
     "onstart",
     "OPLOCK",
     "OPTARG",
@@ -1180,6 +1179,7 @@
     "passwordage",
     "passwordless",
     "PASSWORDNAME",
+    "passwordprompt",
     "PATCHLEVEL",
     "pathed",
     "PATHEXT",
@@ -1236,8 +1236,8 @@
     "plutil",
     "PNAME",
     "Policybuilder",
-    "policyfile",
     "Policyfile",
+    "policyfile",
     "Policyfiles",
     "POLICYNAME",
     "popen",
@@ -1287,10 +1287,10 @@
     "PROTSEQ",
     "PROTSEQS",
     "proxified",
-    "proxifier",
     "Proxifier",
-    "psapi",
+    "proxifier",
     "PSAPI",
+    "psapi",
     "pschaumburg",
     "Pscx",
     "Psec",
@@ -1336,8 +1336,8 @@
     "rassoc",
     "rbag",
     "rbconfig",
-    "rbenv",
     "RBENV",
+    "rbenv",
     "rcscript",
     "rcvar",
     "rdoc",
@@ -1348,8 +1348,8 @@
     "realloc",
     "realname",
     "realpath",
-    "rebooter",
     "Rebooter",
+    "rebooter",
     "rebootnow",
     "rebuilddb",
     "reconfig",
@@ -1363,8 +1363,8 @@
     "REDIRECTOR",
     "Redistributable",
     "redownloading",
-    "reenable",
     "Reenable",
+    "reenable",
     "reenabled",
     "reenabling",
     "refcount",
@@ -1436,8 +1436,8 @@
     "runlock",
     "runpid",
     "runrun",
-    "runtimes",
     "Runtimes",
+    "runtimes",
     "RXACT",
     "rxhash",
     "rxvlan",
@@ -1479,6 +1479,7 @@
     "SETCOUNT",
     "setenv",
     "SETFD",
+    "SETFL",
     "setlocal",
     "SETMARK",
     "setobj",
@@ -1574,16 +1575,16 @@
     "STARTUPINFO",
     "stdcall",
     "stdlib",
-    "stepable",
     "Stepable",
+    "stepable",
     "stopsrc",
     "strace",
     "Stratton",
     "strftime",
     "stringio",
     "strptime",
-    "struct",
     "Struct",
+    "struct",
     "stubabble",
     "stubbable",
     "subclassable",
@@ -1621,16 +1622,16 @@
     "swapon",
     "swappable",
     "swappiness",
-    "symlink",
     "Symlink",
+    "symlink",
     "symlinked",
     "symlinking",
     "symlinks",
     "sync",
     "syntaxcache",
     "sysadminctl",
-    "sysconf",
     "Sysconf",
+    "sysconf",
     "sysconfig",
     "sysctl",
     "sysctld",
@@ -1640,8 +1641,8 @@
     "syslog",
     "sysread",
     "systctl",
-    "systemdrive",
     "SYSTEMDRIVE",
+    "systemdrive",
     "systemrestart",
     "SYSTEMROOT",
     "systemsetup",
@@ -1676,6 +1677,7 @@
     "Thom",
     "THRDS",
     "THREADID",
+    "THREADSIZE",
     "thumbsup",
     "tilesize",
     "Timberman",
@@ -1716,8 +1718,8 @@
     "udiff",
     "UHALF",
     "uids",
-    "uint",
     "UINT",
+    "uint",
     "ulong",
     "ULONGLONG",
     "UNAVAIL",
@@ -1732,11 +1734,11 @@
     "UNEXP",
     "UNEXPORTED",
     "unforked",
-    "unformatter",
     "Unformatter",
+    "unformatter",
     "unhold",
-    "unicast",
     "Unicast",
+    "unicast",
     "unignored",
     "uninst",
     "unintuitive",
@@ -1765,8 +1767,8 @@
     "untap",
     "untar",
     "untracked",
-    "untrusted",
     "Untrusted",
+    "untrusted",
     "unversioned",
     "UNWRITABLE",
     "upcase",
@@ -1860,8 +1862,8 @@
     "winevt",
     "Winmgmt",
     "winmgmts",
-    "winnt",
     "WINNT",
+    "winnt",
     "winprog",
     "WINVER",
     "WIXUI",
@@ -1896,7 +1898,8 @@
     "zolo",
     "zombiejs",
     "Zuazo",
-    "zypp"
+    "zypp",
+    "Ásgeirsson"
   ],
   // flagWords - list of words to be always considered incorrect
   // This is useful for offensive words and common spelling errors.
diff --git a/lib/chef/provider/package/yum.rb b/lib/chef/provider/package/yum.rb
index 76b9b15172..121083ea46 100644
--- a/lib/chef/provider/package/yum.rb
+++ b/lib/chef/provider/package/yum.rb
@@ -237,11 +237,8 @@ class Chef
           @installed_version[index]
         end
 
-        # cache flushing is accomplished by simply restarting the python helper.  this produces a roughly
-        # 15% hit to the runtime of installing/removing/upgrading packages.  correctly using multipackage
-        # array installs (and the multipackage cookbook) can produce 600% improvements in runtime.
         def flushcache
-          python_helper.restart
+          python_helper.close_rpmdb
         end
 
         def yum_binary
diff --git a/lib/chef/provider/package/yum/python_helper.rb b/lib/chef/provider/package/yum/python_helper.rb
index 7758383b95..86aaf7230a 100644
--- a/lib/chef/provider/package/yum/python_helper.rb
+++ b/lib/chef/provider/package/yum/python_helper.rb
@@ -51,6 +51,8 @@ class Chef
           end
 
           def start
+            # For some reason we have to force python to unbuffered here, and then force the input pipe back to line
+            # buffered in the python code.  XXX: I tried to remove this but hit more issues in the python side.
             ENV["PYTHONUNBUFFERED"] = "1"
             @inpipe, inpipe_write = IO.pipe
             outpipe_read, @outpipe = IO.pipe
@@ -81,6 +83,10 @@ class Chef
             start if stdin.nil?
           end
 
+          def close_rpmdb
+            query("close_rpmdb", {})
+          end
+
           def compare_versions(version1, version2)
             query("versioncompare", { "versions" => [version1, version2] }).to_i
           end
@@ -117,12 +123,12 @@ class Chef
             parameters = { "provides" => provides, "version" => version, "arch" => arch }
             repo_opts = options_params(options || {})
             parameters.merge!(repo_opts)
-            # XXX: for now we restart before and after every query with an enablerepo/disablerepo to clean the helpers internal state
-            restart unless repo_opts.empty?
+            # XXX: for now we close the rpmdb before and after every query with an enablerepo/disablerepo to clean the helpers internal state
+            close_rpmdb unless repo_opts.empty?
             query_output = query(action, parameters)
             version = parse_response(query_output.lines.last)
             Chef::Log.trace "parsed #{version} from python helper"
-            restart unless repo_opts.empty?
+            close_rpmdb unless repo_opts.empty?
             version
           end
 
@@ -159,7 +165,7 @@ class Chef
               outpipe.syswrite json + "\n"
               output = inpipe.sysread(4096).chomp
               Chef::Log.trace "got '#{output}' from python helper"
-              return output
+              output
             end
           end
 
@@ -209,7 +215,7 @@ class Chef
             ret
           rescue EOFError, Errno::EPIPE, Timeout::Error, Errno::ESRCH => e
             output = drain_fds
-            if ( max_retries -= 1 ) > 0
+            if ( max_retries -= 1 ) > 0 && !ENV["YUM_HELPER_NO_RETRIES"]
               unless output.empty?
                 Chef::Log.trace "discarding output on stderr/stdout from python helper: #{output}"
               end
diff --git a/lib/chef/provider/package/yum/yum_helper.py b/lib/chef/provider/package/yum/yum_helper.py
index 6a7b481f62..560839eb54 100644
--- a/lib/chef/provider/package/yum/yum_helper.py
+++ b/lib/chef/provider/package/yum/yum_helper.py
@@ -10,6 +10,8 @@ import sys
 import yum
 import signal
 import os
+import fcntl
+
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'simplejson'))
 try: import json
 except ImportError: import simplejson as json
@@ -32,15 +34,6 @@ except ImportError:
 if not hasattr(yum.packages.FakeRepository, 'compare_providers_priority'):
     yum.packages.FakeRepository.compare_providers_priority = 99
 
-base = None
-
-def get_base():
-    global base
-    if base is None:
-        base = yum.YumBase()
-    setup_exit_handler()
-    return base
-
 def versioncompare(versions):
     arch_list = getArchList()
     candidate_arch1 = versions[0].split(".")[-1]
@@ -51,9 +44,9 @@ def versioncompare(versions):
     # then we'll chop the arch component (assuming it *is* a valid one) from the first version string
     # so we're only comparing the evr portions.
     if (candidate_arch2 not in arch_list) and (candidate_arch1 in arch_list):
-       final_version1 = versions[0].replace("." + candidate_arch1,"")
+        final_version1 = versions[0].replace("." + candidate_arch1,"")
     else:
-       final_version1 = versions[0]
+        final_version1 = versions[0]
 
     final_version2 = versions[1]
 
@@ -64,12 +57,11 @@ def versioncompare(versions):
     outpipe.write("%(e)s\n" % { 'e': evr_comparison })
     outpipe.flush()
 
-def install_only_packages(name):
-    base = get_base()
+def install_only_packages(base, name):
     if name in base.conf.installonlypkgs:
-      outpipe.write('True')
+        outpipe.write('True')
     else:
-      outpipe.write('False')
+        outpipe.write('False')
     outpipe.flush()
 
 # python2.4 / centos5 compat
@@ -82,19 +74,17 @@ except NameError:
                 return True
         return False
 
-def query(command):
-    base = get_base()
-
+def query(base, command):
     enabled_repos = base.repos.listEnabled()
 
     # Handle any repocontrols passed in with our options
 
     if 'repos' in command:
-      for repo in command['repos']:
-        if 'enable' in repo:
-          base.repos.enableRepo(repo['enable'])
+        for repo in command['repos']:
+            if 'enable' in repo:
+                base.repos.enableRepo(repo['enable'])
         if 'disable' in repo:
-          base.repos.disableRepo(repo['disable'])
+            base.repos.disableRepo(repo['disable'])
 
     args = { 'name': command['provides'] }
     do_nevra = False
@@ -136,7 +126,7 @@ def query(command):
         #    returnPackages and searchProvides and then apply the Nevra filters to those results.
         pkgs = obj.searchNevra(**args)
         if (command['action'] == "whatinstalled") and (not pkgs):
-          pkgs = obj.searchNevra(name=args['name'], arch=desired_arch)
+            pkgs = obj.searchNevra(name=args['name'], arch=desired_arch)
     else:
         pats = [command['provides']]
         pkgs = obj.returnPackages(patterns=pats)
@@ -157,13 +147,13 @@ def query(command):
 
     # Reset any repos we were passed in enablerepo/disablerepo to the original state in enabled_repos
     if 'repos' in command:
-      for repo in command['repos']:
-        if 'enable' in repo:
-          if base.repos.getRepo(repo['enable']) not in enabled_repos:
-            base.repos.disableRepo(repo['enable'])
+        for repo in command['repos']:
+            if 'enable' in repo:
+                if base.repos.getRepo(repo['enable']) not in enabled_repos:
+                    base.repos.disableRepo(repo['enable'])
         if 'disable' in repo:
-          if base.repos.getRepo(repo['disable']) in enabled_repos:
-            base.repos.enableRepo(repo['disable'])
+            if base.repos.getRepo(repo['disable']) in enabled_repos:
+                base.repos.enableRepo(repo['disable'])
 
 # the design of this helper is that it should try to be 'brittle' and fail hard and exit in order
 # to keep process tables clean.  additional error handling should probably be added to the retry loop
@@ -179,21 +169,29 @@ def setup_exit_handler():
     signal.signal(signal.SIGPIPE, exit_handler)
     signal.signal(signal.SIGQUIT, exit_handler)
 
+def set_blocking(fd):
+    old_flags = fcntl.fcntl(fd, fcntl.F_GETFL)
+    fcntl.fcntl(fd, fcntl.F_SETFL, old_flags & ~os.O_NONBLOCK)
+
+base = None
+
 if len(sys.argv) < 3:
-  inpipe = sys.stdin
-  outpipe = sys.stdout
+    inpipe = sys.stdin
+    outpipe = sys.stdout
 else:
-  inpipe = os.fdopen(int(sys.argv[1]), "r")
-  outpipe = os.fdopen(int(sys.argv[2]), "w")
+    set_blocking(int(sys.argv[1]))
+    set_blocking(int(sys.argv[2]))
+    inpipe = os.fdopen(int(sys.argv[1]), "r")
+    outpipe = os.fdopen(int(sys.argv[2]), "w")
 
 try:
+    setup_exit_handler()
     while 1:
         # stop the process if the parent proc goes away
         ppid = os.getppid()
         if ppid == 1:
             raise RuntimeError("orphaned")
 
-        setup_exit_handler()
         line = inpipe.readline()
 
         # only way to detect EOF in python
@@ -205,14 +203,22 @@ try:
         except ValueError, e:
             raise RuntimeError("bad json parse")
 
+        if base is None:
+            base = yum.YumBase()
+
         if command['action'] == "whatinstalled":
-            query(command)
+            query(base, command)
         elif command['action'] == "whatavailable":
-            query(command)
+            query(base, command)
         elif command['action'] == "versioncompare":
             versioncompare(command['versions'])
         elif command['action'] == "installonlypkgs":
-             install_only_packages(command['package'])
+            install_only_packages(base, command['package'])
+        elif command['action'] == "close_rpmdb":
+            base.closeRpmDB()
+            base = None
+            outpipe.write('nil nil nil\n')
+            outpipe.flush()
         else:
             raise RuntimeError("bad command")
 finally:
diff --git a/spec/functional/resource/yum_package_spec.rb b/spec/functional/resource/yum_package_spec.rb
index 777db6ff2b..2b08736d5b 100644
--- a/spec/functional/resource/yum_package_spec.rb
+++ b/spec/functional/resource/yum_package_spec.rb
@@ -48,6 +48,8 @@ describe Chef::Resource::YumPackage, :requires_root, external: exclude_test do
   end
 
   before(:each) do
+    # force errors to fail and not retry
+    ENV["YUM_HELPER_NO_RETRIES"] = "true"
     File.open("/etc/yum.repos.d/chef-yum-localtesting.repo", "w+") do |f|
       f.write <<~EOF
         [chef-yum-localtesting]