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

SSH support added. #29

Merged
merged 10 commits into from Nov 8, 2015

Conversation

Projects
None yet
8 participants
@fustundag
Collaborator

fustundag commented Jan 3, 2014

Backups can be created at remote server via ssh. Destination folder
should be given like “root@192.168.0.1:/bakcups”

NOTE: Assumed public/private key config is ok.

SSH support added.
Backups can be created at remote server via ssh. Destination folder
should be given like “root@192.168.0.1:/bakcups”

NOTE: Assumed public/private key config is ok.
@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Jan 12, 2014

Owner

Thanks for the pull requests, I try to do some tests with it then will get back to you.

Owner

laurent22 commented Jan 12, 2014

Thanks for the pull requests, I try to do some tests with it then will get back to you.

@thecosmicfrog

This comment has been minimized.

Show comment
Hide comment
@thecosmicfrog

thecosmicfrog Mar 11, 2014

Any update on merging this to the mainline code?

thecosmicfrog commented Mar 11, 2014

Any update on merging this to the mainline code?

@mfr75

This comment has been minimized.

Show comment
Hide comment
@mfr75

mfr75 Mar 30, 2014

Hello, look nice ... up ...

mfr75 commented Mar 30, 2014

Hello, look nice ... up ...

@acorbe

This comment has been minimized.

Show comment
Hide comment
@acorbe

acorbe Apr 8, 2014

Hi all,

looks interesting starting point for further contributions.. up...

acorbe commented Apr 8, 2014

Hi all,

looks interesting starting point for further contributions.. up...

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Apr 9, 2014

Owner

Hi everybody,

I haven't had time to completely go through the changes yet since it's quite a big one. One of my concern is that it wraps many built-in commands in a way that is not completely consistent (eg. some have parameters within the functions, some don't).

Is there any way to improve this and make it more consistent? Or perhaps is there a generic way to call any command via ssh without having to manually wrap each command?

Owner

laurent22 commented Apr 9, 2014

Hi everybody,

I haven't had time to completely go through the changes yet since it's quite a big one. One of my concern is that it wraps many built-in commands in a way that is not completely consistent (eg. some have parameters within the functions, some don't).

Is there any way to improve this and make it more consistent? Or perhaps is there a generic way to call any command via ssh without having to manually wrap each command?

@acorbe

This comment has been minimized.

Show comment
Hide comment
@acorbe

acorbe Apr 9, 2014

Hi,

can you be more explicit?

One possible improvement that I see genuinely refers to the naming convention. (although it would a big alteration to your original work)

One can possibly distinguish local commands (i.e. functions referring to the source) from commands referring to the destination (possibly wrapping ssh). I have sftp in mind.

E.g. lfn_* for local functions and rfn_* for remote functions. This IMHO would increase readability and layout a baseline for further modifications.

For what concerns SSH wrapping, I see the fact that private key should be fixed as a major possible issue. Should the code check a-priori whether passwordless access is possible?

BTW thanks to all the contributors of this awesome work. I tested now both the local and the ssh-based options and the worked as a charm!

acorbe commented Apr 9, 2014

Hi,

can you be more explicit?

One possible improvement that I see genuinely refers to the naming convention. (although it would a big alteration to your original work)

One can possibly distinguish local commands (i.e. functions referring to the source) from commands referring to the destination (possibly wrapping ssh). I have sftp in mind.

E.g. lfn_* for local functions and rfn_* for remote functions. This IMHO would increase readability and layout a baseline for further modifications.

For what concerns SSH wrapping, I see the fact that private key should be fixed as a major possible issue. Should the code check a-priori whether passwordless access is possible?

BTW thanks to all the contributors of this awesome work. I tested now both the local and the ssh-based options and the worked as a charm!

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Apr 9, 2014

Collaborator

Hi,

My approach is like that : There is command to run backup script e.g. mkdir -p -- "$DEST". This command must run. If destination is local, it should run at local. If destination is remote, it should run at remote. So, there is no change on command according to local or remote destination, just needed to run locally or remotely. I tried to do this logic by wrapping commands with custom functions.

For consistency, I checked the script. I forgot to write function for mv command at line 182. If there is another, I can change it.

I tested on Mac OSX and Centos only. Some debian and ubuntu tests should be done.

@acorbe,

Can you give example for further modifications you think? Maybe, if local and remote operating systems are different and commands need to be different, separate local and remote functions for a command is more suitable. But this requires operating system check both on local and remote :). Can we face this issue for the script?

About private key issue, I will add ssh connection check to script and if check fails, script will ask ssh password from command line.

Collaborator

fustundag commented Apr 9, 2014

Hi,

My approach is like that : There is command to run backup script e.g. mkdir -p -- "$DEST". This command must run. If destination is local, it should run at local. If destination is remote, it should run at remote. So, there is no change on command according to local or remote destination, just needed to run locally or remotely. I tried to do this logic by wrapping commands with custom functions.

For consistency, I checked the script. I forgot to write function for mv command at line 182. If there is another, I can change it.

I tested on Mac OSX and Centos only. Some debian and ubuntu tests should be done.

@acorbe,

Can you give example for further modifications you think? Maybe, if local and remote operating systems are different and commands need to be different, separate local and remote functions for a command is more suitable. But this requires operating system check both on local and remote :). Can we face this issue for the script?

