Fixes #26017 - treat tar packing response code 1 as success #253
Conversation
|
Issues: #26017 |
|
Thanks @mbacovsky for the original idea. |
| @@ -20,7 +20,7 @@ def run | |||
| configs = config_files.join(' ') | |||
| execute!("tar --selinux --create --gzip --file=#{tarball} " \ | |||
| "--listed-incremental=#{increments} --ignore-failed-read " \ | |||
| "#{configs}") | |||
| "#{configs}", :valid_exit_statuses => [0, 1]) | |||
There was a problem hiding this comment.
@pmoravec Is it safe? I don't care much about online backup. But it could let some inconsistency sneak in offline backup. I guess that doing config backup after we stop services (#248) might help but still there is a chance of something unexpected.
Could we make this change optional by a new parameter that is set for online backups only?
Does it make sense or I'm I too paranoid?
There was a problem hiding this comment.
It makes sense to be tolerant to changed configs only on online backups - will update the PR.
(that's why I am not a great developer, I fix just my (broken) call flow)
pmoravec
force-pushed
the
online_backup_tar_return_code_1_is_ok
branch
from
ba99f74
to
22e1c49
Compare
|
PR updated. |
|
Anything is pending here, @mbacovsky ? I see similar problem (online backup fails the same way) in #248 is fixed, so let merge this one as well to have this kind of issues (and rhbz 1673908) resolved :) . |
| @@ -11,16 +11,18 @@ class ConfigFiles < ForemanMaintain::Procedure | |||
| param :backup_dir, 'Directory where to backup to', :required => true | |||
| param :proxy_features, 'List of proxy features to backup (default: all)', | |||
| :array => true, :default => ['all'] | |||
| param :strategy, 'Backup strategy' | |||
There was a problem hiding this comment.
This could be renamed to :ignore_changed_files or similar with :flag => true, :default => false options
definitions/scenarios/backup.rb
Outdated
| @@ -69,6 +69,8 @@ def set_context_mapping | |||
| Procedures::Backup::Metadata => :incremental_dir) | |||
| context.map(:proxy_features, | |||
| Procedures::Backup::ConfigFiles => :proxy_features) | |||
| context.map(:strategy, | |||
There was a problem hiding this comment.
The context map should be replaced by passing the :ignore_changed_files param in compose e.g.
add_step_with_context(Procedures::Backup::ConfigFiles, :ignore_changed_files => true)in add_online_backup_steps.
pmoravec
force-pushed
the
online_backup_tar_return_code_1_is_ok
branch
2 times, most recently
from
2f98221
to
9666c8e
Compare
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
pmoravec
force-pushed
the
online_backup_tar_return_code_1_is_ok
branch
from
9666c8e
to
8a2320e
Compare
|
Thanks for feedback, PR updated (and rebased to upstream to resolve conflicts with #248). |
During online backup, tar packages files changing on the fly. It returns 1 if detecting so, while this is not a fatal error for the backup. Therefore we should treat response code 1 as success. Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
pmoravec
force-pushed
the
online_backup_tar_return_code_1_is_ok
branch
from
8a2320e
to
fbb41a7
Compare
During online backup, tar packages files changing on the fly.
It returns 1 if detecting so, while this is not a fatal error
for the backup. Therefore we should treat response code 1 as
success.
Signed-off-by: Pavel Moravec pmoravec@redhat.com