[jira] [Commented] (AMBARI-2453) Add cleanup script to Ambari to make sure previous artifacts are cleaned up.

["Sumit Mohanty (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/AMBARI-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13691726#comment-13691726 ] 

Sumit Mohanty commented on AMBARI-2453:
---------------------------------------

Most of the comments are minor - some are optional to incorporate. 

Let's add a black-list of items to avoid when erasing. Based on our experiments, we can figure out what needs to be in the list.

===========
+HOST_CHECK_FILEPATH_DEFAULT = "/var/lib/ambari-agent/data/hostcheck.result"
+OUTPUT_FILEPATH_DEFAULT = "/var/lib/ambari-agent/data/hostcleanup.result"
=====
Let's read the "prefix" value from ambari-agent.ini and use that as the base path.

===========
+  if options.skip:
+    global SKIP_LIST
+    SKIP_LIST = options.skip.splt(',')
=====
Let's by default skip the users.

===========
+    raise RuntimeError('HostCleanp needs to be run as root.')
=====
Spelling - ...Cleanp

===========
+  if propMap:
+    h.do_cleanup(propMap)
=====
Lets add some conclusion message.

===========
+  def do_erase_dir_silent(self, pathList):
...
+          else:
+            logger.info(path + " is a file and not a directory, deleting file")
+            try:
+              os.remove(path)
=====
Let's call the do_erase_file_silent. That way it will be easier to maintain.

===========
+  def do_erase_packages(self, packageList):
+    packageStr = None
+    if packageList:
+      packageStr = ' '.join(packageList)
=====
If erasing one package fails will this method remove remaining packages. Otherwise, we can erase then one by one.

===========
+      else:
+        logger.warn("Unsupported OS type, cannot get repository location.")
+        return []
=====
There are one or two places where we also check for OS type. We can report the same error on unsupported OS type.

===========
+  def find_repo_files_for_repos(self, repoNameList):
...
+          with open(filePath, 'r') as file:
+            contents = file.read()
+            for repoName in repoNameList:
+              if repoName and repoName in contents and filePath not in repoFiles:
+                repoFiles.append(filePath)
======
This I am a bit concerned about. We need to tighten the search to make sure that we are not accidentally removing repos that we need. Let's talk.

===========
Can we add an unit test that fakes a host-check report and then makes the necessary calls?

                
> Add cleanup script to Ambari to make sure previous artifacts are cleaned up.
> ----------------------------------------------------------------------------
>
>                 Key: AMBARI-2453
>                 URL: https://issues.apache.org/jira/browse/AMBARI-2453
>             Project: Ambari
>          Issue Type: Improvement
>          Components: agent
>    Affects Versions: 1.2.5
>            Reporter: Siddharth Wagle
>            Assignee: Siddharth Wagle
>             Fix For: 1.2.5
>
>         Attachments: AMBARI-2453.patch
>
>
> Add cleanup script to Ambari to make sure previous artifacts are cleaned up. We need to make sure there is a cleanup script which can be run to make with good enough help to cleanup old artifacts if present.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira