Improving `backup_and_restore.sh` script before new-coming features #6030

h3ssan · 2024-08-19T20:08:14Z

Contribution Guidelines

I've read the contribution guidelines and wholeheartedly agree them

What does this PR include?

Adding print_usage function to backup_and_restore.sh script
Add colored messages in backup_and_restore.sh script
Move the thread count notice into the print_usage
Fix: Double quoted variable BACKUP_LOCATION. - Critical: Since it might has spaces therefore will corrupt the backup/restore process.
Bug Fix: overwriting filesystem possible - Critical: Previous code was not checking if the path is a symbolic to a filesystem, if user is root and the given path is a symbolic of filesystem then it will overwrite it!
Simplify the check expression of THREADS
Implement check_required_tools function - Implement check_required_tools function in backup_and_restore.sh script, as well call it in the beginning of the script instead of at the end
Clarifying the duplicate assigning of MAILCOW_BACKUP_LOCATION
Fix Bug: backup_and_restore.sh script, increment i only if needed
Fix: Crash on entering non-numeric value
Fix: backup_location should be exists, for the restore
Fix: Crash on entering non-numeric value
Implement declare_restore_point function
Typo in calling declare_restore_point
Fix: grep: warning: stray \ before :
Implement declare_file_selection function
Implement restore_docker_component to restore components in one place
Fix last grep: warning: stray \ before :
Write a clarification comment for function restore_docker_component
Introduce new help screen, with new flags depends script

Short Description

This PR is not finished yet, I'll include many re-write code for the backup_and_restore.sh script. Enhancement, refactor and improved code, before starting to implement a new features.

Affected Containers

None

Did you run tests?

Tested manually

What did you tested?

backup_and_restore.sh script

What were the final results? (Awaited, got)

All good

Since it might have spaces, it must be double quoted.

Previous code was not checking if the path is a `symbolic` to a filesystem, if user is root and the given path is a `symbolic` of filesystem then it will overwrite it!

Implement `check_required_tools` function in `backup_and_restore.sh` script, as well call it in the beginning of the script instead of at the end

h3ssan · 2024-08-20T00:28:31Z

Hey @DerLinkman, you can now resolve this PR, since I will need a few time to refactor and fully test the backup and restore functionality, since then you can resolve this PR now to avoid conflicts.

DerLinkman · 2024-08-20T06:41:03Z

Hey @DerLinkman, you can now resolve this PR, since I will need a few time to refactor and fully test the backup and restore functionality, since then you can resolve this PR now to avoid conflicts.

Hello @h3ssan! Is this pr now ready to merge or not? If not, we can convert it to a draft.

h3ssan · 2024-08-20T07:20:21Z

Hey there @DerLinkman , yes it’s ready to be merged.

still there’s 2 functions I need to look into but might take some time so yes, resolve it

DerLinkman

Looks fine!

h3ssan · 2024-08-20T15:56:15Z

Clarification

Commit Fix Bug: backup_and_restore.sh script, increment i only if needed, why?

Original Code

  i=1 # Starts at 1, even before finding any folder
  declare -A FOLDER_SELECTION
  if [[ $(find "${BACKUP_LOCATION}"/mailcow-* -maxdepth 1 -type d 2> /dev/null| wc -l) -lt 1 ]]; then
    echo -e "\e[31mSelected backup location has no subfolders\e[0m"
    exit 1
  fi
  for folder in $(ls -d "${BACKUP_LOCATION}"/mailcow-*/); do
    echo "[ ${i} ] - ${folder}"
    FOLDER_SELECTION[${i}]="${folder}"
    ((i++)) 
  done

If i is initial to 1, this will cause:

i = 1 - Initial
Checking if there's a folder, then increment i, therefore 1 = 2

Here we have i equals to 2 but we only found one folder !
Then, when the user choose which folder to restore, the user can simply choose 2 not 1 and the script will continue working, check the following image:

`backup_and_restore.sh` script, with `restore` flag, crashes when entering a non-numeric value as backup choice

h3ssan · 2024-08-20T16:19:32Z

Clarification

Commit: Fix: Crash on entering non-numeric value, fixes the following issue:

Enter , (non-numeric value, actually) will case a crash:

Fix the crash on entering non-numeric value, as well as make the code more readable

h3ssan · 2024-08-20T21:47:43Z

