From c5392d08b8db2e4c50b408b5d7bfb4ba53666434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Czern=C3=BD?= Date: Thu, 7 Sep 2023 11:11:02 +0200 Subject: [PATCH] F #6063: Update Backup Job fix (#2715) * Set priority on Backup Job create * Fix a bug when running backup jobs in sequential mode * Change the update semantics to support replace mode * Update Ruby, Golang and Java API accordingly * F #6063: Adress PR comments --- include/BackupJob.h | 17 ++++++ include/RequestManagerUpdateTemplate.h | 9 --- src/cli/onebackupjob | 7 ++- src/oca/go/src/goca/backupjob.go | 5 +- src/oca/go/src/goca/backupjob_test.go | 3 +- .../org/opennebula/client/bj/BackupJob.java | 10 ++-- src/oca/ruby/opennebula/backupjob.rb | 4 +- src/rm/RequestManagerAllocate.cc | 15 +++++ src/sam/ScheduledActionManager.cc | 8 ++- src/vm/BackupJob.cc | 57 +++++++++++++------ 10 files changed, 94 insertions(+), 41 deletions(-) diff --git a/include/BackupJob.h b/include/BackupJob.h index 9facdc0e8d..061c788432 100644 --- a/include/BackupJob.h +++ b/include/BackupJob.h @@ -232,6 +232,19 @@ public: return _sched_actions; } + /** + * Replace template for this object. Object should be updated + * after calling this method + * @param tmpl_str new contents + * @param keep_restricted If true, the restricted attributes of the + * current template will override the new template + * @param error string describing the error if any + * @return 0 on success + */ + int replace_template(const std::string& tmpl_str, + bool keep_restricted, + std::string& error) override; + /** * Append new attributes to the *user template*. * @param tmpl_str new contents @@ -255,6 +268,10 @@ protected: */ int post_update_template(std::string& error) override { + remove_template_attribute("NAME"); + remove_template_attribute("PRIORITY"); + remove_template_attribute("SCHED_ACTION"); + return parse(error); } diff --git a/include/RequestManagerUpdateTemplate.h b/include/RequestManagerUpdateTemplate.h index 6b926678c3..19ee988d50 100644 --- a/include/RequestManagerUpdateTemplate.h +++ b/include/RequestManagerUpdateTemplate.h @@ -488,15 +488,6 @@ public: pool = nd.get_bjpool(); auth_object = PoolObjectSQL::BACKUPJOB; } - - //Always append to BackupJob templates (not duplicated attributes) - int replace_template(PoolObjectSQL * object, - const std::string & tmpl, - const RequestAttributes &att, - std::string &error_str) override - { - return RequestManagerUpdateTemplate::append_template(object, tmpl, att, error_str); - } }; #endif diff --git a/src/cli/onebackupjob b/src/cli/onebackupjob index 54bf33abb3..7c5269d386 100755 --- a/src/cli/onebackupjob +++ b/src/cli/onebackupjob @@ -177,10 +177,13 @@ CommandParser::CmdParser.new(ARGV) do be launched to modify the current content. EOT - command :update, update_desc, :backupjobid, [:file, nil] do + command :update, update_desc, :backupjobid, [:file, nil], + :options => OpenNebulaHelper::APPEND do helper.perform_action(args[0], options, 'modified') do |obj| if args[1] str = File.read(args[1]) + elsif options[:append] + OpenNebulaHelper.editor_input else rc = obj.info @@ -194,7 +197,7 @@ CommandParser::CmdParser.new(ARGV) do str = OpenNebulaHelper.editor_input(obj.template_like_str('TEMPLATE')) end - obj.update(str) + obj.update(str, options[:append]) end end diff --git a/src/oca/go/src/goca/backupjob.go b/src/oca/go/src/goca/backupjob.go index 11ca404f15..e169cbab89 100644 --- a/src/oca/go/src/goca/backupjob.go +++ b/src/oca/go/src/goca/backupjob.go @@ -22,6 +22,7 @@ import ( "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/backupjob" "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/shared" + "github.com/OpenNebula/one/src/oca/go/src/goca/parameters" ) // BackupJobsController is a controller for Backup Jobs @@ -116,8 +117,8 @@ func (bjc *BackupJobsController) Create(template string) (int, error) { // Update adds Backup Job content. // * tpl: The new Backup Job content. Syntax can be the usual attribute=value or XML. -func (bjc *BackupJobController) Update(tpl string) error { - _, err := bjc.c.Client.Call("one.backupjob.update", bjc.ID, tpl) +func (bjc *BackupJobController) Update(tpl string, uType parameters.UpdateType) error { + _, err := bjc.c.Client.Call("one.backupjob.update", bjc.ID, tpl, uType) return err } diff --git a/src/oca/go/src/goca/backupjob_test.go b/src/oca/go/src/goca/backupjob_test.go index 69392f5a98..23b2a4a848 100644 --- a/src/oca/go/src/goca/backupjob_test.go +++ b/src/oca/go/src/goca/backupjob_test.go @@ -19,6 +19,7 @@ package goca import ( "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/backupjob/keys" "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/shared" + "github.com/OpenNebula/one/src/oca/go/src/goca/parameters" . "gopkg.in/check.v1" ) @@ -66,7 +67,7 @@ func (s *BJSuite) TestGetByNameAndID(c *C) { func (s *BJSuite) TestUpdate(c *C) { bjC := testCtrl.BackupJob(s.bjID) - err := bjC.Update(`BACKUP_VMS = "1,2,3,4"`) + err := bjC.Update(`BACKUP_VMS = "1,2,3,4"`, parameters.Merge) c.Assert(err, IsNil) diff --git a/src/oca/java/src/org/opennebula/client/bj/BackupJob.java b/src/oca/java/src/org/opennebula/client/bj/BackupJob.java index bc26d01a29..843ca65d54 100644 --- a/src/oca/java/src/org/opennebula/client/bj/BackupJob.java +++ b/src/oca/java/src/org/opennebula/client/bj/BackupJob.java @@ -114,11 +114,12 @@ public class BackupJob extends PoolElement * @param client XML-RPC Client. * @param id The Backkup Job ID we want to modify. * @param new_template New template contents + * @param append True to append new attributes instead of replace the whole template * @return If successful the message contains the Backup Job ID. */ - public static OneResponse update(Client client, int id, String new_template) + public static OneResponse update(Client client, int id, String new_template, boolean append) { - return client.call(UPDATE, id, new_template); + return client.call(UPDATE, id, new_template, append ? 1 : 0); } /** @@ -348,11 +349,12 @@ public class BackupJob extends PoolElement * Replaces the template contents. * * @param new_template New template contents + * @param append True to append new attributes instead of replace the whole template * @return If successful the message contains the Backup Job ID. */ - public OneResponse update(String new_template) + public OneResponse update(String new_template, boolean append) { - return update(client, id, new_template); + return update(client, id, new_template, append); } /** diff --git a/src/oca/ruby/opennebula/backupjob.rb b/src/oca/ruby/opennebula/backupjob.rb index 3a84529fcf..0cfc1874e8 100644 --- a/src/oca/ruby/opennebula/backupjob.rb +++ b/src/oca/ruby/opennebula/backupjob.rb @@ -121,8 +121,8 @@ module OpenNebula # # @return [nil, OpenNebula::Error] nil in case of success, Error # otherwise - def update(new_template) - super(BACKUPJOB_METHODS[:update], new_template) + def update(new_template, append = false) + super(BACKUPJOB_METHODS[:update], new_template, append ? 1 : 0) end # Changes the owner/group diff --git a/src/rm/RequestManagerAllocate.cc b/src/rm/RequestManagerAllocate.cc index 0891b4e276..8e195ef70c 100644 --- a/src/rm/RequestManagerAllocate.cc +++ b/src/rm/RequestManagerAllocate.cc @@ -1466,6 +1466,21 @@ Request::ErrorCode BackupJobAllocate::pool_allocate( tmpl->remove("SCHED_ACTION", sas); + int priority; + if (tmpl->get("PRIORITY", priority)) + { + if (priority < BackupJob::MIN_PRIO || priority > BackupJob::MAX_PRIO) + { + att.resp_msg = "Wrong priority value"; + return Request::INTERNAL; + } + + if (!att.is_admin() && priority > 50) + { + return Request::AUTHORIZATION; + } + } + /* ---------------------------------------------------------------------- */ /* Create BackupJob object */ /* ---------------------------------------------------------------------- */ diff --git a/src/sam/ScheduledActionManager.cc b/src/sam/ScheduledActionManager.cc index e95dfdba48..903ac3461a 100644 --- a/src/sam/ScheduledActionManager.cc +++ b/src/sam/ScheduledActionManager.cc @@ -328,11 +328,10 @@ void ScheduledActionManager::backup_jobs() continue; } - // Copy (not reference) of outdated and backing_up vms (modified in loop) auto vms = bj->backup_vms(); - auto outdated = bj->outdated(); - auto backing_up = bj->backing_up(); + auto& outdated = bj->outdated(); + auto& backing_up = bj->backing_up(); auto mode = bj->exec_mode(); auto ds_id = bj->ds_id(); @@ -465,6 +464,9 @@ void ScheduledActionManager::backup_jobs() GroupPool::ONEADMIN_ID, PoolObjectSQL::VM); + NebulaLog::info("SCH", "Backup Job " + to_string(bj_id) + + " executing backup for VM " + to_string(vm_id) ); + auto ec = vm_backup.request_execute(ra, vm_id, ds_id, reset); if ( ec != Request::SUCCESS) diff --git a/src/vm/BackupJob.cc b/src/vm/BackupJob.cc index fa432057c1..5b5e190688 100644 --- a/src/vm/BackupJob.cc +++ b/src/vm/BackupJob.cc @@ -466,6 +466,32 @@ void BackupJob::get_backup_config(Template &tmpl) /* -------------------------------------------------------------------------- */ /* -------------------------------------------------------------------------- */ +int BackupJob::replace_template( + const string& tmpl_str, + bool keep_restricted, + string& error) +{ + auto new_tmpl = make_unique(false,'=',"USER_TEMPLATE"); + string new_str, backup_vms; + + if ( new_tmpl->parse_str_or_xml(tmpl_str, error) != 0 ) + { + return -1; + } + + // Store old BACKUP_VMS value, to detect changes in post_update_template + get_template_attribute("BACKUP_VMS", backup_vms); + + new_tmpl->replace("BACKUP_VMS_OLD", backup_vms); + + new_tmpl->to_xml(new_str); + + return PoolObjectSQL::replace_template(new_str, keep_restricted, error); +} + +/* -------------------------------------------------------------------------- */ +/* -------------------------------------------------------------------------- */ + int BackupJob::append_template( const string& tmpl_str, bool keep_restricted, @@ -505,6 +531,7 @@ int BackupJob::parse(string& error) // - FSFREEZE // - MODE // - EXECUTION + // - PRIORITY // ------------------------------------------------------------------------- if ( erase_template_attribute("KEEP_LAST", iattr) == 0 || iattr < 0 ) { @@ -558,7 +585,11 @@ int BackupJob::parse(string& error) add_template_attribute("EXECUTION", execution_to_str(exec)); - erase_template_attribute("PRIORITY", iattr); + if ( erase_template_attribute("PRIORITY", iattr) > 0 && + iattr >= MIN_PRIO && iattr <= MAX_PRIO ) + { + _priority = iattr; + } // ------------------------------------------------------------------------- // VMs part of this backup job. Order represents backup order. @@ -567,8 +598,6 @@ int BackupJob::parse(string& error) if ( erase_template_attribute("BACKUP_VMS", sattr) != 0 ) { - string sattr_old; - one_util::split(sattr, ',', vms); auto last = std::unique(vms.begin(), vms.end()); @@ -576,13 +605,6 @@ int BackupJob::parse(string& error) vms.erase(last, vms.end()); sattr = one_util::join(vms, ','); - - erase_template_attribute("BACKUP_VMS_OLD", sattr_old); - - if (process_backup_vms(sattr, sattr_old, error) != 0) - { - return -1; - } } else { @@ -591,15 +613,14 @@ int BackupJob::parse(string& error) add_template_attribute("BACKUP_VMS", sattr); - if (sattr.empty()) - { - return 0; - } + string sattr_old; - // ------------------------------------------------------------------------- - // Remove SCHED_ACTION, it can be updated only by Scheduled Actions API - // ------------------------------------------------------------------------- - remove_template_attribute("SCHED_ACTION"); + erase_template_attribute("BACKUP_VMS_OLD", sattr_old); + + if (process_backup_vms(sattr, sattr_old, error) != 0) + { + return -1; + } // ------------------------------------------------------------------------- // Update collections if VMs are no longer in BACKUP_VM