search

OndrejHome / fast-vm

'fast-vm' is a script for defining VMs from images provided in thin LVM pool.
GNU General Public License v3.0
22 stars 14 forks source link

Add ssh key handling #34

Closed itsbill closed 6 years ago

itsbill commented 6 years ago

Thank you for fast-vm! I've come to use it nearly every day now. There are two features I've wanted for some time and I've attempted to implement them here: simplifying the creation of a separate ssh key, solely for fast-vm's use (for passwordless logins) and the ability to scp files easily.

OndrejHome commented 6 years ago

Hi @itsbill,

Thank you for submitting the pull request! I will try to check this over weekend and let you know my thoughts.

OndrejHome commented 6 years ago

Thank you @itsbill for changes. Will try to review them over weekend and let you know if there are any additional changes needed or if it can be merged.

OndrejHome commented 6 years ago

Hi @itsbill , its been some time since your last comment. Would you be looking into the above comments to address them in some near future (May/June)?

itsbill commented 6 years ago

Hi @OndrejHome sorry for the delay. I have made most of the required changes, but I want to doublecheck that I have addressed all the points you made earlier. I hope to do this in the next week or so... thanks.

OndrejHome commented 6 years ago

HI @itsbill, thank you for getting back. The SERIALIZE now starts to make sense to me. While it may cause not the most parallel execution I agree that it makes sense to make it not to mess the screen ( I was briefly considering making the SERIALIZE an option for some commands, but it doesn't seems at moment as best idea for the little gain that it would bring).

I will pull down the changes over this/next weekend and test them out to see if everything was addressed. If there are no huge changes needed I will merge this into master. I will see if I will create a release only with this change in master or if I will add some other changes and make bigger release in upcoming weeks. Stay tuned for updates :)

Thank you for taking the effort to provide this functionality!

OndrejHome commented 6 years ago

Hi @itsbill , code is OK for me. I have some small changes below and one question. Otherwise works like a charm :+1:

Question:

Small changes:

diff --git a/fast-vm b/fast-vm
index 7873cc9..257f511 100755
--- a/fast-vm
+++ b/fast-vm
@@ -984,14 +984,14 @@ case "$1" in
                vm_number="$2"
                if [ -z "$vm_number" ]; then pmsg $P_ERROR "VM number missing\n"; usage scp; fi
                if [[ $vm_number = *[\ ]* ]]; then handle_multiple_requests $@; fi
+               check_vm_number "$vm_number" "inactive"
+               update_access_time "$vm_number"
+
                if [ -z "$4" ]; then
                        pmsg $P_ERROR "scp requires a sourcepath and destination path to copy\n" $vm_number
                        exit 1
                fi

-               check_vm_number "$vm_number" "inactive"
-               update_access_time "$vm_number"
-
                wait_for_ssh "$vm_number"
                sourcepath=$(echo "$3" | sed "s/\(.*\)\bvm:\(.*\)/\1root@192.168.$SUBNET_NUMBER.$vm_number:\2/")
                destpath=$(echo "$4" | sed "s/\(.*\)\bvm:\(.*\)/\1root@192.168.$SUBNET_NUMBER.$vm_number:\2/")
diff --git a/man/fast-vm.8 b/man/fast-vm.8
index 68f96b0..56f2894 100644
--- a/man/fast-vm.8
+++ b/man/fast-vm.8
@@ -117,8 +117,7 @@ fast-vm \(em script for defining VMs from images provided in thin LVM pool
 .B fast-vm
 .B scp
 .RI < VmNumber | "'VmNumber1 VmNumber2 ...'" >
-.RB vm:/remote/sourcefile|/local/sourcefile
-.RI /local/destfile|vm:/remote/destfile
+.RI < "vm:/remote/sourcefile /local/destfile" | "/local/sourcefile vm:/remote/destfile" >

 .SH DESCRIPTION
 fast-vm is provides command-line interface to create virtual machines (VMs) in 
@@ -364,7 +363,7 @@ Create machine with custom definition and hack file. Start it and after it's SSH

 .RB "scp a script to VMs 41 and 42. By default this goes in root's home directory, or specify a path instead"
 .sp
-.BI "fast-vm scp '41 42' script.sh vm:
+.BI "fast-vm scp " "'41 42' script.sh vm:"

 .SH EXIT CODES
 In case of error the fast-vm will return non-zero exit code. When multiple VMs were specified then zero exit code is returned only when operation succeeded on all VMs. If any of VMs reported non-zero exit code, then the overal exit code will also be non-zero.
itsbill commented 6 years ago

Thank you @OndrejHome -- those changes look great! I have not written bash completion before, but I try below. I tried to have it add "vm:" to the source prompt for scp,and only add "vm:" to the destination completion if it wasn't already specified in the source. This gets tricky because it contains a split char (colon)... anyway, here is my attempt, but please change as you see fit :smile:

diff --git a/fast-vm.bash_completion b/fast-vm.bash_completion
index defd6f8..48a542d 100644
--- a/fast-vm.bash_completion
+++ b/fast-vm.bash_completion
@@ -94,6 +94,21 @@ _fast-vm()
        return 0
    fi

+   if [ "$cword" -eq 3 ] && [ ${COMP_WORDS[1]} == "scp" ]; then
+       COMPREPLY=( $( compgen -W "vm:" -f -- "$cur" ) )
+       return 0
+   fi
+   if [ "$cword" -eq 4 ] && [ ${COMP_WORDS[1]} == "scp" ]; then
+       # source path did /not/ contain "vm:", so prompt only that
+       COMPREPLY=( $( compgen -W "vm:" ) )
+       return 0
+   fi
+   if [ "$cword" -ge 5 ] && [ ${COMP_WORDS[1]} == "scp" ]; then
+       # source path /did/ contain "vm:", so exclude it from destination
+       COMPREPLY=( $( compgen -f -- "$cur" ) )
+       return 0
+   fi
+
    case $prev in
        import_image|import_custom_image|import_profile)
            return 0
@@ -122,7 +137,7 @@ _fast-vm()
            COMPREPLY=( $( compgen -W "all active inactive" -- "$cur" ) )
            return 0
            ;;
-       start|stop|delete|console|ssh|info|resize)
+       start|stop|delete|console|keydist|scp|ssh|info|resize)
            local vm_numbers
            case $prev in
                start|resize)
@@ -131,7 +146,7 @@ _fast-vm()
                delete|edit_note|info)
                    vm_numbers=$(fast-vm list all short)
                ;;
-               console|ssh|stop)
+               console|keydist|scp|ssh|stop)
                    vm_numbers=$(fast-vm list active short)
                ;;
            esac
@@ -142,7 +157,7 @@ _fast-vm()

    $split && return 0
    if [ "$cword" -lt 2 ]; then
-       COMPREPLY=( $( compgen -W 'import_image import_custom_image export_image remove_image resize_image import_profile remove_profile edit_note list_images list_profiles create start stop delete resize console ssh list info' -- "$cur" ) )
+       COMPREPLY=( $( compgen -W 'import_image import_custom_image export_image remove_image resize_image import_profile remove_profile edit_note keydist list_images list_profiles create start stop delete resize console scp ssh list info' -- "$cur" ) )
        return 0
    fi
itsbill commented 6 years ago

I reset and incorporated your diff into this latest version, thanks. (I left out the bash completion diff because I am not confident that it is good enough.) Thanks again!

OndrejHome commented 6 years ago

Thanks a lot @itsbill ! I will check this over this weekend.

OndrejHome commented 6 years ago

I will play with bash completion tomorrow. The changes are now merged in master, but I will try to finish other tasks to release it together with version 1.4.0 soon.