adding backup targets

[TAlonglong]

• Closes In the server add backup target host for scp #169
• Tests added
• Fully documented
• Add your name to AUTHORS.md if not there already

[codecov]

Codecov Report

Merging #170 (95c847f) into main (47dd25f) will increase coverage by 0.19%.
The diff coverage is 95.93%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   90.00%   90.19%   +0.19%     
==========================================
  Files          24       24              
  Lines        5100     5203     +103     
==========================================
+ Hits         4590     4693     +103     
  Misses        510      510              

Flag	Coverage Δ
unittests	90.19% <95.93%> (+0.19%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
trollmoves/client.py	83.21% <83.33%> (-0.03%)	⬇️
trollmoves/movers.py	84.05% <93.10%> (+0.59%)	⬆️
trollmoves/server.py	67.96% <93.33%> (+0.15%)	⬆️
trollmoves/move_it_base.py	79.80% <100.00%> (ø)
trollmoves/tests/test_client.py	99.62% <100.00%> (+0.01%)	⬆️
trollmoves/tests/test_ssh_server.py	98.26% <100.00%> (+0.18%)	⬆️

... and 1 file with indirect coverage changes

[pnuu]

LGTM. Just couple of small thoughts that can be ignored.

[mraspaud]

LGTM, just one styling comment.

[pnuu]

I also tested that the Publisher(...).start() didn't break usage with older Posttroll versions. At least tests pass locally with v1.9.0 and the latest v.1.10.0.

[pnuu]

One thing I forgot to ask for: example configuration. Which made me realise that the original primary destination is configured in the Client side, but the backups need to be in the Server config, right? Should the backup servers be sent in the request message by the Client? That way the Server process could be used also for other transfers and it wouldn't fallback to push the data to wrong backup hosts.

[TAlonglong]

Ah, crap. You are right.

[TAlonglong]

The implementation I came up with does not work at all.

[TAlonglong]

So, I think I will revert all of this work and start over. Maybe I'll start a new PR to add the start() to the publisher to be compatible with posttroll 1.10.0

Then I need a sanity check. I think I will implement the backup target if I hit here: https://github.com/pytroll/trollmoves/blob/main/trollmoves/client.py#L671

So the idea is then to, if backup targets/destinations config is provided, restart the request.

Any ideas?

[pnuu]

I would keep the structure you have here, but instead of putting the backup destinations to Server config, put them in Client config and add them in the request message maybe in this else block. Then in Server check the message for backup_destinations list in the message data if the retries go to zero.

[TAlonglong]

Ah, yes. That looks doable.

So, where is this tested then. Then I can try from there

[TAlonglong]

Updated to have the backup targets config in the client config.
This is then sent to the server, which change the destination if the primary fails.
It's tricky to propagate this info from the message to the actual mover, so I added a extra function parameter for this.
One issue I see is that the final message contains the primary target, I have not found a way to replace that information in the message.

Please let me know id you see anything odd

[pnuu]

One issue I see is that the final message contains the primary target, I have not found a way to replace that information in the message.

This is a tricky one. The message should maybe be updated here, but that would require rather large changes so that the actual destination were passed down from the individual Mover to movers.move_it() to server.RequestManager._move_file() to server.RequestManager._move_files() and eventually to movers.RequestManager.push(). And then there's the complication that _move_files() already returns either an error message or None.

[pnuu]

Just couple of small comments inline, otherwise LGTM.

The comment on the config made me think that later we could move the retry logic to movers.move_it() so that all of the movers would have the possibility. Then we could also have backup destinations with different transport schemes; the primary could be scp:// and backups scp:// and ftp:// for example. But that's for another PR.

[TAlonglong]

I'm quite happy with this. Please let me know what you think

[TAlonglong]

A step further. What do you think?

[pnuu]

Couple more things. And a live test would be great 😁

[TAlonglong]

You mean at the status update? And show this is working on the screen? Sure no problem.

[pnuu]

No need to demonstrate 😂 Just run a test that this works in real life.

[pnuu]

LGTM.

Does @mraspaud want to have a look?

[TAlonglong]

Then I see something that might be a feature. First time a backup target is used, the active connection is tagged with the primary target. Therefore the next time, if the previous connection is still alive, this is used even if the actual connection is to the backup target. SO that way if this occur it looks like the primary target is used but the actual cache connection is to the bakcup target.

changing line https://github.com/pytroll/trollmoves/blob/main/trollmoves/movers.py#L136 to
self.active_connections[(self.destination.hostname, port, username)] = connection, timer will solve this, but I'm not sure how the behaviour should be

[TAlonglong]

Not sure if this is ready to merge as I see the approval from @mraspaud is old/changes after the approval.

[lahtinep]

I'm merging this, we need a new release with the .start() method to make PyPI installation work with the latest Posttroll.