About private key issue, I will add ssh connection check to script and if check fails, script will ask ssh password from command line.

@acorbe

This comment has been minimized.

Show comment
Hide comment
@acorbe

acorbe Apr 9, 2014

Hi,

@fustundag

Sorry, I haven't explained myself properly.

The modification I was considering refers only to naming conventions (hence maybe not very helpful at this stage - although, I have restoring functions in mind).

I call local every function operating at the source folder level, and remote every function operating at the destination folder level (hence possibly wrapping ssh).

hence, it would be

 fn_log_info()   ->  lfn_log_info()  (it runs not involving the destination folder)
 fn_ls()             ->  rfn_ls()           (it involves destination folder and possibly ssh)

therefore, in case you'd need a function which creates local symbolic links, you would have lfn_ls(). These functions, as you were saying, can be implicitly parametrized on the operating systems involved so to have a fair, OS-invariant, syntax.

Tests:
Having proper and systematic testing looks as a relevant thing.
I am into research, I am considering to use this nice script to regulate centralized backup of the machines I am responsible of.
Being sure that everything works smoothly would be cool.

I can run some tests, in particular

ubuntu/fedora -> ubuntu
ubuntu/fedora -> ubuntu server ssh
ubuntu/fedora -> ubuntu server ssh + ntfs mount

I am curious about how it behaves with cygwin.

acorbe commented Apr 9, 2014

Hi,

@fustundag

Sorry, I haven't explained myself properly.

The modification I was considering refers only to naming conventions (hence maybe not very helpful at this stage - although, I have restoring functions in mind).

I call local every function operating at the source folder level, and remote every function operating at the destination folder level (hence possibly wrapping ssh).

hence, it would be

 fn_log_info()   ->  lfn_log_info()  (it runs not involving the destination folder)
 fn_ls()             ->  rfn_ls()           (it involves destination folder and possibly ssh)

therefore, in case you'd need a function which creates local symbolic links, you would have lfn_ls(). These functions, as you were saying, can be implicitly parametrized on the operating systems involved so to have a fair, OS-invariant, syntax.

Tests:
Having proper and systematic testing looks as a relevant thing.
I am into research, I am considering to use this nice script to regulate centralized backup of the machines I am responsible of.
Being sure that everything works smoothly would be cool.

I can run some tests, in particular

ubuntu/fedora -> ubuntu
ubuntu/fedora -> ubuntu server ssh
ubuntu/fedora -> ubuntu server ssh + ntfs mount

I am curious about how it behaves with cygwin.

@laurent22 laurent22 referenced this pull request Apr 9, 2014

Closed

Remote Destination #7

@goltra

This comment has been minimized.

Show comment
Hide comment
@goltra

goltra May 6, 2014

Can you show example to connecto to remote destination folder with cygwin?
C:\cygwin\bin\bash.exe -l -c "/home/Administrador/backup.sh /cygdrive/e/escaner root@server.com:/root/des'"
if i do with local folder destination works.. but with remote no....

goltra commented May 6, 2014

Can you show example to connecto to remote destination folder with cygwin?
C:\cygwin\bin\bash.exe -l -c "/home/Administrador/backup.sh /cygdrive/e/escaner root@server.com:/root/des'"
if i do with local folder destination works.. but with remote no....

