F #6227: New API for VMGroup roles

* New API for VMGroup roles: add, del, update
* Go and Java api for VM Group roles

include/Attribute.h

@@ -73,9 +73,7 @@ public:
     virtual std::string marshall(const char * _sep = 0) const = 0;
 
     /**
-     *  Write the attribute using a simple XML format. The string MUST be freed
+     *  Write the attribute using a simple XML format.
-     *  by the calling function.
-     *    @return a string (allocated in the heap) holding the attribute value.
      */
     virtual void to_xml(std::ostringstream& s) const = 0;
 
@@ -424,9 +422,6 @@ public:
      *    ...
      *    <val_name_n>val_value_n</val_name_n>
      *  </attribute_name>
-     *
-     *  The string MUST be freed by the calling function.
-     *    @return a string (allocated in the heap) holding the attribute value.
      */
     void to_xml(std::ostringstream& s) const override;
 
@@ -451,7 +446,7 @@ public:
      * @param replace True to replace existing values, false to copy values
      * only if they don't exist in this vector attribute
      */
-    void merge(VectorAttribute* vattr, bool replace);
+    void merge(const VectorAttribute* vattr, bool replace);
 
     /**
      *  Replace the value of the given vector attribute

include/RequestManagerVMGroup.h

@@ -0,0 +1,98 @@
+/* -------------------------------------------------------------------------- */
+/* Copyright 2002-2023, OpenNebula Project, OpenNebula Systems                */
+/*                                                                            */
+/* Licensed under the Apache License, Version 2.0 (the "License"); you may    */
+/* not use this file except in compliance with the License. You may obtain    */
+/* a copy of the License at                                                   */
+/*                                                                            */
+/* http://www.apache.org/licenses/LICENSE-2.0                                 */
+/*                                                                            */
+/* Unless required by applicable law or agreed to in writing, software        */
+/* distributed under the License is distributed on an "AS IS" BASIS,          */
+/* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.   */
+/* See the License for the specific language governing permissions and        */
+/* limitations under the License.                                             */
+/* -------------------------------------------------------------------------- */
+
+#ifndef REQUEST_MANAGER_VM_GROUP_H
+#define REQUEST_MANAGER_VM_GROUP_H
+
+#include "Request.h"
+#include "Nebula.h"
+#include "VMGroupPool.h"
+
+/* ------------------------------------------------------------------------- */
+/* ------------------------------------------------------------------------- */
+/* ------------------------------------------------------------------------- */
+
+class RequestManagerVMGroup: public Request
+{
+protected:
+    RequestManagerVMGroup(const std::string& method_name,
+                          const std::string& params,
+                          const std::string& help)
+        : Request(method_name,params,help)
+    {
+        Nebula& nd  = Nebula::instance();
+        pool        = nd.get_vmgrouppool();
+
+        auth_object = PoolObjectSQL::VMGROUP;
+        auth_op     = AuthRequest::MANAGE;
+    }
+};
+
+/* ------------------------------------------------------------------------- */
+/* ------------------------------------------------------------------------- */
+
+class VMGroupAddRole : public RequestManagerVMGroup
+{
+public:
+    VMGroupAddRole():
+        RequestManagerVMGroup("one.vmgroup.roleadd", "A:sis",
+                              "Add new role to VMGroup")
+    {
+    }
+
+protected:
+    void request_execute(xmlrpc_c::paramList const& _paramList,
+                         RequestAttributes& att) override;
+};
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+class VMGroupDelRole : public RequestManagerVMGroup
+{
+public:
+    VMGroupDelRole():
+        RequestManagerVMGroup("one.vmgroup.roledelete", "A:sii",
+                              "Delete role from VMGroup")
+    {
+    }
+
+protected:
+    void request_execute(xmlrpc_c::paramList const& _paramList,
+                         RequestAttributes& att) override;
+};
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+class VMGroupUpdateRole : public RequestManagerVMGroup
+{
+public:
+    VMGroupUpdateRole():
+        RequestManagerVMGroup("one.vmgroup.roleupdate", "A:siis",
+                              "Update VMGroup role")
+    {
+    }
+
+protected:
+    void request_execute(xmlrpc_c::paramList const& _paramList,
+                         RequestAttributes& att) override;
+};
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+#endif

include/VMGroup.h

@@ -88,7 +88,7 @@ public:
      */
     int add_vm(const std::string& role_name, int vmid)
     {
-        return roles.add_vm(role_name, vmid);
+        return _roles.add_vm(role_name, vmid);
     }
 
     /**
@@ -100,9 +100,44 @@ public:
      */
     int del_vm(const std::string& role_name, int vmid)
     {
-        return roles.del_vm(role_name, vmid);
+        return _roles.del_vm(role_name, vmid);
     }
 
+    VMGroupRoles& roles()
+    {
+        return _roles;
+    }
+
+    /**
+     *  Adds a new role to the set
+     *    @param vrole VectorAttribute of the role
+     *    @param error string if any
+     *
+     *    @return 0 on success
+     */
+    int add_role(VectorAttribute * vrole, std::string& error);
+
+    /**
+     *  Delete role from the set
+     *    @param id ID of the role
+     *    @param error string if any
+     *
+     *    @return 0 on success
+     */
+    int del_role(int id, std::string& error);
+
+    /**
+     *  Update existing role
+     *    @param id ID of the role to update
+     *    @param vrole VectorAttribute of the role
+     *    @param error string if any
+     *
+     *    @return 0 on success
+     */
+    int update_role(int id, VectorAttribute * vrole, std::string& error);
+
+    int check_consistency(std::string& error_str);
+
 private:
     // -------------------------------------------------------------------------
     // Friends
@@ -199,7 +234,7 @@ private:
 	/**
 	 *  The role set
 	 */
-	VMGroupRoles roles;
+	VMGroupRoles _roles;
 };
 
 #endif /*VMGROUP_H_*/

include/VMGroupRole.h

@@ -40,8 +40,6 @@ class VMGroupRole
 public:
     VMGroupRole(VectorAttribute *_va);
 
-    virtual ~VMGroupRole(){};
-
     /**
      *  @return the role id
      */
@@ -62,6 +60,14 @@ public:
         return va->vector_value("NAME");
     }
 
