Fixes #27576 - config files backup fix #277

upadhyeammit · 2019-08-13T12:16:29Z

This PR adds,

There is MAX_RETRIES constant which is max occurrences config file backup will be tried.
The create_tarball method which actually handles,
- This will loop 3 times at max(MAX_RETRIES) if exit_status of the tar command does not include in statutes. The loop terminates when it finds exit_status in statuses array.
- I am exiting the loop as per statuses because, I think we should only retry collecting tar when ignore_changed_files is false([0]), else it means user wants to continue execution even when tar is incomplete.
- If the tar fails at third attempt I am not sure if I should skip deleting the tar archive or not ? current implementation deletes it because in this case user expects tar should be complete one.
- It shows warning using warn! when the last time(3) tar command fails with exit status 1. This means the backup fails with error message.
I have separated few methods to make it more simpler to understand.

theforeman-bot · 2019-08-13T12:16:37Z

upadhyeammit · 2019-08-13T12:44:20Z

Removed unnecessary && condition from below one as its not required,

warn! 'Config files backup failed' if MAX_RETRIES == ret && !@ignore_changed_files

kgaikwad

Thank you @upadhyeammit!

Added few comments. I haven't tested the changes yet.

kgaikwad · 2019-08-14T09:44:50Z

definitions/procedures/backup/config_files.rb

+    def create_tarball
+      (1..MAX_RETRIES).each do |ret|
+        exit_status = run_tar_cmd
+        break unless statuses.include? exit_status


exit_status is used only in condition then why not to add run_tar_cmd directly?

Suggested change

break unless statuses.include? exit_status

break unless statuses.include? run_tar_cmd

Could you rename method run_tar_cmd to execute_tar_cmd?

Yes we can, but I think we should keep exit_status so its good to understand that we are looking exit_status in array ? maybe I am wrong !
Recent commit keeps exit_status, request to add suggestions !

@upadhyeammit,

Could you please give specific name to statuses method?
Example - allowed_exit_statuses or allowed_exit_codes.

As per me then after you can directly use method execute_tar_cmd in the condition.

definitions/procedures/backup/config_files.rb

kgaikwad · 2019-08-14T11:02:19Z

definitions/procedures/backup/config_files.rb

+        exit_status = run_tar_cmd
+        break unless statuses.include? exit_status
+
+        warn "\nRemoving config files archive #{tarball} as its incomplete"


Are we going to show warnings on every run if one fails?

As I haven't tested it but I would like to see difference between two outputs i.e using spinner.update and using warn.

The current output with warn and warn!,

# bin/foreman-maintain advanced procedure run backup-config-files --backup-dir /tmp/ Running ForemanMaintain::Scenario ================================================================================ Backup config files: / Collecting config files to backup Removing config files archive /tmp/config_files.tar.gz as its incomplete Recollecting config files backup, retry 1 ! \ Collecting config files to backup Removing config files archive /tmp/config_files.tar.gz as its incomplete Recollecting config files backup, retry 2 ! / Collecting config files to backup Removing config files archive /tmp/config_files.tar.gz as its incomplete Recollecting config files backup, retry 3 !

If I use the spinner.update for "Removing config files archive /tmp/config_files.tar.gz as its incomplete"

# bin/foreman-maintain advanced procedure run backup-config-files --backup-dir /tmp/ Running ForemanMaintain::Scenario ================================================================================ Backup config files: \ Removing config files archive /tmp/config_files.tar.gz as its incompleteRecollecting config files backup, retry 1 ! \ | Removing config files archive /tmp/config_files.tar.gz as its incompleteRecollecting config files backup, retry 2 ! | / - Removing config files archive /tmp/config_files.tar.gz as its incompleteRecollecting config files backup, retry 3 !

As per this I thought of using it for "Recollecting config files backup, retry 3 !"

# bin/foreman-maintain advanced procedure run backup-config-files --backup-dir /tmp/ Running ForemanMaintain::Scenario ================================================================================ Backup config files: - - - \ | | | / - - Recollecting config files backup, retry 3 ! [FAIL]

