IMPALA-13396: Unify tmp dir management in CustomClusterTestSuite

There are many custom cluster tests that require creating temporary
directory. The temporary directory typically live within a scope of test
method and cleaned afterwards. However, some test do create temporary
directory directly and forgot to clean them afterwards, leaving junk
dirs under /tmp/ or $LOG_DIR.

This patch unify the temporary directory management inside
CustomClusterTestSuite. It introduce new 'tmp_dir_placeholders' arg in
CustomClusterTestSuite.with_args() that list tmp dirs to create.
'impalad_args', 'catalogd_args', and 'impala_log_dir' now accept
formatting pattern that is replaceable by a temporary dir path, defined
through 'tmp_dir_placeholders'.

There are few occurrences where mkdtemp is called and not replaceable by
this work, such as tests/comparison/cluster.py. In that case, this patch
change them to supply prefix arg so that developer knows that it comes
from Impala test script.

This patch also addressed several flake8 errors in modified files.

Testing:
- Pass custom cluster tests in exhaustive mode.
- Manually run few modified tests and observe that the temporary dirs
  are created and removed under logs/custom_cluster_tests/ as the tests
  go.

Change-Id: I8dd665e8028b3f03e5e33d572c5e188f85c3bdf5
Reviewed-on: http://gerrit.cloudera.org:8080/21836
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>

tests/authorization/test_authorization.py

@@ -18,17 +18,10 @@
 # Client tests for SQL statement authorization
 
 from __future__ import absolute_import, division, print_function
-import os
 import pytest
-import tempfile
-import grp
-import re
 import random
-import sys
-import subprocess
 import threading
 import time
-import urllib
 
 from getpass import getuser
 from ImpalaService import ImpalaHiveServer2Service
@@ -37,15 +30,13 @@
 from thrift.transport.TTransport import TBufferedTransport
 from thrift.protocol import TBinaryProtocol
 from tests.common.custom_cluster_test_suite import CustomClusterTestSuite
-from tests.common.file_utils import assert_file_in_dir_contains,\
-    assert_no_files_in_dir_contain
+from tests.common.file_utils import assert_file_in_dir_contains
 from tests.common.test_result_verifier import error_msg_equal
-from tests.common.skip import SkipIf
-
 
 PRIVILEGES = ['all', 'alter', 'drop', 'insert', 'refresh', 'select']
 ADMIN = "admin"
 