+    /**
+     *  Set role name
+     */
+    void name(const std::string& new_name)
+    {
+        va->replace("NAME", new_name);
+    }
+
     /**
      *  @return the set of VMs in a string in a comma separated list
      */
@@ -80,6 +86,14 @@ public:
         return va->vector_value("POLICY");
     };
 
+    /**
+     * Function to print the VMGroupRole into a string stream in XML format
+     *   @param xml Output string stream
+     */
+    void to_xml(std::ostringstream &oss) const;
+
+    void update(VectorAttribute* va_update);
+
     /* ---------------------------------------------------------------------- */
     /* VMS set Interface                                                      */
     /* ---------------------------------------------------------------------- */
@@ -161,31 +175,30 @@ private:
 class VMGroupRoles
 {
 public:
-    VMGroupRoles():roles_template(false,'=',"ROLES"), next_role(0){};
+    VMGroupRoles() = default;
+
+    // Disable copy constructor
+    VMGroupRoles(const VMGroupRoles&) = delete;
+
+    // Disable copy assignment
+    VMGroupRoles& operator=(const VMGroupRoles&) = delete;
 
     ~VMGroupRoles()
     {
-        by_id.delete_roles();
+        delete_roles();
-    };
+    }
 
-    /* ---------------------------------------------------------------------- */
-    /* ---------------------------------------------------------------------- */
     /**
      * Max number of roles in a VMGroup
      */
     const static int MAX_ROLES = 32;
 
-    /* ---------------------------------------------------------------------- */
-    /* ---------------------------------------------------------------------- */
     /**
      * Function to print the VMGroupRoles into a string in XML format
      *   @param xml the resulting XML string
      *   @return a reference to the generated string
      */
-    std::string& to_xml(std::string& xml_str) const
+    std::string& to_xml(std::string& xml_str) const;
-    {
-        return roles_template.to_xml(xml_str);
-    }
 
     /**
      *  Builds the object from an xml node
@@ -203,6 +216,14 @@ public:
      */
     int add_role(VectorAttribute * vrole, std::string& error);
 
+    /**
+     *  Delete role from the set
+     *    @param id ID of the role
+     */
+    void del_role(int id);
+
+    int rename_role(VMGroupRole* role, const std::string& new_name);
+
     /**
      *  Generates the ids corresponding to a set of role names
      *    @param rnames string with a comma separated list of role names
@@ -241,7 +262,14 @@ public:
      */
     VMGroupRole * get(const std::string& rname)
     {
-        return by_name.get(rname);
+        auto it = by_name.find(rname);
+
+        if (it == by_name.end())
+        {
+            return nullptr;
+        }
+
+        return it->second;
     }
 
     /**
@@ -250,7 +278,14 @@ public:
      */
     VMGroupRole * get(int id)
     {
-        return by_id.get(id);
+        auto it = by_id.find(id);
+
+        if (it == by_id.end())
+        {
+            return nullptr;
+        }
+
+        return it->second;
     }
 
     /* ---------------------------------------------------------------------- */
@@ -309,105 +344,34 @@ public:
     typedef class RoleIterator role_iterator;
 
 private:
-    /**
-     *  A role map indexed by different key types
-     */
-    template<class T>
-    class RoleMap
-    {
-    public:
-        /**
-         *  Inserts a new role in the map
-         *    @param k the key
-         *    @param r pointer to yhe VMGroupRole
-         *    @return true if the role was successfully inserted
-         */
-        bool insert(const T& k, VMGroupRole * r)
-        {
-            std::pair<T, VMGroupRole *> rpair(k, r);
-            std::pair<roles_it, bool> rc;
-
-            rc = roles.insert(rpair);
-
-            return rc.second;
-        }
-
-        /**
-         *  Frees the memory associated to the map and clears it
-         */
-        void delete_roles()
-        {
-            for (roles_it it = roles.begin() ; it != roles.end() ; ++it )
-            {
-                delete it->second;
-            }
-
-            clear();
-        }
-
-        VMGroupRole * get(const T& k)
-        {
-            auto it = roles.find(k);
-
-            if ( it == roles.end() )
-            {
-                return nullptr;
-            }
-
-            return it->second;
-        }
-
-        /**
-         *  Clears the contents of the map
-         */
-        void clear()
-        {
-            roles.clear();
-        }
-
-        size_t erase(const T& k)
-        {
-            return roles.erase(k);
-        }
-
-        /**
-         *  Iterators for the map
-         */
-        typedef typename std::map<T, VMGroupRole *>::iterator roles_it;
-
-        roles_it begin()
-        {
-            return roles.begin();
-        }
-
-        roles_it end()
-        {
-            return roles.end();
-        }
-
-    private:
-        std::map<T, VMGroupRole *> roles;
-    };
-
-    /**
-     *  The role template to store the VMGroupRole
-     */
-	Template roles_template;
-
     /**
      *  The next role id
      */
-    int next_role;
+    int next_role = 0;
 
     /**
      *  Map to access the roles by their name
      */
-    RoleMap<std::string> by_name;
+    std::map<std::string, VMGroupRole *> by_name;
 
     /**
      *  Map to access the roles by their id
      */
-    RoleMap<int> by_id;
+    std::map<int, VMGroupRole *> by_id;
+
+    /**
+     *  Frees the memory associated with the roles
+     */
+    void delete_roles()
+    {
+        for (auto it : by_id)
+        {
+            delete it.second;
+        }
+
+        by_id.clear();
+        by_name.clear();
+    }
 };
 
 #endif /*VMGROUP_ROLE_H*/

share/doc/xsd/vm_group.xsd

@@ -46,6 +46,7 @@
                       <xs:element name="ID" type="xs:integer"/>
                       <xs:element name="NAME" type="xs:string"/>
                       <xs:element name="POLICY" type="xs:string" minOccurs="0" maxOccurs="1"/>
+                      <xs:element name="VMS" type="xs:string" minOccurs="0" maxOccurs="1"/>
                     </xs:sequence>
                   </xs:complexType>
                 </xs:element>

src/cli/one_helper/onevmgroup_helper.rb

@@ -81,6 +81,21 @@ class OneVMGroupHelper < OpenNebulaHelper::OneHelper
         table
     end
 