One more thing here is if I use spinner.update instead of warn! when MAX_RETRIES== ret ; the run show status as OK instead of FAILED, I tried to forcefully fail it by setting @status= :fail, @status =1 but it did not help to make it fail. If I am missing anything here then request to elaborate on same ?

warn! 'Config files backup failed' if MAX_RETRIES == ret

I preferred to show warning at each run as its good to understand that we are deleting the failed backup and retrying again, so user is not confused anywhere.

kgaikwad · 2019-08-14T11:16:00Z

@upadhyeammit,

In PR-269, @mbacovsky made code changes in the config_files procedure. My suggestion would be that you could add code changes once his PR get merged.

upadhyeammit · 2019-08-19T04:31:30Z

@mbacovsky I still feel that we should have sleep in-between each failed run because even for 3 runs it completes swiftly and so I feel this code wont add any value because config files wont be committed to disk. Maybe we can start with sleep = 2 seconds and double that for every failed run ? like 2, 4 and 8 seconds ? or maybe something else ?

theforeman-bot · 2019-08-19T04:35:50Z

There were the following issues with the commit message:

1ccffc4 must not be authored by vagrant or root user

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

kgaikwad · 2019-08-27T11:27:04Z

@upadhyeammit, could you please rebase and update the pull-request.

upadhyeammit · 2019-08-30T10:38:40Z

definitions/procedures/backup/config_files.rb

+    end
+
+    def statuses
+      @ignore_changed_files ? [0, 1] : [0]


I can see tar feature has same statuses assignment, I have kept this one as its single line method. To call it from feature tar need to create method in that first, if it has benefit then will do changes accordingly.

upadhyeammit · 2019-08-30T10:44:25Z

@upadhyeammit, could you please rebase and update the pull-request.

done, request to check. Thank You !

upadhyeammit · 2019-08-30T10:44:48Z

@mbacovsky rebased this one, request to have a look once. Thank You !

kgaikwad · 2019-09-03T08:44:45Z

definitions/procedures/backup/config_files.rb

+    def create_tarball
+      (1..MAX_RETRIES).each do |ret|
+        exit_status = run_tar_cmd
+        break unless statuses.include? exit_status


@upadhyeammit,

Could you please give specific name to statuses method?
Example - allowed_exit_statuses or allowed_exit_codes.

As per me then after you can directly use method execute_tar_cmd in the condition.

upadhyeammit · 2019-09-05T09:01:23Z

@kgaikwad changed method names ! request to check.

kgaikwad · 2019-09-05T09:20:55Z

definitions/procedures/backup/config_files.rb

+    def execute_tar_cmd
+      @tarball_path ||= File.join(@backup_dir, 'config_files.tar.gz')
+      @increments_path ||= File.join(@backup_dir, '.config.snar')
+      configs, to_exclude = config_files


Suggested change

configs, to_exclude = config_files

configs, exclude_configs = config_files

kgaikwad · 2019-09-05T09:23:18Z

@jameerpathan111, mind testing these changes.

jameerpathan111 · 2019-09-05T10:02:37Z

@jameerpathan111, mind testing these changes.

@kgaikwad, @lvrtelov will be looking into this issue.

kgaikwad

@upadhyeammit, @mbacovsky,
Do we still need to keep with_content? method inside foreman_proxy feature?
Can't we directly check the presence of pulp feature inside two conditions i.e at line 24 and line 44

kgaikwad · 2019-11-17T05:44:18Z

definitions/procedures/backup/config_files.rb

+        else
+          attempt_no += 1
+          FileUtils.rm_rf(tarball_path)
+          puts "We will re-try after #{RETRY_DELAY} seconds."


Instead of using we, could you change a message something like -

Suggested change

puts "We will re-try after #{RETRY_DELAY} seconds."

puts "Waiting #{RETRY_DELAY} seconds before re-try."

@kgaikwad I think with_content? is useful information about the proxy and the proxy itself should be responsible for providing it. If we start to define content differently e.g. Pulp 3 has different feature or is not mandatory we want to have single place to update.

Currently, inside with_content? it just verifies the feature presence and method defined inside feature instance at line 43 says foreman_proxy_with_content? so I thought it's better to add condition there.