+
 class TestAuthorization(CustomClusterTestSuite):
   def setup(self):
     host, port = (self.cluster.impalads[0].service.hostname,
@@ -96,10 +87,9 @@ def __open_hs2(self, user, configuration, verify=True):
     return resp
 
   @pytest.mark.execute_serially
-  @CustomClusterTestSuite.with_args("--server_name=server1\
-      --authorization_policy_file=ignored_file",
-      impala_log_dir=tempfile.mkdtemp(prefix="test_deprecated_",
-      dir=os.getenv("LOG_DIR")))
+  @CustomClusterTestSuite.with_args(
+      "--server_name=server1 --authorization_policy_file=ignored_file",
+      impala_log_dir="{deprecated_flags}", tmp_dir_placeholders=['deprecated_flags'])
   def test_deprecated_flags(self):
     assert_file_in_dir_contains(self.impala_log_dir, "Ignoring removed flag "
                                                      "authorization_policy_file")

tests/authorization/test_authorized_proxy.py

@@ -18,39 +18,35 @@
 from __future__ import absolute_import, division, print_function
 import pytest
 import os
-import grp
 import time
 import json
-import tempfile
-import shutil
 
-from getpass import getuser
 from ImpalaService import ImpalaHiveServer2Service
 from TCLIService import TCLIService
 from thrift.transport.TSocket import TSocket
 from thrift.transport.TTransport import TBufferedTransport
 from thrift.protocol import TBinaryProtocol
 from tests.common.custom_cluster_test_suite import CustomClusterTestSuite
-from tests.common.skip import SkipIf
 from tests.hs2.hs2_test_suite import operation_id_to_query_id
 
-AUDIT_LOG_DIR = tempfile.mkdtemp(dir=os.getenv("LOG_DIR"))
-
-RANGER_IMPALAD_ARGS = "--server-name=server1 " \
-                      "--ranger_service_type=hive " \
-                      "--ranger_app_id=impala " \
-                      "--authorization_provider=ranger " \
-                      "--abort_on_failed_audit_event=false " \
-                      "--audit_event_log_dir={0}".format(AUDIT_LOG_DIR)
-RANGER_CATALOGD_ARGS = "--server-name=server1 " \
-                       "--ranger_service_type=hive " \
-                       "--ranger_app_id=impala " \
-                       "--authorization_provider=ranger"
+AUDIT_LOG_DIR = 'audit_log_dir'
+
+RANGER_IMPALAD_ARGS = ("--server-name=server1 "
+                       "--ranger_service_type=hive "
+                       "--ranger_app_id=impala "
+                       "--authorization_provider=ranger "
+                       "--abort_on_failed_audit_event=false "
+                       "--audit_event_log_dir={" + AUDIT_LOG_DIR + "}")
+RANGER_CATALOGD_ARGS = ("--server-name=server1 "
+                        "--ranger_service_type=hive "
+                        "--ranger_app_id=impala "
+                        "--authorization_provider=ranger")
 RANGER_ADMIN_USER = "admin"
 
 
 class TestAuthorizedProxy(CustomClusterTestSuite):
-  def setup(self):
+  def setup_method(self, method):
+    super(TestAuthorizedProxy, self).setup_method(method)
     host, port = (self.cluster.impalads[0].service.hostname,
                   self.cluster.impalads[0].service.hs2_port)
     self.socket = TSocket(host, port)
@@ -59,10 +55,10 @@ def setup(self):
     self.protocol = TBinaryProtocol.TBinaryProtocol(self.transport)
     self.hs2_client = ImpalaHiveServer2Service.Client(self.protocol)
 
-  def teardown(self):
+  def teardown_method(self, method):
+    super(TestAuthorizedProxy, self).teardown_method(method)
     if self.socket:
       self.socket.close()
-    shutil.rmtree(AUDIT_LOG_DIR, ignore_errors=True)
 
   def _execute_hs2_stmt(self, statement, verify=True):
     """
@@ -103,7 +99,8 @@ def _open_hs2(self, user, configuration, verify=True):
   @CustomClusterTestSuite.with_args(
     impalad_args="{0} --authorized_proxy_user_config=foo=bar;hue=non_owner "
                  .format(RANGER_IMPALAD_ARGS),
-    catalogd_args=RANGER_CATALOGD_ARGS)
+    catalogd_args=RANGER_CATALOGD_ARGS,
+    tmp_dir_placeholders=[AUDIT_LOG_DIR])
   def test_authorized_proxy_user_with_ranger(self):
     """Tests authorized proxy user with Ranger using HS2."""
     self._test_authorized_proxy_with_ranger(self._test_authorized_proxy, "non_owner",
@@ -115,7 +112,8 @@ def test_authorized_proxy_user_with_ranger(self):
                  "--authorized_proxy_group_config=foo=bar;hue=non_owner "
                  "--use_customized_user_groups_mapper_for_ranger"
                  .format(RANGER_IMPALAD_ARGS),
-    catalogd_args=RANGER_CATALOGD_ARGS)
+    catalogd_args=RANGER_CATALOGD_ARGS,
+    tmp_dir_placeholders=[AUDIT_LOG_DIR])
   def test_authorized_proxy_group_with_ranger(self):
     """Tests authorized proxy group with Ranger using HS2."""
     self._test_authorized_proxy_with_ranger(self._test_authorized_proxy, "non_owner",
@@ -125,7 +123,8 @@ def test_authorized_proxy_group_with_ranger(self):
   @CustomClusterTestSuite.with_args(
     impalad_args="{0} --authorized_proxy_user_config=foo=bar "
                  "--authorized_proxy_group_config=foo=bar".format(RANGER_IMPALAD_ARGS),
-    catalogd_args=RANGER_CATALOGD_ARGS)
+    catalogd_args=RANGER_CATALOGD_ARGS,
+    tmp_dir_placeholders=[AUDIT_LOG_DIR])
   def test_no_matching_user_and_group_authorized_proxy_with_ranger(self):
     self._test_no_matching_user_and_group_authorized_proxy()
 
@@ -137,8 +136,8 @@ def _test_no_matching_user_and_group_authorized_proxy(self):
     resp = self.hs2_client.OpenSession(open_session_req)
     assert "User 'hue' is not authorized to delegate to 'abc'" in str(resp)
 
-  def _test_authorized_proxy_with_ranger(self, test_func, delegated_user,
-                                         delegated_to_group):
+  def _test_authorized_proxy_with_ranger(
+      self, test_func, delegated_user, delegated_to_group):
     try:
       self.session_handle = self._open_hs2(RANGER_ADMIN_USER, dict()).sessionHandle
       if not delegated_to_group:
@@ -217,14 +216,16 @@ def _wait_for_audit_record(self, user, impersonator, timeout_secs=30):
     # found.
     start_time = time.time()
     while time.time() - start_time < timeout_secs:
-      for audit_file_name in os.listdir(AUDIT_LOG_DIR):
-        if self._find_matching_audit_record(audit_file_name, user, impersonator):
+      for audit_file_name in os.listdir(self.get_tmp_dir(AUDIT_LOG_DIR)):
+        if self._find_matching_audit_record(
+          audit_file_name, user, impersonator):
           return True
       time.sleep(1)
     return False
 
   def _find_matching_audit_record(self, audit_file_name, user, impersonator):
-    with open(os.path.join(AUDIT_LOG_DIR, audit_file_name)) as audit_log_file:
+    with open(
+      os.path.join(self.get_tmp_dir(AUDIT_LOG_DIR), audit_file_name)) as audit_log_file:
       for line in audit_log_file.readlines():
         json_dict = json.loads(line)
         if len(json_dict) == 0: continue

tests/authorization/test_provider.py

@@ -19,8 +19,6 @@
 
 from __future__ import absolute_import, division, print_function
 import pytest
-import os
-import tempfile
 
 from tests.common.file_utils import assert_file_in_dir_contains
 from tests.common.custom_cluster_test_suite import CustomClusterTestSuite
@@ -36,28 +34,29 @@ class TestAuthorizationProvider(CustomClusterTestSuite):
   """
 
   BAD_FLAG = "foobar"
-  LOG_DIR = tempfile.mkdtemp(prefix="test_provider_", dir=os.getenv("LOG_DIR"))
-  MINIDUMP_PATH = tempfile.mkdtemp()
+  LOG_DIR = "invalid_provider_flag"
+  MINIDUMP_PATH = "invalid_provider_flag_minidump"
 
   pre_test_cores = None
 
   @pytest.mark.execute_serially
   @CustomClusterTestSuite.with_args(
       expect_cores=True,
-      impala_log_dir=LOG_DIR,
-      impalad_args="--minidump_path={0} "
+      impala_log_dir="{" + LOG_DIR + "}",
+      impalad_args="--minidump_path={" + MINIDUMP_PATH + "} "
                    "--server-name=server1 "
                    "--ranger_service_type=hive "
                    "--ranger_app_id=impala "
-                   "--authorization_provider={1}".format(MINIDUMP_PATH, BAD_FLAG),
-      catalogd_args="--minidump_path={0} "
+                   "--authorization_provider=" + BAD_FLAG,
+      catalogd_args="--minidump_path={" + MINIDUMP_PATH + "} "
                     "--server-name=server1 "
                     "--ranger_service_type=hive "
                     "--ranger_app_id=impala "
-                    "--authorization_provider={1}".format(MINIDUMP_PATH, BAD_FLAG))
-  def test_invalid_provider_flag(self, unique_name):
+                    "--authorization_provider=" + BAD_FLAG,
+      tmp_dir_placeholders=[LOG_DIR, MINIDUMP_PATH])
+  def test_invalid_provider_flag(self):
     # parse log file for expected exception
-    assert_file_in_dir_contains(TestAuthorizationProvider.LOG_DIR,
+    assert_file_in_dir_contains(self.get_tmp_dir(self.LOG_DIR),
                                 "InternalException: Could not parse "
                                 "authorization_provider flag: {0}"
-                                .format(TestAuthorizationProvider.BAD_FLAG))
+                                .format(self.BAD_FLAG))