+    def retrieve_role_id(vmg_id, id)
+        return [0, id.to_i] if id =~ /\A\d+\z/
+
+        puts vmg_id
+        vmg = retrieve_resource(vmg_id)
+        vmg.info
+
+        ids = vmg.retrieve_elements("ROLES/ROLE[NAME='#{id}']/ID")
+
+        return [-1, "#{id} not found or duplicated"] \
+                if ids.nil? || ids.size > 1
+
+        [0, ids[0].to_i]
+    end
+
     private
 
     def factory(id=nil)

src/cli/onevmgroup

@@ -108,7 +108,9 @@ CommandParser::CmdParser.new(ARGV) do
     end
 
     set :format, :vmgroupid, OneVMGroupHelper.to_id_desc do |arg|
-        helper.to_id(arg)
+        tmp = helper.to_id(arg)
+        @current_vmg = tmp[1]
+        tmp
     end
 
     set :format, :vmgroupid_list, OneVMGroupHelper.list_to_id_desc do |arg|
@@ -119,6 +121,10 @@ CommandParser::CmdParser.new(ARGV) do
         helper.filterflag_to_i(arg)
     end
 
+    format :roleid, 'Role identifier' do |arg|
+        helper.retrieve_role_id(@current_vmg, arg)
+    end
+
     ########################################################################
     # Commands
     ########################################################################
@@ -270,4 +276,61 @@ CommandParser::CmdParser.new(ARGV) do
             vmg.unlock
         end
     end