Secondly, with the different features for Pulp2 & Pulp3 and the suggested way on PR-290 it will resolve the concern. What do you think?

IMO feature(:foreman_proxy).with_content? and feature(:instance).foreman_proxy_with_content? are two different things. The former tells if there is content managed with the proxy while the later tells if the machine is proxy with content only (no server). So I think both make sense to keep.

Further I'd suggest not to reduce with_content to with_pulp without revisiting the referencing code and checking what actually we are testing. I can imagine content proxy that is using remote pulp - it would still be content proxy but the current test will not be sufficient. If we need just pulp we should test for presence of pulp and not content proxy. Revisiting this is probably out of scope of this PR.

Thank you @mbacovsky for giving pointers.
Because after seeing at the method, I was under an impression that it just checks the presence of pulp and didn't get the scope of it.

upadhyeammit · 2019-11-18T13:39:07Z

@upadhyeammit, @mbacovsky,
Do we still need to keep with_content? method inside foreman_proxy feature?
Can't we directly check the presence of pulp feature inside two conditions i.e at line 24 and line 44

Nice suggestion, we could just check if pulp is there. Not sure whats coming up with Pulp 3, if its sort of node based where we can have pulp feature but not the content then this method should be useful.

upadhyeammit · 2019-11-18T13:40:54Z

@kgaikwad I have fixed the message as per suggestion. Thank You !

kgaikwad

Thank you @upadhyeammit

ACK from my side except the comment mentioned related to with_content?.
If everyone agree I don't mind to handle it separately in a different pull-request as not a part of an original issue.

mbacovsky

Looks good to merge, Thanks @upadhyeammit, @kgaikwad!

mbacovsky · 2019-11-20T16:48:22Z

@lvrtelov do you want to give it another try before it is merged?

lvrtelov · 2019-11-22T11:31:29Z

I gave it a quick try :) I think it looks good and I also like the new message more.

Tested with Satellite

brand new set-up with foreman-maintain nightly + PR patch

Test cases

changing config -> failed backup
stable config -> successful backup

# foreman-maintain advanced procedure run backup-config-files --backup-dir /backup/
Running ForemanMaintain::Scenario
================================================================================
Backup config files: 
- Collecting config files to backup                                             
WARNING: Attempt 1/3 to collect all config files failed!
Some files were modified during creation of the archive.
Waiting 10 seconds before re-try
| Collecting config files to backup                                   [OK]      
--------------------------------------------------------------------------------

mbacovsky · 2019-11-22T16:11:08Z

@lvrtelov, thanks a lot for testing this 🦄 !

theforeman-bot added Needs testing Not yet reviewed labels Aug 13, 2019

kgaikwad suggested changes Aug 14, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 14, 2019

theforeman-bot added Needs re-review and removed Waiting on contributor labels Aug 19, 2019

theforeman-bot added the Waiting on contributor label Aug 19, 2019

kgaikwad added the 0.4.x pull-requests targeted for next version 0.4.x label Aug 20, 2019

upadhyeammit force-pushed the 27576-config-file branch from 1ccffc4 to 25a5860 Compare August 22, 2019 07:28

theforeman-bot removed the Waiting on contributor label Aug 22, 2019

upadhyeammit force-pushed the 27576-config-file branch from 25a5860 to ab4f651 Compare August 30, 2019 10:34

upadhyeammit commented Aug 30, 2019

View reviewed changes

kgaikwad suggested changes Sep 3, 2019

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review and removed Needs re-review Waiting on contributor labels Sep 3, 2019

kgaikwad reviewed Sep 5, 2019

View reviewed changes

upadhyeammit added 11 commits November 13, 2019 13:11

Fixes #27576 - config file fix

55fb8cf

Fixes #27576 - rebased changes

7b45e8f

Refs #27576 - delete extra methods

d94a5a0

Refs #27576 - fix rubocop failures

9fea14e

Refs #27576 - change method names

f4e0510

Refs #27576 - fix nil in proxy

d9b999d

Refs #27576 - change warn message

f2ec659

Refs #27576 - change hash structure

eec755d

Refs #27576 - change warn message

7550a86

Refs #27576 - Martin's changes