Till this point, what I have tested is below:

Tested Command	Expected	Status
`./backup_and_restore.sh backup vmail`	Backed up¹	Good ✓
`./backup_and_restore.sh backup all`	Backed up¹	Good ✓
`./backup_and_restore.sh backup anythingelse`	Throw `Unknown argument: anythingelse`	Good ✓
`./backup_and_restore.sh anythingelse`	print the usage	Good ✓
`./backup_and_restore.sh restore` with correct folder	Restored¹	Good ✓
`./backup_and_restore.sh restore` with non-existence folder	Throw `location not found`	Good ✓
`./backup_and_restore.sh restore` with location with no data	Throw `no subfolders`	Good ✓
`./backup_and_restore.sh restore` with location to `/dev/sda`	Throw `invalid path`	Good ✓
`./backup_and_restore.sh backup vmail` with location to `/dev/sda`	Throw `invalid path`	Good ✓

¹ I DID NOT check the actual process, just trusted what the script said! Therefore it might need extensive/dedicated testing.

Refactor `backup_and_restore.sh` script, implement `declare_restore_point` function.

h3ssan · 2024-08-21T00:35:34Z

Sorry about the last commit, there was an issue with the committer and author details. I soft reset, fixed it and push again.

h3ssan · 2024-08-21T00:46:56Z

Please note that this commit Implement declare_restore_point function still need testing. I'll do the test in a few hours.

Refactor `backup_and_restore.sh` script, implement `declare_file_selection` function

Implement `restore_docker_component` function to restore `vmail`, `redis`, `crypt`, `rspamd` and `postfix`, all from one function

h3ssan · 2024-08-27T08:01:50Z

Thank you @DerLinkman for the testing process.
I have fixed the issues, please do the testing process again!

sorry I know it's noisy but necessary.

DerLinkman

See comment

helper-scripts/backup_and_restore.sh

DerLinkman · 2024-08-27T09:12:18Z

How does this script behave when it is automated? E.g runned via cron?

Especially the part with -d (delete backups) you have to enter YES to confirm deletion?

h3ssan · 2024-08-27T09:19:14Z

How does this script behave when it is automated? E.g runned via cron?

Especially the part with -d (delete backups) you have to enter YES to confirm deletion?

For now, it won't. In the next feat PR, I'll make it automated.

DerLinkman · 2024-08-27T09:21:29Z

For now, it won't. In the next feat PR, I'll make it automated.

That makes it hard for us to track... as we always take all staging commits into the next update and we await the full functionality of that. If you say that the automation is not implemented yet inside this pr, then we won't merge it... i'm sorry. As this is heavily used by other users.

In general: Why did you want to make multiple prs instead of one which includes all of your changes, even the new additions? I did not get it.

h3ssan · 2024-08-27T09:26:01Z

For now, it won't. In the next feat PR, I'll make it automated.

That makes it hard for us to track... as we always take all staging commits into the next update and we await the full functionality of that. If you say that the automation is not implemented yet inside this pr, then we won't merge it... i'm sorry. As this is heavily used by other users.

In general: Why did you want to make multiple prs instead of one which includes all of your changes, even the new additions? I did not get it.

Then, If something happened unexpected, we will revert everything instead of the PR that has the problem. Please correct me if I am wrong.

As well, many features inside one PR will make the testing harder.

If you resist, then it's ok for me. Please confirm that you need the full implementation inside this PR and I'll start make the script fully automated.

DerLinkman · 2024-08-27T10:28:58Z

I definitely unterstand your doing. I prefer this. But currently your backup script is removing a automated part. So yes: Please readd the automation into your scripts remake PR.

h3ssan · 2024-08-27T15:39:39Z

The script now can be automated using --yes flag.

Example:

./helper-scripts/backup_and_restore.sh --delete /opt/backup 7 --yes

Please note that since --yes flag is critical and won't ask for any confirmation, therefore there will not be any short flag for it. Only --yes.

DerLinkman

See comment (only one :D) rest looks fine to me.

However, we need to adjust the mailcow docs too. Do you want to do it by yourself or should we adapt it?

Just asking, that we don't do the same work parallel :D

helper-scripts/backup_and_restore.sh

h3ssan · 2024-08-29T10:41:09Z

See comment (only one :D) rest looks fine to me.

