AMBARI-11263. Optimize creating users and groups (aonishuk)

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

@@ -22,6 +22,8 @@ import os
 from resource_management.core.system import System
 from resource_management.core import Environment, Fail, sudo
 from resource_management.core.resources import Directory
+import pwd
+import grp
 
 @patch.object(System, "os_family", new = 'redhat')
 class TestDirectoryResource(TestCase):
@@ -32,16 +34,19 @@ class TestDirectoryResource(TestCase):
   @patch.object(sudo, "stat")
   @patch.object(sudo,"chmod")
   @patch.object(sudo,"chown")
-  @patch("resource_management.core.providers.system._coerce_uid")
-  @patch("resource_management.core.providers.system._coerce_gid")
-  def test_create_directory_recursive(self, _coerce_gid_mock, _coerce_uid_mock,
+  @patch.object(pwd, "getpwnam")
+  @patch.object(grp, "getgrnam")
+  def test_create_directory_recursive(self, getgrnam_mock, getpwnam_mock,
                                       os_chown_mock, os_chmod_mock, os_stat_mock,
                                       isdir_mock, os_makedirs_mock, 
                                       os_path_exists_mock):
     os_path_exists_mock.return_value = False
     isdir_mock.return_value = True
-    _coerce_uid_mock.return_value = 66
-    _coerce_gid_mock.return_value = 77
+    getpwnam_mock.return_value = MagicMock()
+    getpwnam_mock.return_value.pw_uid = 66
+    getgrnam_mock.return_value = MagicMock()
+    getgrnam_mock.return_value.gr_gid = 77
+    
     os_stat_mock.return_value = type("", (), dict(st_mode=0755, st_uid=0, st_gid=0))()
     
     with Environment('/') as env:
@@ -55,8 +60,7 @@ class TestDirectoryResource(TestCase):
       
     os_makedirs_mock.assert_called_with('/a/b/c/d', 0777)
     os_chmod_mock.assert_called_with('/a/b/c/d', 0777)
-    os_chown_mock.assert_any_call('/a/b/c/d', 'hdfs', None)
-    os_chown_mock.assert_any_call('/a/b/c/d', None, 'hadoop')
+    os_chown_mock.assert_any_call('/a/b/c/d', getpwnam_mock.return_value, getgrnam_mock.return_value)
   
   @patch.object(sudo, "path_exists")
   @patch.object(os.path, "dirname")
@@ -65,17 +69,19 @@ class TestDirectoryResource(TestCase):
   @patch.object(sudo, "stat")
   @patch.object(sudo,"chmod")
   @patch.object(sudo,"chown")
-  @patch("resource_management.core.providers.system._coerce_uid")
-  @patch("resource_management.core.providers.system._coerce_gid")
-  def test_create_directory_not_recursive(self, _coerce_gid_mock, _coerce_uid_mock,
+  @patch.object(pwd, "getpwnam")
+  @patch.object(grp, "getgrnam")
+  def test_create_directory_not_recursive(self, getgrnam_mock, getpwnam_mock,
                                       os_chown_mock, os_chmod_mock, os_stat_mock,
                                       mkdir_mock, isdir_mock, os_dirname_mock, 
                                       os_path_exists_mock):
     os_path_exists_mock.return_value = False
     os_dirname_mock.return_value = "/a/b/c"
     isdir_mock.return_value = True
-    _coerce_uid_mock.return_value = 66
-    _coerce_gid_mock.return_value = 77
+    getpwnam_mock.return_value = MagicMock()
+    getpwnam_mock.return_value.pw_uid = 66
+    getgrnam_mock.return_value = MagicMock()
+    getpwnam_mock.return_value.gr_gid = 77
     os_stat_mock.return_value = type("", (), dict(st_mode=0755, st_uid=0, st_gid=0))()
     
     with Environment('/') as env:
@@ -88,8 +94,7 @@ class TestDirectoryResource(TestCase):
       
     mkdir_mock.assert_called_with('/a/b/c/d', 0777)
     os_chmod_mock.assert_called_with('/a/b/c/d', 0777)
-    os_chown_mock.assert_any_call('/a/b/c/d', 'hdfs', None)
-    os_chown_mock.assert_any_call('/a/b/c/d', None, 'hadoop')
+    os_chown_mock.assert_any_call('/a/b/c/d', getpwnam_mock.return_value, getgrnam_mock.return_value)
     
   @patch.object(sudo, "path_exists")
   @patch.object(os.path, "dirname")

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

@@ -133,23 +133,6 @@ class TestExecuteResource(TestCase):
     except Fail as e:
       pass
 
-  @patch.object(grp, "getgrnam")
-  @patch.object(pwd, "getpwnam")
-  def test_attribute_group(self, getpwnam_mock, getgrnam_mock):
-    def error(argument):
-      self.assertEqual(argument, "test_group")
-      raise KeyError("fail")
-
-    getpwnam_mock.side_effect = 1
-    getgrnam_mock.side_effect = error
-    try:
-      with Environment("/") as env:
-        Execute('echo "1"',
-                group="test_group",
-        )
-    except Fail as e:
-      pass
-
   @patch.object(subprocess, "Popen")
   def test_attribute_environment(self, popen_mock):
     expected_dict = {"JAVA_HOME": "/test/java/home"}

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

@@ -21,6 +21,8 @@ from unittest import TestCase
 from mock.mock import patch, MagicMock
 import os
 import sys
+import grp
+import pwd
 from resource_management.core import Environment, Fail
 from resource_management.core.resources import File
 from resource_management.core.system import System
@@ -261,16 +263,16 @@ class TestFileResource(TestCase):
     self.assertEqual(open_mock.call_count, 0)
 
 
-  @patch("resource_management.core.providers.system._coerce_uid")
-  @patch("resource_management.core.providers.system._coerce_gid")
+  @patch.object(pwd, "getpwnam")
+  @patch.object(grp, "getgrnam")
   @patch.object(sudo, "chown")
   @patch.object(sudo, "chmod")
   @patch.object(sudo, "stat")
   @patch.object(sudo, "create_file")
   @patch.object(sudo, "path_exists")
   @patch.object(sudo, "path_isdir")
-  def test_ensure_metadata(self, isdir_mock, exists_mock, create_file_mock, stat_mock, chmod_mock, chown_mock, gid_mock,
-                           uid_mock):
+  def test_ensure_metadata(self, isdir_mock, exists_mock, create_file_mock, stat_mock, chmod_mock, chown_mock, getgrnam_mock,
+                           getpwnam_mock):
     """
     Tests if _ensure_metadata changes owner, usergroup and permissions of file to proper values
     """
@@ -284,8 +286,10 @@ class TestFileResource(TestCase):
         self.st_gid = 1
 
     stat_mock.return_value = stat()
-    gid_mock.return_value = 0
-    uid_mock.return_value = 0
+    getpwnam_mock.return_value = MagicMock()
+    getpwnam_mock.return_value.pw_uid = 0
+    getgrnam_mock.return_value = MagicMock()
+    getgrnam_mock.return_value.gr_gid = 0
 
     with Environment('/') as env:
       File('/directory/file',
@@ -301,14 +305,16 @@ class TestFileResource(TestCase):
     self.assertEqual(create_file_mock.call_count, 1)
     stat_mock.assert_called_with('/directory/file')
     self.assertEqual(chmod_mock.call_count, 1)
-    self.assertEqual(chown_mock.call_count, 2)
-    gid_mock.assert_called_once_with('hdfs')
-    uid_mock.assert_called_once_with('root')
+    self.assertEqual(chown_mock.call_count, 1)
+    getgrnam_mock.assert_called_once_with('hdfs')
+    getpwnam_mock.assert_called_with('root')
 
     chmod_mock.reset_mock()
     chown_mock.reset_mock()
-    gid_mock.return_value = 1
-    uid_mock.return_value = 1
+    getpwnam_mock.return_value = MagicMock()
+    getpwnam_mock.return_value.pw_uid = 1
+    getgrnam_mock.return_value = MagicMock()
+    getgrnam_mock.return_value.gr_gid = 1
 
     with Environment('/') as env:
       File('/directory/file',
@@ -321,7 +327,7 @@ class TestFileResource(TestCase):
     
 
     self.assertEqual(chmod_mock.call_count, 1)
-    self.assertEqual(chown_mock.call_count, 0)
+    chown_mock.assert_called_with('/directory/file', None, None)
 
   @patch("resource_management.core.providers.system._ensure_metadata")
   @patch("resource_management.core.providers.system.FileProvider._get_content")

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

@@ -62,7 +62,7 @@ class TestGroupResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getgrnam_mock.return_value = "mapred"
+    getgrnam_mock.return_value = _get_group()
 
     with Environment('/') as env:
       Group('mapred',
@@ -84,7 +84,7 @@ class TestGroupResource(TestCase):
     subproc_mock.returncode = 1
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getgrnam_mock.return_value = "mapred"
+    getgrnam_mock.return_value = _get_group()
 
     try:
       with Environment('/') as env:
@@ -110,7 +110,7 @@ class TestGroupResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getgrnam_mock.return_value = "mapred"
+    getgrnam_mock.return_value = _get_group()
 
     with Environment('/') as env:
       Group('mapred',
@@ -119,7 +119,7 @@ class TestGroupResource(TestCase):
     
 
     self.assertEqual(popen_mock.call_count, 1)
-    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'groupdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
+    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'ambari-sudo.sh  PATH=/bin -H -E groupdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
     getgrnam_mock.assert_called_with('mapred')
 
 
@@ -131,7 +131,7 @@ class TestGroupResource(TestCase):
     subproc_mock.returncode = 1
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getgrnam_mock.return_value = "mapred"
+    getgrnam_mock.return_value = _get_group()
 
     try:
       with Environment('/') as env:
@@ -144,5 +144,15 @@ class TestGroupResource(TestCase):
       pass
 
     self.assertEqual(popen_mock.call_count, 1)
-    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'groupdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
+    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'ambari-sudo.sh  PATH=/bin -H -E groupdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
     getgrnam_mock.assert_called_with('mapred')
+    
+def _get_group():
+  group = MagicMock()
+  group.gr_name='mapred'
+  group.gr_passwd='x'
+  group.gr_gid=0
+  group.gr_mem=[]
+  
+  return group
+  

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

@@ -46,7 +46,7 @@ class TestUserResource(TestCase):
 
     popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', "ambari-sudo.sh  PATH=/bin -H -E useradd -m -s /bin/bash mapred"], shell=False, preexec_fn=None, stderr=-2, stdout=5, env={'PATH': '/bin'}, bufsize=1, cwd=None, close_fds=True)
     self.assertEqual(popen_mock.call_count, 1)
-
+    
   @patch.object(subprocess, "Popen")
   @patch.object(pwd, "getpwnam")
   def test_action_create_existent(self, getpwnam_mock, popen_mock):
@@ -54,7 +54,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", shell = "/bin/bash")
@@ -74,7 +74,7 @@ class TestUserResource(TestCase):
     with Environment('/') as env:
       user = User("mapred", action = "remove", shell = "/bin/bash")
 
-    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'userdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
+    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'ambari-sudo.sh  PATH=/bin -H -E userdel mapred'], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
     self.assertEqual(popen_mock.call_count, 1)
 
   @patch.object(subprocess, "Popen")
@@ -84,7 +84,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", comment = "testComment", 
@@ -100,7 +100,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", home = "/test/home", 
@@ -116,7 +116,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", password = "secure", 
@@ -132,7 +132,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", shell = "/bin/sh")
@@ -147,7 +147,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", uid = "1", shell = "/bin/bash")
@@ -162,7 +162,7 @@ class TestUserResource(TestCase):
     subproc_mock.returncode = 0
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", gid = "1", shell = "/bin/bash")
@@ -179,13 +179,13 @@ class TestUserResource(TestCase):
     user_groups_mock.return_value = ['hadoop']
     subproc_mock.stdout.readline = MagicMock(side_effect = ['OK'])
     popen_mock.return_value = subproc_mock
-    getpwnam_mock.return_value = 1
+    getpwnam_mock.return_value = _get_user_entity()
 
     with Environment('/') as env:
       user = User("mapred", action = "create", groups = ['1','2','3'], 
           shell = "/bin/bash")
 
-    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'ambari-sudo.sh  PATH=/bin -H -E usermod -G 1,2,3,hadoop -s /bin/bash mapred'], shell=False, preexec_fn=None, env={'PATH': '/bin'}, close_fds=True, stdout=5, stderr=-2, bufsize=1, cwd=None)
+    popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', 'ambari-sudo.sh  PATH=/bin -H -E usermod -s /bin/bash -G 1,2,3,hadoop mapred'], shell=False, preexec_fn=None, env={'PATH': '/bin'}, close_fds=True, stdout=5, stderr=-2, bufsize=1, cwd=None)
     self.assertEqual(popen_mock.call_count, 1)
 
   @patch.object(subprocess, "Popen")
@@ -202,3 +202,14 @@ class TestUserResource(TestCase):
     popen_mock.assert_called_with(['/bin/bash', '--login', '--noprofile', '-c', "ambari-sudo.sh  PATH=/bin -H -E useradd -m mapred"], shell=False, preexec_fn=None, stderr=-2, stdout=5, bufsize=1, env={'PATH': '/bin'}, cwd=None, close_fds=True)
     self.assertEqual(popen_mock.call_count, 1)
 
+def _get_user_entity():
+  user = MagicMock()
+  user.pw_name='mapred'
+  user.pw_passwd='x'
+  user.pw_uid=0
+  user.pw_gid=0
+  user.pw_gecos='root'
+  user.pw_dir='/root'
+  user.pw_shell='/bin/false'
+  
+  return user

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

@@ -30,38 +30,48 @@ from resource_management.core.logger import Logger
 
 
 class UserProvider(Provider):
+  options = dict(
+    comment=(lambda self: self.user.pw_gecos, "-c"),
+    gid=(lambda self: grp.getgrgid(self.user.pw_gid).gr_name, "-g"),
+    uid=(lambda self: self.user.pw_uid, "-u"),
+    shell=(lambda self: self.user.pw_shell, "-s"),
+    password=(lambda self: self.user.pw_password, "-p"),
+    home=(lambda self: self.user.pw_dir, "-d"),
+    groups=(lambda self: self.user_groups, "-G")
+  )
+    
   def action_create(self):
     if not self.user:
       command = ['useradd', "-m"]
       Logger.info("Adding user %s" % self.resource)
     else:
       command = ['usermod']
+      
+      for option_name, attributes in self.options.iteritems():
+        if getattr(self.resource, option_name) != None and getattr(self.resource, option_name) != attributes[0](self):
+          # groups on system contain the one we need
+          if attributes[1] == "-G" and set(getattr(self.resource, option_name)).issubset(set(attributes[0](self))):
+            continue
+          break
+      else:
+        return
+      
       Logger.info("Modifying user %s" % (self.resource.username))
 
-    options = dict(
-      comment="-c",
-      gid="-g",
-      uid="-u",
-      shell="-s",
-      password="-p",
-      home="-d",
-    )
-
     if self.resource.system and not self.user:
       command.append("--system")
-
-    if self.resource.groups:
-      
-      groups = self.resource.groups
-      if self.user and self.user_groups:
-        groups += self.user_groups
       
-      command += ["-G", ",".join(groups)]
-
-    for option_name, option_flag in options.items():
-      option_value = getattr(self.resource, option_name)
-      if option_flag and option_value:
-        command += [option_flag, str(option_value)]
+    for option_name, attributes in self.options.iteritems():   
+      if attributes[1] == "-G":
+        groups = self.resource.groups
+        if self.user and self.user_groups:
+          groups += self.user_groups
+        option_value = ",".join(groups)
+      else:
+        option_value = getattr(self.resource, option_name)
+        
+      if attributes[1] and option_value:
+        command += [attributes[1], str(option_value)]
 
     # if trying to modify existing user, but no values to modify are provided
     if self.user and len(command) == 1:
@@ -74,7 +84,7 @@ class UserProvider(Provider):
   def action_remove(self):
     if self.user:
       command = ['userdel', self.resource.username]
-      shell.checked_call(command)
+      shell.checked_call(command, sudo=True)
       Logger.info("Removed user %s" % self.resource)
 
   @property
@@ -89,6 +99,10 @@ class UserProvider(Provider):
     return [g.gr_name for g in grp.getgrall() if self.resource.username in g.gr_mem]
 
 class GroupProvider(Provider):
+  options = dict(
+    gid=(lambda self: self.group.gr_gid, "-g"),
+    password=(lambda self: self.group.gr_passwd, "-p")
+  )
   def action_create(self):
     group = self.group
     if not group:
@@ -96,28 +110,32 @@ class GroupProvider(Provider):
       Logger.info("Adding group %s" % self.resource)
     else:
       command = ['groupmod']
-      Logger.info("Modifying group %s" % (self.resource.group_name))
       
-    options = dict(
-        gid="-g",
-        password="-p",
-    )
+      for option_name, attributes in self.options.iteritems():
+        if getattr(self.resource, option_name) != None and getattr(self.resource, option_name) != attributes[0](self):
+          break
+      else:
+        return
+      
+      Logger.info("Modifying group %s" % (self.resource.group_name))
 
-    for option_name, option_flag in options.items():
+    for option_name, attributes in self.options.iteritems():
       option_value = getattr(self.resource, option_name)
-      if option_flag and option_value:
-        command += [option_flag, str(option_value)]
+      if attributes[1] and option_value:
+        command += [attributes[1], str(option_value)]
         
     command.append(self.resource.group_name)
 
+    # if trying to modify existing group, but no values to modify are provided
+    if self.group and len(command) == 1:
+      return
+    
     shell.checked_call(command, sudo=True)
 
-    group = self.group
-
   def action_remove(self):
     if self.group:
       command = ['groupdel', self.resource.group_name]
-      shell.checked_call(command)
+      shell.checked_call(command, sudo=True)
       Logger.info("Removed group %s" % self.resource)
 
   @property

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

@@ -25,32 +25,34 @@ from __future__ import with_statement
 import re
 import os
 import time
+import pwd
+import grp
 from resource_management.core import shell
 from resource_management.core import sudo
 from resource_management.core.base import Fail
 from resource_management.core import ExecuteTimeoutException
 from resource_management.core.providers import Provider
 from resource_management.core.logger import Logger
-from resource_management.core.utils import _coerce_uid
-from resource_management.core.utils import _coerce_gid
 
 def _ensure_metadata(path, user, group, mode=None, cd_access=None):
   stat = sudo.stat(path)
-
+  user_entity = group_entity = None
+  
   if user:
-    uid = _coerce_uid(user)
-    if stat.st_uid != uid:
+    _user_entity = pwd.getpwnam(user)
+    if stat.st_uid != _user_entity.pw_uid:
+      user_entity = _user_entity
       Logger.info(
         "Changing owner for %s from %d to %s" % (path, stat.st_uid, user))
       
-      sudo.chown(path, user, None)
-      
   if group:
-    gid = _coerce_gid(group)
-    if stat.st_gid != gid:
+    _group_entity = grp.getgrnam(group)
+    if stat.st_gid != _group_entity.gr_gid:
+      group_entity = _group_entity
       Logger.info(
         "Changing group for %s from %d to %s" % (path, stat.st_gid, group))
-      sudo.chown(path, None, group)
+  
+  sudo.chown(path, user_entity, group_entity)
       
   if mode:
     if stat.st_mode != mode:
@@ -214,7 +216,7 @@ class LinkProvider(Provider):
 def _preexec_fn(resource):
   def preexec():
     if resource.group:
-      gid = _coerce_gid(resource.group)
+      gid = grp.getgrnam(resource.group).gr_gid
       os.setgid(gid)
       os.setegid(gid)
 

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

@@ -25,13 +25,14 @@ import shutil
 import stat
 from resource_management.core import shell
 from resource_management.core.logger import Logger
-from resource_management.core.utils import _coerce_uid
-from resource_management.core.utils import _coerce_gid
 from ambari_commons.os_check import OSCheck
 
 if os.geteuid() == 0:
   def chown(path, owner, group):
-    return os.chown(path, _coerce_uid(owner) if owner else -1, _coerce_gid(group) if group else -1)
+    uid = owner.pw_uid if owner else -1
+    gid = group.gr_gid if group else -1
+    if uid != -1 or gid != 1:
+      return os.chown(path, uid, gid)
   
   def chmod(path, mode):
     return os.chmod(path, mode)
@@ -101,10 +102,10 @@ else:
 
   # os.chown replacement
   def chown(path, owner, group):
-    if owner:
-      shell.checked_call(["chown", owner, path], sudo=True)
-    if group:
-      shell.checked_call(["chgrp", group, path], sudo=True)
+    owner = owner.pw_name if owner else ""
+    group = group.gr_name if group else ""
+    if owner or group:
+      shell.checked_call(["chown", owner+":"+group, path], sudo=True)
       
   # os.chmod replacement
   def chmod(path, mode):

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

@@ -106,28 +106,3 @@ def checked_unite(dict1, dict2):
   result.update(dict2)
   
   return result
-
-def _coerce_uid(user):
-  import pwd
-
-  try:
-    uid = int(user)
-  except ValueError:
-    try:
-      uid = pwd.getpwnam(user).pw_uid
-    except KeyError:
-      raise Fail("User %s doesn't exist." % user)
-  return uid
-
-
-def _coerce_gid(group):
-  import grp
-
-  try:
-    gid = int(group)
-  except ValueError:
-    try:
-      gid = grp.getgrnam(group).gr_gid
-    except KeyError:
-      raise Fail("Group %s doesn't exist." % group)
-  return gid