@acorbe

This comment has been minimized.

Show comment
Hide comment
@acorbe

acorbe May 6, 2014

I am just guessing comparing by eyes the commits..
Looks like @fustundag 's fork was born before cygwin support was considered.

acorbe commented May 6, 2014

I am just guessing comparing by eyes the commits..
Looks like @fustundag 's fork was born before cygwin support was considered.

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag May 6, 2014

Collaborator

As @acorbe said, I forked project before cygwin support. I will try to add cygwin support.

Collaborator

fustundag commented May 6, 2014

As @acorbe said, I forked project before cygwin support. I will try to add cygwin support.

@goltra

This comment has been minimized.

Show comment
Hide comment
@goltra

goltra May 7, 2014

it will be great .. thanks...

goltra commented May 7, 2014

it will be great .. thanks...

@goltra

This comment has been minimized.

Show comment
Hide comment
@goltra

goltra May 16, 2014

hello again.. i see you update 17 days ago rsync_tmbackup.sh file.. news about ssh and cygwin?
thanks and congratulations for this great project

goltra commented May 16, 2014

hello again.. i see you update 17 days ago rsync_tmbackup.sh file.. news about ssh and cygwin?
thanks and congratulations for this great project

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 May 21, 2014

Owner

@goltra, the pull request cannot be merged as it is for the reasons mentioned above. Cygwin support is easy to add, but making the code more consistent is trickier. Perhaps the approach mentioned by @acorbe is the way to go.

Owner

laurent22 commented May 21, 2014

@goltra, the pull request cannot be merged as it is for the reasons mentioned above. Cygwin support is easy to add, but making the code more consistent is trickier. Perhaps the approach mentioned by @acorbe is the way to go.

@acorbe

This comment has been minimized.

Show comment
Hide comment
@acorbe

acorbe May 22, 2014

so what about giving guidelines about what you have in mind?

acorbe commented May 22, 2014

so what about giving guidelines about what you have in mind?

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Sep 24, 2015

Owner

I'm curious, is there still some interest in this feature? The initial pull request cannot currently be merged due to the lack of Cygwin compatibility, however if someone is interested in fixing this and getting the feature merged, I'm happy to give you commit access. Please let me know here or by email.

Owner

laurent22 commented Sep 24, 2015

I'm curious, is there still some interest in this feature? The initial pull request cannot currently be merged due to the lack of Cygwin compatibility, however if someone is interested in fixing this and getting the feature merged, I'm happy to give you commit access. Please let me know here or by email.

@thecosmicfrog

This comment has been minimized.

Show comment
Hide comment
@thecosmicfrog

thecosmicfrog Sep 25, 2015

I'm definitely interested in this being merged with upstream. I'm currently using @fustundag's fork for a lot of my systems, since it's an easy and robust backup option. I couldn't commit to working on this due to the number of projects I'm currently working on though. I also have no experience with developing for Cygwin.

thecosmicfrog commented Sep 25, 2015

I'm definitely interested in this being merged with upstream. I'm currently using @fustundag's fork for a lot of my systems, since it's an easy and robust backup option. I couldn't commit to working on this due to the number of projects I'm currently working on though. I also have no experience with developing for Cygwin.

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Sep 25, 2015

Collaborator

I am trying to merge master with mine.

I have no experience with cywin. I try to install windows machine with cygwin. If I get any progress, I will keep you informed

Collaborator

fustundag commented Sep 25, 2015

I am trying to merge master with mine.

I have no experience with cywin. I try to install windows machine with cygwin. If I get any progress, I will keep you informed

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Sep 25, 2015

Owner

I don't have a machine with Cygwin setup at the moment, but normally most things that work in Linux work in Cygwin. The difference might just be how the dates are generated and parsed (and this fix is already in master).

Owner

laurent22 commented Sep 25, 2015

I don't have a machine with Cygwin setup at the moment, but normally most things that work in Linux work in Cygwin. The difference might just be how the dates are generated and parsed (and this fix is already in master).

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Sep 25, 2015

Collaborator

I will create EC2 instance when my merge is completed and I will test quickly.

Collaborator

fustundag commented Sep 25, 2015

I will create EC2 instance when my merge is completed and I will test quickly.

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Sep 25, 2015