+
+    role_add_desc = <<-EOT.unindent
+        Add role to VM Group.
+    EOT
+
+    command :"role-add", role_add_desc, :vmgroupid, [:file, nil] do
+        begin
+            template = File.read(args[1]) if args[1]
+            template = STDIN.read if STDIN.wait_readable(0)
+        rescue StandardError => e
+            STDERR.puts "Error reading template: #{e.message}."
+            exit(-1)
+        end
+
+        # Ensure the template attributes are in ROLE section
+        unless template.gsub(' ', '').match(/ROLE=\[/)
+            template = "ROLE=[\n#{template.split("\n").join(",\n")}]"
+        end
+
+        helper.perform_action(args[0], options, 'Role added') do |vmg|
+            vmg.role_add(template)
+        end
+    end
+
+    role_delete_desc = <<-EOT.unindent
+        Deletes role from VM Group.
+    EOT
+
+    command :"role-delete", role_delete_desc, :vmgroupid, :roleid do
+        roleid = args[1].to_i
+
+        helper.perform_action(args[0], options, 'Role removed') do |vmg|
+            vmg.role_delete(roleid)
+        end
+    end
+
+    role_update_desc = <<-EOT.unindent
+        Update VM Group role
+    EOT
+
+    command :"role-update", role_update_desc, :vmgroupid, :roleid, [:file, nil] do
+        vmg_id = args[0].to_i
+        role_id = args[1].to_i
+        file = args[2]
+
+        helper.perform_action(vmg_id, options, 'Role updated') do |vmg|
+            str = OpenNebulaHelper.update_template(vmg_id, vmg, file,
+                                                   "ROLES/ROLE[ID=#{role_id}]")
+
+            # Ensure the updated attributes are in ROLE section
+            unless str.gsub(' ', '').match(/ROLE=\[/)
+                str = "ROLE=[\n#{str.split("\n").join(",\n")}]"
+            end
+
+            vmg.role_update(role_id, str)
+        end
+    end
 end

src/common/Attribute.cc

@@ -197,7 +197,7 @@ void VectorAttribute::replace(const map<string,string>& attr)
 /* -------------------------------------------------------------------------- */
 /* -------------------------------------------------------------------------- */
 
-void VectorAttribute::merge(VectorAttribute* vattr, bool replace)
+void VectorAttribute::merge(const VectorAttribute* vattr, bool replace)
 {
     const map<string,string>& source_values = vattr->value();
 

src/oca/go/src/goca/vmgroup.go

@@ -137,20 +137,6 @@ func (vc *VMGroupsController) CreateContext(ctx context.Context, tpl string) (in
 	return response.BodyInt(), nil
 }
 
-// Clone clones an existing vmGroup.
-// * newName: Name for the new vmGroup.
-func (vc *VMGroupController) Clone(newName string) error {
-	return vc.CloneContext(context.Background(), newName)
-}
-
-// CloneContext clones an existing vmGroup.
-// * ctx: context for cancelation
-// * newName: Name for the new vmGroup.
-func (vc *VMGroupController) CloneContext(ctx context.Context, newName string) error {
-	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.clone", vc.ID, newName)
-	return err
-}
-
 // Delete deletes the given vmGroup from the pool.
 func (vc *VMGroupController) Delete() error {
 	return vc.DeleteContext(context.Background())
@@ -226,6 +212,7 @@ func (vc *VMGroupController) Lock(level shared.LockLevel) error {
 }
 
 // LockContext locks the vmGroup following lock level. See levels in locks.go.
+// * ctx: context for cancelation
 func (vc *VMGroupController) LockContext(ctx context.Context, level shared.LockLevel) error {
 	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.lock", vc.ID, level)
 	return err
@@ -237,7 +224,51 @@ func (vc *VMGroupController) Unlock() error {
 }
 
 // UnlockContext unlocks the vmGroup.
+// * ctx: context for cancelation
 func (vc *VMGroupController) UnlockContext(ctx context.Context) error {
 	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.unlock", vc.ID)
 	return err
 }
+
+// RolaAdd adds role to VM Group
+// * roleTemplate: The new role content. Syntax can be the usual attribute=value or XML.
+func (vc *VMGroupController) RoleAdd(roleTemplate string) error {
+	return vc.RoleAddContext(context.Background(), roleTemplate)
+}
+
+// RoleAddContext adds role to VM Group
+// * ctx: context for cancelation
+// * roleTemplate: The new role content. Syntax can be the usual attribute=value or XML.
+func (vc *VMGroupController) RoleAddContext(ctx context.Context, roleTemplate string) error {
+	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.roleadd", vc.ID, roleTemplate)
+	return err
+}
+
+// RoleDelete deletes role from VM Group
+// * roleID: ID of the role to delete
+func (vc *VMGroupController) RoleDelete(roleID int) error {
+	return vc.RoleDeleteContext(context.Background(), roleID)
+}
+
+// RoleDeleteContext deletes role from VM Group
+// * roleID: ID of the role to delete
+func (vc *VMGroupController) RoleDeleteContext(ctx context.Context, roleID int) error {
+	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.roledelete", vc.ID, roleID)
+	return err
+}
+
+// RoleUpdate updates VM Group role
+// * ctx: context for cancelation
+// * roleID: ID of the role to update
+// * roleTemplate: The new role content. Syntax can be the usual attribute=value or XML.
+func (vc *VMGroupController) RoleUpdate(roleID int, roleTemplate string) error {
+	return vc.RoleUpdateContext(context.Background(), roleID, roleTemplate)
+}
+
+// RoleUpdateContext update VM Group role
+// * roleID: ID of the role to update
+// * roleTemplate: The new role content. Syntax can be the usual attribute=value or XML.
+func (vc *VMGroupController) RoleUpdateContext(ctx context.Context, roleID int, roleTemplate string) error {
+	_, err := vc.c.Client.CallContext(ctx, "one.vmgroup.roleupdate", vc.ID, roleID, roleTemplate)
+	return err
+}

src/oca/go/src/goca/vmgroup_test.go

@@ -0,0 +1,201 @@
+// +build !disabled
+
+/* -------------------------------------------------------------------------- */
+/* Copyright 2002-2023, OpenNebula Project, OpenNebula Systems                */
+/*                                                                            */
+/* Licensed under the Apache License, Version 2.0 (the "License"); you may    */
+/* not use this file except in compliance with the License. You may obtain    */
+/* a copy of the License at                                                   */
+/*                                                                            */
+/* http://www.apache.org/licenses/LICENSE-2.0                                 */
+/*                                                                            */
+/* Unless required by applicable law or agreed to in writing, software        */
+/* distributed under the License is distributed on an "AS IS" BASIS,          */
+/* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.   */
+/* See the License for the specific language governing permissions and        */
+/* limitations under the License.                                             */
+/*--------------------------------------------------------------------------- */
+
+package goca
+
+import (
+	//"testing"
+	//"strconv"
+
+	//ds "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/datastore"
+	//dskeys "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/datastore/keys"
+	//"github.com/OpenNebula/one/src/oca/go/src/goca/schemas/vmgroup"
+	//"github.com/OpenNebula/one/src/oca/go/src/goca/schemas/vm/keys"
+	"github.com/OpenNebula/one/src/oca/go/src/goca/schemas/shared"
+	. "gopkg.in/check.v1"
+)
+
+// Hook up gocheck into the "go test" runner.
+//func Test(t *testing.T) { TestingT(t) }
+
+type VMGroupSuite struct {
+	vmgID int
+}
+
+var _ = Suite(&VMGroupSuite{})
+
+func (s *VMGroupSuite) SetUpSuite(c *C) {
+}
+
+func (s *VMGroupSuite) SetUpTest(c *C) {
+	vmgTmpl := `NAME = "VM Group Test"
+		ROLE = [ NAME = web ]
+		ROLE = [ NAME = db, POLICY = "AFFINED"  ]
+		ROLE = [ NAME = app  ]
+		ROLE = [ NAME = bck  ]
+
+		AFFINED = "web, bck"
+		ANTI_AFFINED = "db"
+		`
+	vmgID, err := testCtrl.VMGroups().Create(vmgTmpl)
+	c.Assert(err, IsNil)
+	s.vmgID = vmgID
+}
+
+func (s *VMGroupSuite) TearDownTest(c *C) {
+	// Delete VM Group
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	vmgC.Delete()
+}
+
+func (s *VMGroupSuite) TearDownSuite(c *C) {
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+func (s *VMGroupSuite) TestCreated(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Name, Equals, "VM Group Test")
+
+	// Test roles
+	c.Assert(vmg.Roles[0].ID, Equals, 0)
+	c.Assert(vmg.Roles[0].Name, Equals, "web")
+	c.Assert(vmg.Roles[1].ID, Equals, 1)
+	c.Assert(vmg.Roles[1].Name, Equals, "db")
+	c.Assert(vmg.Roles[1].Policy, Equals, "AFFINED")
+	c.Assert(vmg.Roles[2].ID, Equals, 2)
+	c.Assert(vmg.Roles[2].Name, Equals, "app")
+	c.Assert(vmg.Roles[3].ID, Equals, 3)
+	c.Assert(vmg.Roles[3].Name, Equals, "bck")
+
+	// Get Backup Job by Name
+	id, err := testCtrl.VMGroups().ByName(vmg.Name)
+	c.Assert(err, IsNil)
+	c.Assert(id, Equals, s.vmgID)
+
+}
+
+func (s *VMGroupSuite) TestUpdate(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.Update("A=B", 1)
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	val, _ := vmg.Template.GetStr("A")
+	c.Assert(val, Equals, "B")
+}
+
+func (s *VMGroupSuite) TestChmod(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Permissions.String(), Equals, "um-------")
+
+	changedPerm := shared.Permissions{1, 1, 1, 1, 1, 1, 1, 1, 1}
+	vmgC.Chmod(changedPerm)
+
+	vmg, err = vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(*vmg.Permissions, Equals, changedPerm)
+}
+
+func (s *VMGroupSuite) TestChown(c *C) {
+	// Test only if the call exists, no real change
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.Chown(1, 1)
+
+	c.Assert(err, IsNil)
+}
+
+func (s *VMGroupSuite) TestRename(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.Rename("Renamed")
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Name, Equals, "Renamed")
+}
+
+func (s *VMGroupSuite) TestLock(c *C) {
+	// Lock
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.Lock(shared.LockUse)
+
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+
+	c.Assert(err, IsNil)
+	c.Assert(vmg.LockInfos.Locked, Equals, 1);
+
+	// Unlock
+	err = vmgC.Unlock()
+	c.Assert(err, IsNil)
+
+	vmg, err = vmgC.Info(false)
+
+	c.Assert(err, IsNil)
+	c.Assert(vmg.LockInfos, IsNil)
+}
+
+func (s *VMGroupSuite) TestRoleAdd(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.RoleAdd(`ROLE = [ name = "other" ]`)
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Roles[4].ID, Equals, 4)
+	c.Assert(vmg.Roles[4].Name, Equals, "other")
+}
+
+func (s *VMGroupSuite) TestRoleDelete(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.RoleDelete(2) // app
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Roles[2].ID, Equals, 3)
+	c.Assert(vmg.Roles[2].Name, Equals, "bck")
+}
+
+func (s *VMGroupSuite) TestRoleUpdate(c *C) {
+	vmgC := testCtrl.VMGroup(s.vmgID)
+	err := vmgC.RoleUpdate(2, `ROLE = [ POLICY = "ANTI_AFFINED" ]`)
+	c.Assert(err, IsNil)
+
+	vmg, err := vmgC.Info(false)
+	c.Assert(err, IsNil)
+
+	c.Assert(vmg.Roles[2].Name, Equals, "app")
+	c.Assert(vmg.Roles[2].Policy, Equals, "ANTI_AFFINED")
+}

src/oca/java/src/org/opennebula/client/vmgroup/VMGroup.java

@@ -37,6 +37,9 @@ public class VMGroup extends PoolElement{
     private static final String RENAME          = METHOD_PREFIX + "rename";
     private static final String LOCK            = METHOD_PREFIX + "lock";
     private static final String UNLOCK          = METHOD_PREFIX + "unlock";
+    private static final String ROLEADD         = METHOD_PREFIX + "roleadd";
+    private static final String ROLEDELETE      = METHOD_PREFIX + "roledelete";
+    private static final String ROLEUPDATE      = METHOD_PREFIX + "roleupdate";
 
     /**
      * Creates a new vmgroup representation.
@@ -238,6 +241,46 @@ public class VMGroup extends PoolElement{
         return client.call(UNLOCK, id);
     }
 
+    /**
+     * Add VM Group role
+     *
+     * @param client XML-RPC Client.
+     * @param id The id of the target VM Group.
+     * @param roleTemplate New template content.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public static OneResponse roleAdd(Client client, int id, String roleTemplate)
+    {
+        return client.call(ROLEADD, id, roleTemplate);
+    }
+
+    /**
+     * Delete VM Group role
+     *
+     * @param client XML-RPC Client.
+     * @param id The id of the target VM Group.
+     * @param roleID The id of the target role.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public static OneResponse roleDelete(Client client, int id, int roleID)
+    {
+        return client.call(ROLEDELETE, id, roleID);
+    }
+
+    /**
+     * Unlock this vmgroup
+     *
+     * @param client XML-RPC Client.
+     * @param id The id of the target VM Group.
+     * @param roleID The id of the target role.
+     * @param roleTemplate Updated template content.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public static OneResponse roleUpdate(Client client, int id, int roleID, String roleTemplate)
+    {
+        return client.call(ROLEUPDATE, id, roleID, roleTemplate);
+    }
+
     // =================================
     // Instanced object XML-RPC methods
     // =================================
@@ -400,6 +443,40 @@ public class VMGroup extends PoolElement{
         return unlock(client, id);
     }
 
+    /**
+     * Add VM Group role
+     *
+     * @param roleTemplate Updated template content.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public OneResponse roleAdd(String roleTemplate)
+    {
+        return roleAdd(client, id, roleTemplate);
+    }
+
+    /**
+     * Delete VM Group role
+     *
+     * @param roleID The id of the target role.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public OneResponse roleDelete(int roleID)
+    {
+        return roleDelete(client, id, roleID);
+    }
+
+    /**
+     * Update VM Group role
+     *
+     * @param roleID The id of the target role.
+     * @param roleTemplate Updated template content.
+     * @return If an error occurs the error message contains the reason.
+     */
+    public OneResponse roleUpdate(int roleID, String roleTemplate)
+    {
+        return roleUpdate(client, id, roleID, roleTemplate);
+    }
+
     // =================================
     // Helpers
     // =================================

src/oca/ruby/opennebula/vm_group.rb

@@ -31,7 +31,10 @@ module OpenNebula
             :chmod       => "vmgroup.chmod",
             :rename      => "vmgroup.rename",
             :lock        => "vmgroup.lock",
-            :unlock        => "vmgroup.unlock"
+            :unlock      => "vmgroup.unlock",
+            :roleadd     => "vmgroup.roleadd",
+            :roledelete  => "vmgroup.roledelete",
+            :roleupdate  => "vmgroup.roleupdate"
         }
 
         # Creates a VMGroup description with just its identifier
@@ -134,6 +137,37 @@ module OpenNebula
             return call(VMGROUP_METHODS[:rename], @pe_id, name)
         end
 
+        # Add role to VM Group
+        #
+        # @param template [String] String template for the new role
+        #
+        # @return [nil, OpenNebula::Error] nil in case of success, Error
+        #   otherwise
+        def role_add(template)
+            return call(VMGROUP_METHODS[:roleadd], @pe_id, template)
+        end
+
+        # Delete role from VM Group
+        #
+        # @param roleid [Integer] ID of the role to remove
+        #
+        # @return [nil, OpenNebula::Error] nil in case of success, Error
+        #   otherwise
+        def role_delete(roleid)
+            return call(VMGROUP_METHODS[:roledelete], @pe_id, roleid)
+        end
+
+        # Update VM Group role
+        #
+        # @param roleid [Integer] ID of the role to remove
+        # @param template [String] String template with updated values
+        #
+        # @return [nil, OpenNebula::Error] nil in case of success, Error
+        #   otherwise
+        def role_update(roleid, template)
+            return call(VMGROUP_METHODS[:roleupdate], @pe_id, roleid, template)
+        end
+
         #######################################################################
         # Helpers to get VMGroup information
         #######################################################################

src/rm/RequestManager.cc

@@ -50,6 +50,7 @@
 #include "RequestManagerMarketPlaceApp.h"
 #include "RequestManagerVirtualRouter.h"
 #include "RequestManagerSecurityGroup.h"
+#include "RequestManagerVMGroup.h"
 #include "RequestManagerVNTemplate.h"
 #include "RequestManagerHook.h"
 #include "RequestManagerMarketPlace.h"
@@ -557,6 +558,11 @@ void RequestManager::register_xml_methods()
     // Security Group methods
     xmlrpc_c::methodPtr secg_commit(new SecurityGroupCommit());
 
+    // VMGroup methods
+    xmlrpc_c::methodPtr vmg_addrole(new VMGroupAddRole());
+    xmlrpc_c::methodPtr vmg_delrole(new VMGroupDelRole());
+    xmlrpc_c::methodPtr vmg_updaterole(new VMGroupUpdateRole());
+
     // Hook methods
     xmlrpc_c::methodPtr hook_retry(new HookRetry());
 
@@ -980,6 +986,9 @@ void RequestManager::register_xml_methods()
     RequestManagerRegistry.addMethod("one.vmgroup.update",   vmg_update);
     RequestManagerRegistry.addMethod("one.vmgroup.lock",     vmg_lock);
     RequestManagerRegistry.addMethod("one.vmgroup.unlock",   vmg_unlock);
+    RequestManagerRegistry.addMethod("one.vmgroup.roleadd",  vmg_addrole);
+    RequestManagerRegistry.addMethod("one.vmgroup.roledelete", vmg_delrole);
+    RequestManagerRegistry.addMethod("one.vmgroup.roleupdate", vmg_updaterole);
 
     RequestManagerRegistry.addMethod("one.vmgrouppool.info", vmgpool_info);
 

src/rm/RequestManagerVMGroup.cc

@@ -0,0 +1,155 @@
+/* -------------------------------------------------------------------------- */
+/* Copyright 2002-2023, OpenNebula Project, OpenNebula Systems                */
+/*                                                                            */
+/* Licensed under the Apache License, Version 2.0 (the "License"); you may    */
+/* not use this file except in compliance with the License. You may obtain    */
+/* a copy of the License at                                                   */
+/*                                                                            */
+/* http://www.apache.org/licenses/LICENSE-2.0                                 */
+/*                                                                            */
+/* Unless required by applicable law or agreed to in writing, software        */
+/* distributed under the License is distributed on an "AS IS" BASIS,          */
+/* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.   */
+/* See the License for the specific language governing permissions and        */
+/* limitations under the License.                                             */
+/* -------------------------------------------------------------------------- */
+
+#include "RequestManagerVMGroup.h"
+
+using namespace std;
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+void VMGroupAddRole::request_execute(xmlrpc_c::paramList const& paramList,
+                                     RequestAttributes& att)
+{
+    int    oid      = paramList.getInt(1);
+    string tmpl_str = paramList.getString(2);
+
+    if (!basic_authorization(oid, att))
+    {
+        return;
+    }
+
+    Template tmpl;
+
+    if (tmpl.parse_str_or_xml(tmpl_str, att.resp_msg) != 0)
+    {
+        failure_response(ACTION, att);
+        return;
+    }
+
+    VectorAttribute * va = tmpl.get("ROLE");
+
+    if (!va)
+    {
+        att.resp_msg = "No ROLE attribute in template";
+
+        failure_response(ACTION, att);
+        return;
+    }
+
+    auto vmgroup = pool->get<VMGroup>(oid);
+
+    if (!vmgroup)
+    {
+        att.resp_id = oid;
+        failure_response(NO_EXISTS, att);
+
+        return;
+    }
+
+    if (vmgroup->add_role(va, att.resp_msg) != 0)
+    {
+        failure_response(ACTION, att);
+        return;
+    }
+
+    pool->update(vmgroup.get());
+
+    success_response(oid, att);
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+void VMGroupDelRole::request_execute(xmlrpc_c::paramList const& paramList,
+                                     RequestAttributes& att)
+{
+    int    oid      = paramList.getInt(1);
+    int    role_id  = paramList.getInt(2);
+
+    if (!basic_authorization(oid, att))
+    {
+        return;
+    }
+
+    auto vmgroup = pool->get<VMGroup>(oid);
+
+    if (!vmgroup)
+    {
+        att.resp_id = oid;
+        failure_response(NO_EXISTS, att);
+
+        return;
+    }
+
+    if (vmgroup->del_role(role_id, att.resp_msg) != 0)
+    {
+        failure_response(ACTION, att);
+        return;
+    }
+
+    pool->update(vmgroup.get());
+
+    success_response(oid, att);
+
+    return;
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+void VMGroupUpdateRole::request_execute(xmlrpc_c::paramList const& paramList,
+                                        RequestAttributes& att)
+{
+    int    oid      = paramList.getInt(1);
+    int    role_id  = paramList.getInt(2);
+    string tmpl_str = paramList.getString(3);
+
+    if (!basic_authorization(oid, att))
+    {
+        return;
+    }
+
+    Template tmpl;
+
+    if (tmpl.parse_str_or_xml(tmpl_str, att.resp_msg) != 0)
+    {
+        failure_response(ACTION, att);
+        return;
+    }
+
+    VectorAttribute * va = tmpl.get("ROLE");
+
+    if (!va)
+    {
+        att.resp_msg = "No ROLE attribute in template";
+
+        failure_response(ACTION, att);
+        return;
+    }
+
+    auto vmgroup = pool->get<VMGroup>(oid);
+
+    if (vmgroup->update_role(role_id, va, att.resp_msg) != 0)
+    {
+        failure_response(ACTION, att);
+        return;
+    }
+
+    pool->update(vmgroup.get());
+
+    success_response(oid, att);
+}

src/rm/SConstruct

@@ -52,6 +52,7 @@ source_files=[
     'RequestManagerMarketPlaceApp.cc',
     'RequestManagerVirtualRouter.cc',
     'RequestManagerSecurityGroup.cc',
+    'RequestManagerVMGroup.cc',
     'RequestManagerVNTemplate.cc',
     'RequestManagerHook.cc',
     'RequestManagerBackupJob.cc',

src/vm_group/VMGroup.cc

@@ -64,7 +64,7 @@ string& VMGroup::to_xml(string& xml) const
         "<NAME>"    << name     << "</NAME>"   <<
         perms_to_xml(perms_xml)                <<
         lock_db_to_xml(lock_str)               <<
-        roles.to_xml(roles_xml)                <<
+        _roles.to_xml(roles_xml)               <<
         obj_template->to_xml(template_xml)     <<
     "</VM_GROUP>";
 
@@ -114,7 +114,7 @@ int VMGroup::from_xml(const string &xml_str)
 
     if (!content.empty())
     {
-        rc += roles.from_xml_node(content[0]);
+        rc += _roles.from_xml_node(content[0]);
     }
 
     ObjectXML::free_nodes(content);
@@ -241,7 +241,7 @@ int VMGroup::check_rule_names(VMGroupPolicy policy, std::string& error)
     {
         std::set<int> id_set;
 
-        if ( roles.names_to_ids(sattr->value(), id_set) != 0 )
+        if ( _roles.names_to_ids(sattr->value(), id_set) != 0 )
         {
             oss.str("");
 
@@ -276,7 +276,7 @@ int VMGroup::get_rules(VMGroupPolicy policy, VMGroupRule::rule_set& rules,
     {
         std::set<int> id_set;
 
-        roles.names_to_ids(sattr->value(), id_set);
+        _roles.names_to_ids(sattr->value(), id_set);
 
         VMGroupRule rule(policy, id_set);
 
@@ -301,6 +301,125 @@ int VMGroup::get_rules(VMGroupPolicy policy, VMGroupRule::rule_set& rules,
 /* -------------------------------------------------------------------------- */
 /* -------------------------------------------------------------------------- */
 
+int VMGroup::add_role(VectorAttribute * vrole, std::string& error)
+{
+    if (_roles.add_role(vrole, error) != 0)
+    {
+        return -1;
+    }
+
+    return check_consistency(error);
+}
+
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+int VMGroup::del_role(int role_id, std::string& error)
+{
+    auto role = _roles.get(role_id);
+
+    if ( !role )
+    {
+        ostringstream oss;
+        oss << "Unable to delete role " << role_id << ", the role doesn't exists";
+
+        error = oss.str();
+
+        return -1;
+    }
+
+    if (role->size_vms() > 0)
+    {
+        ostringstream oss;
+        oss << "Unable to delete role " << role_id << ", the role has assigned VMs ";
+
+        error = oss.str();
+
+        return -1;
+    }
+
+    _roles.del_role(role_id);
+
+    return check_consistency(error);
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+int VMGroup::update_role(int role_id, VectorAttribute * vrole, std::string& error)
+{
+    // Create method VMGroup::role_update and do all checks there
+    auto role = _roles.get(role_id);
+
+    if (!role)
+    {
+        ostringstream oss;
+        oss << "VMGroup " << oid << " doesn't have role " << role_id;
+
+        error = oss.str();
+
+        return -1;
+    }
+
+    if (role->size_vms() > 0)
+    {
+        ostringstream oss;
+        oss << "VMGroup " << oid << " Role " << role_id
+            << " has VMs, unable to update";
+
+        error = oss.str();
+
+        return -1;
+    }
+
+    string new_name = vrole->vector_value("NAME");
+    if (!new_name.empty() && new_name != role->name())
+    {
+        if (_roles.rename_role(role, new_name) != 0)
+        {
+            ostringstream oss;
+            oss << "VMGroup " << oid << " Role " << role_id
+                << " can not update name, role with name " << new_name
+                << " already exists";
+
+            error = oss.str();
+
+            return -1;
+        }
+    }
+
+    role->update(vrole);
+
+    return check_consistency(error);
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+int VMGroup::check_consistency(std::string& error_str)
+{
+    if ( check_rule_names(VMGroupPolicy::AFFINED, error_str) == -1 )
+    {
+        return -1;
+    }
+
+    if ( check_rule_names(VMGroupPolicy::ANTI_AFFINED, error_str) == -1 )
+    {
+        return -1;
+    }
+
+    if ( check_rule_consistency(error_str) == -1 )
+    {
+        return -1;
+    }
+
+    return 0;
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
 int VMGroup::check_rule_consistency(std::string& error)
 {
     VMGroupRule::rule_set affined, anti;
@@ -320,7 +439,7 @@ int VMGroup::check_rule_consistency(std::string& error)
         {
             if ( rs[i] == 1 )
             {
-                VMGroupRole * role = roles.get(i);
+                VMGroupRole * role = _roles.get(i);
 
                 if ( role != 0 && role->policy() == VMGroupPolicy::ANTI_AFFINED )
                 {
@@ -349,7 +468,7 @@ int VMGroup::check_rule_consistency(std::string& error)
         {
             if ( rs[i] == 1 )
             {
-                VMGroupRole * role = roles.get(i);
+                VMGroupRole * role = _roles.get(i);
 
                 if ( role != 0 )
                 {
@@ -404,7 +523,7 @@ int VMGroup::insert(SqlDB *db, string& error_str)
             continue;
         }
 
-        if ( roles.add_role(vatt, error_str) == -1 )
+        if ( _roles.add_role(vatt, error_str) == -1 )
         {
             delete attr;
             error = true;
@@ -416,17 +535,7 @@ int VMGroup::insert(SqlDB *db, string& error_str)
         return -1;
     }
 
-    if ( check_rule_names(VMGroupPolicy::AFFINED, error_str) == -1 )
+    if ( check_consistency(error_str) != 0)
-    {
-        return -1;
-    }
-
-    if ( check_rule_names(VMGroupPolicy::ANTI_AFFINED, error_str) == -1 )
-    {
-        return -1;
-    }
-
-    if ( check_rule_consistency(error_str) == -1 )
     {
         return -1;
     }
@@ -444,7 +553,7 @@ int VMGroup::insert(SqlDB *db, string& error_str)
 
 int VMGroup::post_update_template(string& error)
 {
-    int vms = roles.vm_size();
+    int vms = _roles.vm_size();
 
     if ( vms > 0 )
     {
@@ -458,17 +567,7 @@ int VMGroup::post_update_template(string& error)
 
     obj_template->erase("ROLE");
 
-    if ( check_rule_names(VMGroupPolicy::AFFINED, error) == -1 )
+    if ( check_consistency(error) != 0 )
-    {
-        return -1;
-    }
-
-    if ( check_rule_names(VMGroupPolicy::ANTI_AFFINED, error) == -1 )
-    {
-        return -1;
-    }
-
-    if ( check_rule_consistency(error) == -1 )
     {
         return -1;
     }

src/vm_group/VMGroupRole.cc

@@ -63,6 +63,24 @@ VMGroupPolicy VMGroupRole::policy()
 /* -------------------------------------------------------------------------- */
 /* -------------------------------------------------------------------------- */
 
+void VMGroupRole::to_xml(ostringstream &oss) const
+{
+    va->to_xml(oss);
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+void VMGroupRole::update(VectorAttribute* va_update)
+{
+    va_update->remove("VMS"); // Do not allow to update VMS, it's system attribute
+
+    va->merge(va_update, true);
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
 void VMGroupRole::add_vm(int vm_id)
 {
     auto rc = vms.insert(vm_id);
@@ -75,6 +93,9 @@ void VMGroupRole::add_vm(int vm_id)
     set_vms();
 }
 
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
 void VMGroupRole::del_vm(int vm_id)
 {
     size_t rc = vms.erase(vm_id);
@@ -87,6 +108,9 @@ void VMGroupRole::del_vm(int vm_id)
     set_vms();
 }
 
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
 void VMGroupRole::set_vms()
 {
     if ( vms.empty() )
@@ -223,16 +247,36 @@ void VMGroupRole::antiaffined_host_requirements(std::string& reqs)
 /* -------------------------------------------------------------------------- */
 /* -------------------------------------------------------------------------- */
 
+string& VMGroupRoles::to_xml(std::string& xml_str) const
+{
+    ostringstream oss;
+
+    oss << "<" << "ROLES" << ">";
+
+    for ( auto it : by_id )
+    {
+        it.second->to_xml(oss);
+    }
+
+    oss << "</" << "ROLES" << ">";
+
+    xml_str = oss.str();
+
+    return xml_str;
+}
+
 int VMGroupRoles::from_xml_node(const xmlNodePtr node)
 {
     std::vector<VectorAttribute *> roles;
 
-    if ( roles_template.from_xml_node(node) == -1 )
+    Template templ;
+
+    if ( templ.from_xml_node(node) == -1 )
     {
         return -1;
     }
 
-    roles_template.get("ROLE", roles);
+    templ.get("ROLE", roles);
 
     for (auto vattr : roles)
     {
@@ -246,26 +290,20 @@ int VMGroupRoles::from_xml_node(const xmlNodePtr node)
             return -1;
         }
 
+        if (by_id.find(rid) != by_id.end() || by_name.find(rname) != by_name.end())
+        {
+            return -1;
+        }
+
         if ( rid >= next_role )
         {
             next_role = rid + 1;
         }
 
-        VMGroupRole * role = new VMGroupRole(vattr);
+        VMGroupRole * role = new VMGroupRole(vattr->clone());
 
-        if ( by_id.insert(rid, role) == false )
+        by_id.insert(make_pair(rid, role));
-        {
+        by_name.insert(make_pair(rname, role));
-            delete role;
-            return -1;
-        }
-
-        if ( by_name.insert(rname, role) == false )
-        {
-            by_id.erase(rid);
-
-            delete role;
-            return -1;
-        }
     }
 
     return 0;
@@ -289,29 +327,55 @@ int VMGroupRoles::add_role(VectorAttribute * vrole, string& error)
 
     vrole->remove("VMS");
 
-    VMGroupRole * role = new VMGroupRole(vrole);
+    if (by_id.find(next_role) != by_id.end() || by_name.find(rname) != by_name.end())
-
-    if ( by_id.insert(next_role, role) == false )
     {
-        delete role;
+        error = "Role already exists";
 
-        error = "Role ID already exists";
         return -1;
     }
 
-    if ( by_name.insert(rname, role) == false )
+    VMGroupRole * role = new VMGroupRole(vrole->clone());
-    {
-        by_id.erase(next_role);
 
-        delete role;
+    by_id.insert(make_pair(next_role, role));
-
+    by_name.insert(make_pair(rname, role));
-        error = "Role NAME already exists";
-        return -1;
-    }
 
     next_role += 1;
 
-    roles_template.set(vrole);
+    return 0;
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+void VMGroupRoles::del_role(int id)
+{
+    auto role = get(id);
+
+    if ( !role )
+    {
+        return;
+    }
+
+    by_name.erase(role->name());
+    by_id.erase(id);
+
+    delete role;
+}
+
+/* -------------------------------------------------------------------------- */
+/* -------------------------------------------------------------------------- */
+
+int VMGroupRoles::rename_role(VMGroupRole* role, const std::string& new_name)
+{
+    if (by_name.find(new_name) != by_name.end())
+    {
+        return -1;
+    }
+
+    by_name.erase(role->name());
+    by_name[new_name] = role;
+
+    role->name(new_name);
 
     return 0;
 }
@@ -321,11 +385,9 @@ int VMGroupRoles::add_role(VectorAttribute * vrole, string& error)
 
 int VMGroupRoles::add_vm(const std::string& role_name, int vmid)
 {
-    VMGroupRole * role;
+    VMGroupRole * role = get(role_name);
 
-    role = by_name.get(role_name);
+    if ( !role )
-
-    if ( role == 0 )
     {
         return -1;
     }
@@ -340,11 +402,9 @@ int VMGroupRoles::add_vm(const std::string& role_name, int vmid)
 
 int VMGroupRoles::del_vm(const std::string& role_name, int vmid)
 {
-    VMGroupRole * role;
+    VMGroupRole * role = get(role_name);
 
-    role = by_name.get(role_name);
+    if ( !role )
-
-    if ( role == 0 )
     {
         return -1;
     }
@@ -361,9 +421,9 @@ int VMGroupRoles::vm_size()
 {
     int total = 0;
 
-    for ( auto it = begin(); it != end() ; ++it )
+    for ( auto it : by_id )
     {
-        total += (*it)->get_vms().size();
+        total += it.second->get_vms().size();
     }
 
     return total;
@@ -385,9 +445,9 @@ int VMGroupRoles::names_to_ids(const std::string& rnames, std::set<int>&  keyi)
 
     for ( const auto& name : key_set )
     {
-        VMGroupRole *r = by_name.get(name);
+        VMGroupRole *r = get(name);
 
-        if ( r == 0 )
+        if ( !r )
         {
             keyi.clear();
             return -1;