32503e2

Refs #27576 - Remove extra condition

db0e679

upadhyeammit force-pushed the 27576-config-file branch from fffb8fd to db0e679 Compare November 14, 2019 09:32

kgaikwad suggested changes Nov 17, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 17, 2019

kgaikwad removed the Needs testing label Nov 17, 2019

Refs #27576 - Change display message

adbdcf5

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 18, 2019

kgaikwad approved these changes Nov 19, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 19, 2019

mbacovsky approved these changes Nov 20, 2019

View reviewed changes

mbacovsky merged commit 25850e9 into theforeman:master Nov 22, 2019

upadhyeammit deleted the 27576-config-file branch December 3, 2021 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #27576 - config files backup fix #277

Fixes #27576 - config files backup fix #277

upadhyeammit commented Aug 13, 2019

theforeman-bot commented Aug 13, 2019

upadhyeammit commented Aug 13, 2019

kgaikwad left a comment

kgaikwad Aug 14, 2019

upadhyeammit Aug 19, 2019 •

edited

Loading

kgaikwad Sep 3, 2019 •

edited

Loading

kgaikwad Aug 14, 2019

upadhyeammit Aug 19, 2019

kgaikwad commented Aug 14, 2019 •

edited

Loading

upadhyeammit commented Aug 19, 2019

theforeman-bot commented Aug 19, 2019

kgaikwad commented Aug 27, 2019

upadhyeammit Aug 30, 2019

upadhyeammit commented Aug 30, 2019

upadhyeammit commented Aug 30, 2019

kgaikwad Sep 3, 2019 •

edited

Loading

upadhyeammit commented Sep 5, 2019

kgaikwad Sep 5, 2019

kgaikwad commented Sep 5, 2019

jameerpathan111 commented Sep 5, 2019

kgaikwad left a comment

kgaikwad Nov 17, 2019

mbacovsky Nov 18, 2019

kgaikwad Nov 19, 2019

mbacovsky Nov 19, 2019

kgaikwad Nov 19, 2019

upadhyeammit commented Nov 18, 2019

upadhyeammit commented Nov 18, 2019

kgaikwad left a comment •

edited

Loading

mbacovsky left a comment

mbacovsky commented Nov 20, 2019

lvrtelov commented Nov 22, 2019

mbacovsky commented Nov 22, 2019

	break unless statuses.include? exit_status
	break unless statuses.include? run_tar_cmd

	configs, to_exclude = config_files
	configs, exclude_configs = config_files

	puts "We will re-try after #{RETRY_DELAY} seconds."
	puts "Waiting #{RETRY_DELAY} seconds before re-try."

Fixes #27576 - config files backup fix #277

Fixes #27576 - config files backup fix #277

Conversation

upadhyeammit commented Aug 13, 2019

theforeman-bot commented Aug 13, 2019

upadhyeammit commented Aug 13, 2019

kgaikwad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upadhyeammit Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

kgaikwad Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Aug 14, 2019 • edited Loading

upadhyeammit commented Aug 19, 2019

theforeman-bot commented Aug 19, 2019

kgaikwad commented Aug 27, 2019

Choose a reason for hiding this comment

upadhyeammit commented Aug 30, 2019

upadhyeammit commented Aug 30, 2019

kgaikwad Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

upadhyeammit commented Sep 5, 2019

Choose a reason for hiding this comment

kgaikwad commented Sep 5, 2019

jameerpathan111 commented Sep 5, 2019

kgaikwad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upadhyeammit commented Nov 18, 2019

upadhyeammit commented Nov 18, 2019

kgaikwad left a comment • edited Loading

Choose a reason for hiding this comment

mbacovsky left a comment

Choose a reason for hiding this comment

mbacovsky commented Nov 20, 2019

lvrtelov commented Nov 22, 2019

mbacovsky commented Nov 22, 2019

upadhyeammit Aug 19, 2019 •

edited

Loading

kgaikwad Sep 3, 2019 •

edited

Loading

kgaikwad commented Aug 14, 2019 •

edited

Loading

kgaikwad Sep 3, 2019 •

edited

Loading

kgaikwad left a comment •

edited

Loading