However, we need to adjust the mailcow docs too. Do you want to do it by yourself or should we adapt it?

Just asking, that we don't do the same work parallel :D

Sure, I’ll do adjust the docs!

Full error message: ``` Error: auth-master: userdb list: connect(/run/dovecot/auth-userdb) failed: Connection refused Panic: file auth-master.c: line 436 (auth_master_unset_io): assertion failed: (conn->to == NULL) ```

DerLinkman · 2024-09-10T15:22:50Z

@MAGICCC could you try to test this too? It works and looks good for me but if a seperate user can take a look at this i would be happy

MAGICCC · 2024-09-10T15:57:01Z

Yes I will check on the weekend!

h3ssan · 2024-09-10T15:57:29Z

Yes I will check on the weekend!

Thank you!

mrclschstr · 2024-09-17T13:27:41Z

Would be awesome to include this as well: #4833

helper-scripts/backup_and_restore.sh

DerLinkman · 2024-10-15T09:30:18Z

Would be awesome to include this as well: #4833

@h3ssan could you adapt this in your next step (adding new features)?

h3ssan · 2024-10-15T14:28:29Z

Would be awesome to include this as well: #4833

@h3ssan could you adapt this in your next step (adding new features)?

I went through the PR, interesting. I will implement it later (after completing this step).

h3ssan added 7 commits August 19, 2024 22:59

Adding print_usage function to backup_and_restore.sh script

e3311ae

Add colored messages in backup_and_restore.sh script

2460a98

Move the thread count notice into the usage menu

5eceecf

Fix: Double quoted variable BACKUP_LOCATION.

82def75

Since it might have spaces, it must be double quoted.

Bug Fix: overwriting filesystem possible

a621aea

Previous code was not checking if the path is a `symbolic` to a filesystem, if user is root and the given path is a `symbolic` of filesystem then it will overwrite it!

Simplify the check expression of THREADS

0d95962

Implement check_required_tools function

67ba81a

Implement `check_required_tools` function in `backup_and_restore.sh` script, as well call it in the beginning of the script instead of at the end

DerLinkman approved these changes Aug 20, 2024

View reviewed changes

h3ssan added 2 commits August 20, 2024 18:00

Clarifying the duplicate assigning of MAILCOW_BACKUP_LOCATION

f64efb3

Fix Bug: backup_and_restore.sh script, increment i only if needed

cfe13b0

Fix: Crash on entering non-numeric value

4d00ca6

`backup_and_restore.sh` script, with `restore` flag, crashes when entering a non-numeric value as backup choice

h3ssan added 2 commits August 20, 2024 20:55

Fix: backup_location should be exists, for the restore

a2ab527

Fix: Crash on entering non-numeric value

ea24505

Fix the crash on entering non-numeric value, as well as make the code more readable

h3ssan force-pushed the Implement/backup-and-restore-script branch 2 times, most recently from cc8c889 to ea24505 Compare August 21, 2024 00:31

Implement declare_restore_point function

c45b860

Refactor `backup_and_restore.sh` script, implement `declare_restore_point` function.

h3ssan added 6 commits August 21, 2024 14:34

Typo in calling declare_restore_point

de743cd

Fix: grep: warning: stray \ before :

728a995

Implement declare_file_selection function

d398eca

Refactor `backup_and_restore.sh` script, implement `declare_file_selection` function

Implement restore_docker_component to restore components in one place

bd229b9

Implement `restore_docker_component` function to restore `vmail`, `redis`, `crypt`, `rspamd` and `postfix`, all from one function

Fix last grep: warning: stray \ before :

d0b38b7

Write a clarification comment for function restore_docker_component

93fc601

h3ssan added 2 commits August 27, 2024 10:53

Add grep, find and sed to the required_tools, also BusyBox check.

cd75642

Remove duplicate threads notice for backup process

4cac270

h3ssan requested a review from DerLinkman August 27, 2024 08:02

DerLinkman requested changes Aug 27, 2024

View reviewed changes

helper-scripts/backup_and_restore.sh Outdated Show resolved Hide resolved

h3ssan requested a review from DerLinkman August 27, 2024 09:20

h3ssan added 2 commits August 27, 2024 18:05

Implement --yes flag, to automate --delete and --backup script

48592ab

Fix: find: No such file or directory when directory not contains data

9d97377

DerLinkman requested changes Aug 29, 2024

View reviewed changes

helper-scripts/backup_and_restore.sh Show resolved Hide resolved

h3ssan added 2 commits August 29, 2024 15:16

Fix dovecot resync: failed: Connection refused

a20d927

Full error message: ``` Error: auth-master: userdb list: connect(/run/dovecot/auth-userdb) failed: Connection refused Panic: file auth-master.c: line 436 (auth_master_unset_io): assertion failed: (conn->to == NULL) ```

Fix: dovecot resync: failed connection refused

e8b6eb3

MAGICCC added the add into docs label Aug 29, 2024

h3ssan mentioned this pull request Sep 6, 2024

Update backup and restore docs mailcow/mailcow-dockerized-docs#747

Open

MAGICCC approved these changes Sep 17, 2024

View reviewed changes

ashish1099 reviewed Sep 26, 2024

View reviewed changes

helper-scripts/backup_and_restore.sh Show resolved Hide resolved

DerLinkman force-pushed the staging branch 2 times, most recently from aaee040 to 4d688c5 Compare November 15, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving `backup_and_restore.sh` script before new-coming features #6030

Improving `backup_and_restore.sh` script before new-coming features #6030

h3ssan commented Aug 19, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024

DerLinkman commented Aug 20, 2024

h3ssan commented Aug 20, 2024 •

edited

Loading

DerLinkman left a comment

h3ssan commented Aug 20, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024

h3ssan commented Aug 21, 2024

h3ssan commented Aug 21, 2024

h3ssan commented Aug 27, 2024

DerLinkman left a comment

DerLinkman commented Aug 27, 2024

h3ssan commented Aug 27, 2024

DerLinkman commented Aug 27, 2024 •

edited

Loading

h3ssan commented Aug 27, 2024 •

edited

Loading

DerLinkman commented Aug 27, 2024

h3ssan commented Aug 27, 2024

DerLinkman left a comment

h3ssan commented Aug 29, 2024

DerLinkman commented Sep 10, 2024

MAGICCC commented Sep 10, 2024

h3ssan commented Sep 10, 2024

mrclschstr commented Sep 17, 2024

DerLinkman commented Oct 15, 2024

h3ssan commented Oct 15, 2024

Improving backup_and_restore.sh script before new-coming features #6030

Are you sure you want to change the base?

Improving backup_and_restore.sh script before new-coming features #6030

Conversation

h3ssan commented Aug 19, 2024 • edited Loading

Contribution Guidelines

What does this PR include?

Short Description

Affected Containers

Did you run tests?

What did you tested?

What were the final results? (Awaited, got)

h3ssan commented Aug 20, 2024

DerLinkman commented Aug 20, 2024

h3ssan commented Aug 20, 2024 • edited Loading

DerLinkman left a comment

Choose a reason for hiding this comment

h3ssan commented Aug 20, 2024 • edited Loading

Clarification

Original Code

h3ssan commented Aug 20, 2024 • edited Loading

Clarification

h3ssan commented Aug 20, 2024

h3ssan commented Aug 21, 2024

h3ssan commented Aug 21, 2024

h3ssan commented Aug 27, 2024

DerLinkman left a comment

Choose a reason for hiding this comment

DerLinkman commented Aug 27, 2024

h3ssan commented Aug 27, 2024

DerLinkman commented Aug 27, 2024 • edited Loading

h3ssan commented Aug 27, 2024 • edited Loading

DerLinkman commented Aug 27, 2024

h3ssan commented Aug 27, 2024

DerLinkman left a comment

Choose a reason for hiding this comment

h3ssan commented Aug 29, 2024

DerLinkman commented Sep 10, 2024

MAGICCC commented Sep 10, 2024

h3ssan commented Sep 10, 2024

mrclschstr commented Sep 17, 2024

DerLinkman commented Oct 15, 2024

h3ssan commented Oct 15, 2024

Improving `backup_and_restore.sh` script before new-coming features #6030

Improving `backup_and_restore.sh` script before new-coming features #6030

h3ssan commented Aug 19, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024 •

edited

Loading

h3ssan commented Aug 20, 2024 •

edited

Loading

DerLinkman commented Aug 27, 2024 •

edited

Loading

h3ssan commented Aug 27, 2024 •

edited

Loading