Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOCS] Add socat pull mode #5150

Merged
merged 20 commits into from
Jun 23, 2020
Merged

[DOCS] Add socat pull mode #5150

merged 20 commits into from
Jun 23, 2020

Conversation

BenediktSeidl
Copy link
Contributor

I tried to document the socat pull mode described in #900 (comment).

To keep it simple I did not include the systemd method. Should it be added?

borg/docs/conf.py:114: RemovedInSphinx40Warning: The app.add_stylesheet() is deprecated. Please use app.add_css_file() instead.
@fantasya-pbem
Copy link
Contributor

Hi Benedikt,
I made some comments and hope you find them helpful. I appreciate that you took the task of adding the socat method!

@BenediktSeidl BenediktSeidl changed the title Add socat pull mode [DOCS] Add socat pull mode Apr 21, 2020
@ThomasWaldmann
Copy link
Member

@fantasya-pbem did you submit your other comments? I don't see any others...

Copy link
Contributor

@fantasya-pbem fantasya-pbem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomasWaldmann You're right, seems I didn't submit.

docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
remote server SSH would normally connect to. As this argument is also ignored,
we can replace the server name with a underscore::

borg create ssh://_/path_to_repository::name_of_backup /path_to_backup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting an underscore instead of the server name makes debugging confusing, e.g. in ps output with multiple pull mode repositories (if policy is the motivation to use pull mode, one would do it for all its repositories (backup servers)) one would not see at a glance to which server a particular borg create is backing up to. Borg itself also remembers repository locations, not sure whether it bases this (exclusively) on the location specified, but that may also be another area where unecessary ambiguity is introduced.
That sh -c hack is not nice either, have a wrapper taking the arguments and have it use the server name to construct the path to the correct socket, e.g. /var/run/borg/servername.socket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal was to show the simplest and shortest version of creating a pull backup. That's also why I haven't mentioned the system.d unit files used in #900 (comment) and that's why I don't like the idea of using the arguments given to the rsh command. I think the document should show a simple proof of concept that works (and has sane defaults). If the users wants more comfort then they can still create a systemd file or do other stuff.
Regarding the underscore, I added 3cca926. We can also use reponame as default and mention that the argument is ignored.
If the server name is used for internal hashing, we should also mention it in the docs. How can we find out if this is the case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yes, commenting that you did forget to mention that the server side socket could be handled by systemd socket activation. To me at least, it is the one thing that stood out, making it possible to turn this the setup into something practically usable (i.e. automated, reliable).
No need to provide the unit files or anything, but pointing out this option to implement this idea nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b68ea52 adds a note for systemd.

are you okay with additional sentence for the underscore?

so the only thing left open here is the question if the server name is used for some internal hashing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented on the systemd part in the respective commit.

Not ok at all with the underscore. You yourself say "we can replace the server name with an underscore", but there is absolutely no reason why one have to or should. There is no true benefit to it either. One types the command once into a script/cron job and that is it.
Anyone feeling like shortening it in weird ways with no good reason, he can do so when implementing it for himself, the documentation should not do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 6ad2657 for the systemd part, thanks for your suggestion.

I think it's more confusing if it seems like the server name is relevant. It's not about shortening the command, but to show what arguments are necessary and how they work together. The server name is not necessary for the command to work, so why provide it? We also don't provide a port and a username. Is this also shortening in weird ways?

Anyways I changed it in 1625a5b.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elho are you okay with the current wording (systemd and server name)?

@ThomasWaldmann Do you know if the server name is used for anything beside the host name of the ssh connection? Should we mention it in the docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reread the entire diff now, after all the changes. 🙂 Fine with things except for the 2 minor comments made now.

docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
docs/deployment/pull-backup.rst Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 26, 2020

Codecov Report

Merging #5150 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5150      +/-   ##
==========================================
- Coverage   84.01%   83.53%   -0.48%     
==========================================
  Files          37       37              
  Lines        9834     9834              
  Branches     1636     1636              
==========================================
- Hits         8262     8215      -47     
- Misses       1092     1133      +41     
- Partials      480      486       +6     
Impacted Files Coverage Δ
src/borg/platform/__init__.py 72.00% <0.00%> (-12.00%) ⬇️
src/borg/platform/xattr.py 82.53% <0.00%> (-6.35%) ⬇️
src/borg/xattr.py 48.83% <0.00%> (-3.49%) ⬇️
src/borg/platform/base.py 77.58% <0.00%> (-3.45%) ⬇️
src/borg/helpers/misc.py 77.02% <0.00%> (-2.03%) ⬇️
src/borg/archive.py 81.94% <0.00%> (-1.78%) ⬇️
src/borg/helpers/fs.py 86.66% <0.00%> (-1.34%) ⬇️
src/borg/archiver.py 80.78% <0.00%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb06160...511460a. Read the comment docs.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, iirc there is a bounty to add such docs, did you see it?

Comment on lines 220 to 222
On *borg-server* the socket file is opened by the user accessing
``/path/to/repo``, so the user has to have read and write permissions on
``/run/borg``::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a slightly strange way to specify a user.

maybe rather:

  • ... by the user running the "borg serve" process, ...
  • ...by the user writing to the repository, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added e5ef137 for this

``/path/to/repo``, so the user has to have read and write permissions on
``/run/borg``::

borg-server:~$ sudo chown user_accessing_path_to_repo /run/borg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another idea instead of always repeating this long user_accessing_path_to_repo (and similar, or long paraphrasings of this) would be to define early and then always use:

  • borgs as the username running borg serve (on the server)
  • borgc as the username running borg (on the client)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and also note that this is used for clarification and other user names might be required / can be working also, as long as one is consistent)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added e5ef137 for this

@BenediktSeidl
Copy link
Contributor Author

btw, iirc there is a bounty to add such docs, did you see it?

Yes, but I don't want to claim it. What do I have to do to redirect the money to the borg project?

@BenediktSeidl
Copy link
Contributor Author

Just cought my eye now, repositories contain archives, so repo::archive would be the correct borg terminogoly instaed of repo::name_of_backup.

I've added 2959e30 for that


borg-client:~$ borg create ssh://borg-server/path/to/repo::archive /path_to_backup

When creating a backup should be scheduled or otherwise automated, the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not a native speaker myself, I have a feeling that a native speaker would not put it this way. Had to reread the first part before I got what it is trying to say, too.
How about simply:
When automating backup creation, the ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> edb463f

borgc@borg-client \
borg create \
--rsh "sh -c 'exec socat STDIO UNIX-CONNECT:/run/borg/reponame.sock'" \
ssh://_/path/to/repo::archive /path_to_backup \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another _ in there, to be replaced by borg-server 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> 447ecb8

@BenediktSeidl
Copy link
Contributor Author

What's left to do here? Is it already time for rebasing and squashing?

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. i didn´t try it, but i hope you or the original authors did.

if there is some issue, we can also improve it later.

thanks for working on this.

docs/conf.py Show resolved Hide resolved
@ThomasWaldmann ThomasWaldmann merged commit 6a1f31b into borgbackup:master Jun 23, 2020
fantasya-pbem added a commit to fantasya-pbem/borg that referenced this pull request Dec 11, 2021
Backport from master borgbackup#5150: Document the socat pull mode described in borgbackup#900.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants