Restructure role, add Systemd timer option

[conloos]

• if needed creation of a service user incl. creation of the ssh-key,
• add the ssh key to authorized_keys,
• auto init of the repos (optional),
• creation and start of systemd timer and services and
• installation of the Docker helperscript.

[m3nu]

Hi there! Appreciate the thorough refactor. Few notes:

• Possible to keep Cron? The plan was to make this optional. If existing users run your new role, they will have 2 jobs (cron and Systemd)
• We may need to update the Python version GH Actions uses to pass the tests.

[conloos]

Hi there! Appreciate the thorough refactor. Few notes:

* Possible to keep Cron? The plan was to make this optional. If existing users run your new role, they will have 2 jobs (cron and Systemd)

Yes, having cron and systemd-timer up and running at the same time is not an option. I would have to adjust the variable names again so that they work for both, or I write a task that clears the cron jobs. But the danger is that there are user-generated cronjobs that get stuck.

* We may need to update the Python version GH Actions uses to pass the tests.

I have an eye on that, but i didn't used that before.

[conloos]

Hi @m3nu,

the installation of the timer (cron or systemd) is now optional, the autoinit, too. I run a lot of tests for:

• run as root (cron and systemd-timer)
• run as service account (cron and systemd-timer) It would be good if we implement this as a molecule test. However, as I already explained, I haven't implemented this before and I would be happy if you can support me here.

Thanks Frank

[m3nu]

Fixed a small indention issue in tasks/02_user_management.yml.

Also, what's the purpose of when: install_backup is not defined or install_backup? I see this var as tag, but not as var.

[m3nu]

And initializing the repo shouldn't be automatic. Usually the key isn't set up yet, if it's generated by the role.

I also feel that all the stuff and vars in tasks/03_create_key.yml should be documented at least.

[m3nu]

Testing fails here: https://github.com/borgbase/ansible-role-borgbackup/actions/runs/4392059341/jobs/7691597745

TASK [m3nu.ansible_role_borgbackup : Set authorized key taken from file] *******
  fatal: [almalinux-8 -> {{ backup_repository | regex_search('@(.*):', '\1') | first }}]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'backup_repository' is undefined. 'backup_repository' is undefined\n\nThe error appears to be in '/home/runner/work/ansible-role-borgbackup/ansible-role-borgbackup/tasks/03_create_key.yml': line 36, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n    - name: Set authorized key taken from file\n      ^ here\n"}

I feel this part is too specific for a public role. It relies on a very specific setup, which not many users will have. Maybe better if you move this part out of the role.

[m3nu]

Why do we need install_backup everywhere? Isn't it easier to just not run the role or add the tag when adding the role in a playbook?

[conloos]

Hi @m3nu,

thanks for the QS. I added the description, that had slipped through. The var: "install_backup" is a global switch to skip this role in playbooks with multiple roles. I use very large playbooks to build my labs, and some VMs dont need a backup.

So "install_bakup: false" is to skip the role

Thanks Frank

[conloos]

Why do we need install_backup everywhere? Isn't it easier to just not run the role or add the tag when adding the role in a playbook?

This is possible, too.

[m3nu]

So "install_bakup: false" is to skip the role

I see, but why not do it like this when including the role and keep this tag out of each role playbook?

    - role: raid-monitor
      tags: raid-monitor
      when: raid_vendor is defined ....

[conloos]

And initializing the repo shouldn't be automatic. Usually the key isn't set up yet, if it's generated by the role.

I also feel that all the stuff and vars in tasks/03_create_key.yml should be documented at least.

Ok, i switch that, so the User have to set it to true. I will document it in the README.md and in the Task.

[m3nu]

Then for readability it's maybe better to remove all the install_backup stuff.

And about the key? Specifically the last block in tasks/03_create_key.yml: Creating some key if it doesn't exist yet is OK. We did that before. The type should be customizable. It's hard-coded to RSA right now, even the var name.

Copying an existing key is not what we generally recommend. The key should be generated and kept on the machine that uses it.

And the part where you copy the key to another machine (Set authorized key taken from file) is too specific. The target machine may not be managed by Ansible. This will fail for most users (and in our tests). So I'd just print it.

And finally, the repo init should be optional for cases where the conditions are OK. That's the minority of cases. Your command also won't work with older Borg versions. Maybe better to have it outside the role, if someone really wants it.

I'm happy to work on those changes too. Just putting them here to keep track.

[conloos]

Then for readability it's maybe better to remove all the install_backup stuff.

I'm fine with that.

And about the key? Specifically the last block in tasks/03_create_key.yml: Creating some key if it doesn't exist yet is OK. We did that before. The type should be customizable. It's hard-coded to RSA right now, even the var name.

Ok, let us define that as var to choose between rsa and ec

Copying an existing key is not what we generally recommend. The key should be generated and kept on the machine that uses it.

Right, that was from my first iteration. This should be deleted.

And the part where you copy the key to another machine (Set authorized key taken from file) is too specific. The target machine may not be managed by Ansible. This will fail for most users (and in our tests). So I'd just print it.

I prefere (in my case :) to make this disabled by default. But can be activated by a boolean value, if the value isn't set or false (which is default) the key is printed as wanted.

And finally, the repo init should be optional for cases where the conditions are OK. That's the minority of cases. Your command also won't work with older Borg versions. Maybe better to have it outside the role, if someone really wants it.

I agree.

I'm happy to work on those changes too. Just putting them here to keep track.

I can help with the revision tonight. I am happy to receive all comments. If you have already worked through, please comment in the chat.

[m3nu]

No, I didn't start yet. All my changes were pushed earlier.

You just add the changes as you see fit. Will check back after that.

Thanks! All in all the refactoring you did is good and makes it better to understand and maintain.

[m3nu]

Noticed a few other things that can be modernized and added them is individual comments to track them better.

[conloos]

backup_repository | regex_search

First, this is now a option and only used if selected by user.

Than, "backup_repository" was a double definition: I added the following explanation:

# example:
#   borg_repository: m5vz9gp4@m5vz9gp4.repo.borgbase.com:repo
#   have three parts: "username"@"FQDN":"path/to/store/backup", specific:
#     a) user: m5vz9gp4
#     b) fqdn: m5vz9gp4.repo.borgbase.co
#     c) dir: repo

So we use part a) and b) to add the key to the targed

If you have a better idea than regex, then I would be happy. The other possibility I see is split() but that will be very long and I have to do many steps with register.

[m3nu]

Careful. SFTP-style URLs were already deprecated. I doubt you will want to implement parsing repo URLs in Ansible.

This role runs commands on a single machine to set up Borg and Borgmatic for backups. Setting up the receiving end is out of scope unfortunately. Sorry if this wasn't clear before.

[conloos]

Careful. SFTP-style URLs were already deprecated. I doubt you will want to implement parsing repo URLs in Ansible.

This role runs commands on a single machine to set up Borg and Borgmatic for backups. Setting up the receiving end is out of scope unfortunately. Sorry if this wasn't clear before.

ok, I didn't know that. I still finish the work now and will remove the specific parts tomorrow morning.

[conloos]

Hi @m3nu,

i think the code is ready for the next check.

Thanks Con

[m3nu]

Perfect! Will work on it tomorrow morning first thing.

[m3nu]

I guess this needs some more testing, since the borg_cron_package wasn't even used. 😬

[nain-F49FF806]

I tried to run this using the example given in readme.

I am getting some error regarding dpkg. I believe it's related to not having sudo permissions.

Running the example root-user example playbook : https://dpaste.org/0zw5d

It's looking to use apt, but doesn't have root permission.

[m3nu]

It's looking to use apt, but doesn't have root permission.

@nain-F49FF806, this Ansible role needs to run as root user during setup. After setup it can use a service user (recent addition by @conloos )

Try this or see here for more.

- hosts: webservers
  become: true
  roles:
  - role: m3nu.ansible_role_borgbackup
    borg_encryption_passphrase: CHANGEME
    borg_repository: m5vz9gp4@m5vz9gp4.repo.borgbase.com:repo
    borgmatic_timer: cron
    borg_source_directories:
      - /srv/www
      - /var/lib/automysqlbackup

[m3nu]

Hopefully I found all the bugs and sorry again for slimming down your role a bit to make it more universal. Should be simple enough to add the other pieces in other playbooks.

[m3nu]

@adhawkins, big refactor of this role to split tasks better and add Systemd support. Would be great if you can have a quick look, if you see any issues. 🙏

[adhawkins]

@adhawkins, big refactor of this role to split tasks better and add Systemd support. Would be great if you can have a quick look, if you see any issues. 🙏

There's a lot of changes there. Had a quick look over it and nothing obvious jumped out at me. Worth considering automatically creating the repo / SSH keys at Borgbase using my ansible collection?

[m3nu]

Thanks for checking. Will merge it then.

Worth considering automatically creating the repo / SSH keys at Borgbase using my ansible collection?

It was part of this role previously, IIRC. Better to do one thing well. We could put an extended example in EXAMPLES.md. As Frank already did for copying the key to his remote backup store.

[conloos]

Hi @m3nu ,

little changes: a) you already renamed borgmatic_user to borg_user, for some reason this was not redrawn on me, I have now made up for it. b) If the users are to be created, then this must be done before the packages are installed. If pip is used, then the links are created and the chown fails if the locale user is not yet created.

Thanks Frank

[conloos]

Hi @m3nu ,

unfortunately I have made a mistake. In the two commits is unfortunately a feature included: #115. I wanted to merge this only after the completed pull.

So i think only switching 01 and 02 is needed. For #115 i create a own pull request.

Thanks Frank

[m3nu]

You can just remove those commits. No problem. Better to finish this PR before making more changes.

I was testing the changes on some production machines this week. Also to check for breaking changes. The only thing I noticed was to keep borg_ssh_command at false by default. Because otherwise it will hard-code one key name and won't work with existing keys. I'll change that after you're happy with your last commits.

[conloos]

You can just remove those commits. No problem. Better to finish this PR before making more changes.

I was testing the changes on some production machines this week. Also to check for breaking changes. The only thing I noticed was to keep borg_ssh_command at false by default. Because otherwise it will hard-code one key name and won't work with existing keys. I'll change that after you're happy with your last commits.

Can you briefly show me how to undo commits? I have no idea.

Thanks Frank

[m3nu]

What I'll do is to overwrite your 2 last commits. Then they will be gone anyways. You will still have them locally and can cherry-pick them later.

Regarding switching those files: I don't think it's needed and /usr/local/bin/borg etc should be owned by root, not by the user. Else a normal user can manipulate bins for the whole system, which we can't have.

[m3nu]

You can do a final check and then we can merge it. Then do other changes as separate PRs, so it's easier to review.

[m3nu]

Best would be to open an issue, if you see some remaining issue that needs solving.