Collaborator

Test is done: I sent backup files from Cygwin 2.2.1 at Windows Server 2012 R2 64bit to CentOS linux.
More tests are always better :)

Collaborator

fustundag commented Sep 25, 2015

Test is done: I sent backup files from Cygwin 2.2.1 at Windows Server 2012 R2 64bit to CentOS linux.
More tests are always better :)

@thecosmicfrog

This comment has been minimized.

Show comment
Hide comment
@thecosmicfrog

thecosmicfrog Sep 25, 2015

Haha, and @fustundag's changes have broken my Salt states! :D

All for progress!

thecosmicfrog commented Sep 25, 2015

Haha, and @fustundag's changes have broken my Salt states! :D

All for progress!

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Sep 25, 2015

Collaborator

@thecosmicfrog sorry for broken things. I hope it is not big deal for you :)

Let me know if I can do something.

Collaborator

fustundag commented Sep 25, 2015

@thecosmicfrog sorry for broken things. I hope it is not big deal for you :)

Let me know if I can do something.

@thecosmicfrog

This comment has been minimized.

Show comment
Hide comment
@thecosmicfrog

thecosmicfrog Sep 25, 2015

@fustundag Haha, not at all. Looks like some indentations may have changed? I have local changes in my repo for some reason. I just did a git checkout -- on rsync_tmbackup.sh and did a fresh pull. Seems to be working again :)

thecosmicfrog commented Sep 25, 2015

@fustundag Haha, not at all. Looks like some indentations may have changed? I have local changes in my repo for some reason. I just did a git checkout -- on rsync_tmbackup.sh and did a fresh pull. Seems to be working again :)

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Nov 3, 2015

There is some issue here as the script doesn't incorporate a bug fix related to this INPROGRESS_FILE. So if the backup crashes before the end, it won't be possible to resume it since INPROGRESS_FILE won't have been deleted. The master branch includes a fix for this issue but it would need to ported to the ssh way.

laurent22 commented on rsync_tmbackup.sh in f3a265b Nov 3, 2015

There is some issue here as the script doesn't incorporate a bug fix related to this INPROGRESS_FILE. So if the backup crashes before the end, it won't be possible to resume it since INPROGRESS_FILE won't have been deleted. The master branch includes a fix for this issue but it would need to ported to the ssh way.

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Nov 5, 2015

Collaborator

@laurent22 my first commit for this pull request is very old : fustundag@f3a265b

Can you check sh file from my repo master?

I ported "pid >> in progress file" and "pgrep appname" fixes.

Is there any fix needed to port?

Collaborator

fustundag commented Nov 5, 2015

@laurent22 my first commit for this pull request is very old : fustundag@f3a265b

Can you check sh file from my repo master?

I ported "pid >> in progress file" and "pgrep appname" fixes.

Is there any fix needed to port?

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Nov 8, 2015

Owner

@fustundag, the reason it wasn't working for me is that I was testing on Cygwin, and apparently neither solution works in this env. Process names for Bash scripts only show up as "/usr/bin/bash" so it's not straightforward to detect if it's already running. I will merge the pull request and add a TODO for Cygwin.

Owner

laurent22 commented Nov 8, 2015

@fustundag, the reason it wasn't working for me is that I was testing on Cygwin, and apparently neither solution works in this env. Process names for Bash scripts only show up as "/usr/bin/bash" so it's not straightforward to detect if it's already running. I will merge the pull request and add a TODO for Cygwin.

laurent22 pushed a commit that referenced this pull request Nov 8, 2015

@laurent22 laurent22 merged commit b7d2cae into laurent22:master Nov 8, 2015

@laurent22

This comment has been minimized.

Show comment
Hide comment
@laurent22

laurent22 Nov 8, 2015

Owner

Okay, finally the feature is merged 👍 Thanks a lot for your work!

Owner

laurent22 commented Nov 8, 2015

Okay, finally the feature is merged 👍 Thanks a lot for your work!

@fustundag

This comment has been minimized.

Show comment
Hide comment
@fustundag

fustundag Nov 8, 2015

Collaborator

@laurent22 Ok, I will test with cygwin.

Collaborator

fustundag commented Nov 8, 2015

@laurent22 Ok, I will test with cygwin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment