file copy

[viktorkertesz]

Hi Carl, all,
I wonder if you wanted to implement file copy (aka SCP) in specific device drivers. I did a POC to make it work with asyncssh. It does work, though the implementation is a bit ugly to my eyes. We need to open a 2nd session to the device and for that I use "private" connection options.
My idea was to do similar to netmiko file copy functionality. Are you interested or it's a dead idea?

Regards,
Viktor Kertesz

[carlmontanari]

Hey @viktorkertesz, thanks for opening this!

While I don't want to say I'd "never" want to add copy functionality, my initial gut feeling is its kinda a dead idea for me -- but again, not saying never!

The main objections I have are:

• Just more "stuff" to maintain and test... I am sorta at capacity with scrapli and the associated bits (definitely my own fault for adding netconf, community, cfg, and replay...) so I would be hesitant to take on more functionality.
• I sorta wonder about how useful it is? Clearly its useful enough for it to exist in netmiko and for you to have made a POC of it -- but I guess for me I've just historically issued copy commands on devices with send_interactive and have never had a problem. Perhaps this is more of a problem than I'm aware?
• Perhaps the biggest one: I'd be hesitant to have it in just one transport (asyncssh for example), and if we had it in all transports then thats a fair bit more work than just one! I know that currently all transports are not really created equal... but Id like to avoid any more unequalness with them if possible.
• Kinda similar to point 1 -- I'm unsure I'd want to add copy functionality to scrapli core even if I was super keen on adding it in general. In my view scrapli "core" is pretty much maxed out on the functionality that I want to add in to it. Id generally like to keep it doing one(ish) thing, and then add functionality in plugins/wrappers (like netconf and cfg for example).

Stuff like this is/was part of the reason I had thought it would be a good idea to keep the transports as separate repos/libraries.... then if they diverged in functionality more it wasn't as big of a deal as if they are all in the same repo w/ wildly different features. But.... maintaining all the plugin repos was more trouble than it was worth (at least at the time I suppose).

One possible option to kinda get the best of all worlds could be to build a third party plugin that extends the existing asyncssh transport -- as far as I recall the plugin machinery for transports still exists in scrapli core, so all you would need to do would be to create the plugin and call it "scrapli_asyncssh2" or something (important part is starting with "scrapli_" so it can be loaded properly).

Sorry for the wall of text! Let me know what ya think?

Carl

[carlmontanari]

But because of the above mentioned features, new methods should be implemented in the drivers as different OS handles files differently.

Ahhh I totally did not catch this the first time! Ok that makes lots of sense -- and good news, we can make this work fairly seamlessly already! scrapli_community is a way to add "non core" platforms on its face... but it can be a bit more as well...

We could add core platforms just w/ a slightly different name (i.e. instead of "cisco_iosxe" we can call it "cisco_iosxe_community" or something^1) and this community platform can have all the additional methods you want to contribute -- scp, md5 verify, etc. etc.. I think/hope this could really satisfy the original question and also keep me happy as scrapli "core" doesn't take on any extra work :D Does that sound like it may be interesting?

1: side note -- I have a long term desire to move all the core platforms out of scrapli core and into community so this extra "community" name could be squished into just "normal" "cisco_iosxe" at some point.

Edit meant to post this as well.... the test platform has a "custom" method added to it here -- so basically TL;DR would be you can (for now, pending that side note being a thing) inherit from IOSXEDriver and extend it w/ whatever methods you want!

[viktorkertesz]

Thanks Carl! I try to experiment with the community package then and leave the core alone.

[viktorkertesz]

Carl, can you confirm, that I won't be able to create new transport in community project? Supported transports are hardcoded in transport/init.py
I can implement SCP in the driver level though with the ugly statement like that:

class AsyncCommunityIOSXEDriver(AsyncIOSXEDriver):
    async def get_file(self, src_name, dst_name):
        # Setup secondary connection to the same box for SCP
        # I had to access all previously set options by this dirty reference
        options = self.transport.session._options
        SCP_CONN = {
            "host" : self.host,
            "username" : self.auth_username,
            "password" : self.auth_password,
            "options" : options
        }

        async with connect(**SCP_CONN) as scp_conn:
            await scp((scp_conn, src_name), dst_name)

Here that options is questionable. I plan to put helper functions under scrapli_community/scrapli/helper.py similar to main Scrapli project. This example is just the POC :)

[carlmontanari]

You can create a new transport, however it would have to be its own library (at least at the moment) -- they will get loaded here. Though I think you wouldn't need to build a whole new transport really -- looks like what you have would work ya?

I think I misunderstood again earlier today and have probably made things more confusing :) -- I was thinking it was all going to happen "in band" now but I see what you're doing and get what you said earlier about not wanting to do one off stuff for each platform. In any case, it seems like this could be a viable option so hopefully no wasted effort!

[viktorkertesz]

You can create a new transport, however it would have to be its own library (at least at the moment) -- they will get loaded here.

This I don't like because I would like to extend either scrapli or scrapli_community. But then it would be a bad idea to keep the dependent transport in a separate package.
I was able to separate iosxe drivers under the community project and now trying to implement the SCP functions. Then comes the logic to make smart file copy in the inherited drivers.

[viktorkertesz]

Hey @carlmontanari ,
I have a kinda working version. I implemented it as a Feature which extends existing Driver functionality by inheritance.
The abstract & general went to the extension, and the device specific to the Community version of IOSXE.

I'd need your opinion on the whole thing and also some advice if I want to implement timeout prevention. The file transfer will block all other operation at the moment and it can time out the main SSH channel. I have a workaround which enters empty commands in a specific interval during the copy, but I had to patch asyncssh for that to work. (progress_handler function call was not async but I could use that callback to handle timeout)
Yet I was thinking on starting a thread for file copy operations so it wouldn't block the driver. But this leads to complex solution which breaks your rule of keeping things simplistic.

BTW, I attached the demo code in action.
This is the call:

            result = await conn.file_transfer(operation='get', src=file_name, dst=".", overwrite=True,
                                              force_scp_config=False, progress_handler=progress_handler)

scrapli_scp.mp4

[carlmontanari]

Hey @viktorkertesz nice work!

This is a lot of stuff, and I'm impressed and intimidated by it all at once 😅!

Before talking about the technical bits: now that you've had some time to work on this, is this something you are thinking you want to get merged into community or are you happy just working on it and keeping it in your own repo/whatever? (either is a 1000% cool option, just asking rather than making assumptions!).

The file transfer will block all other operation at the moment and it can time out the main SSH channel

I assume this is because there is no "normal" scrapli stuff happening while the transfer is happening and so things just die/timeout? Would it be sufficient to just crank up (or disable) the timeout_transport during the transfer operation? Or will you also hit exec timeouts and stuff on the box and hence the sending some random characters comment?

If you are worried about the exec stuff I had built a "network keep alive" before... its like what you are saying -- just a thing that sends an innocuous command when it can get a lock. I removed it because it was just kinda extra work and nobody cared about it haha but you can see it here -- it may be handy?

Yet I was thinking on starting a thread for file copy operations so it wouldn't block the driver. But this leads to complex solution which breaks your rule of keeping things simplistic.

You might have to start a thread if you have any desire to implement this in sync land as well... but yeah, for async I think we can/should hopefully be able to do make it happen w/out a thread -- perhaps w/ a modified "network keep alive" thing and the actual transfer being gathered/awaited so they both happen at the same time. (and the keep alive can be "done" when the transfer is marked as done as some private internal flag or something??? dunno... just thinking out loud!)

[viktorkertesz]

Hi @carlmontanari ,
Ok, first on the repo question. For me it's totally up to your preference, it's your project. I made my changes on a fork so I can either do pull request or keep it separate. I'd be happy to contribute to the project so after polishing this part and maybe implement it to other vendors like Juniper, we can review and merge it back to the original community project.

Bits and bytes:
I was able to go the async way and no thread is needed. I was discussing this with the maintainer of asyncssh 😊
Yeah, the problem of timeout is coming from the fact that transfer can be slow. So the device will kick you out after some time if you don't do anything on the Scrapli side (waiting for the transfer to end). I could schedule the mentioned _keepalive_network(). I could call it instead of self.send_command(""). Probably my solution is slower but I didn't have many options 😀 If you think, I can re-implement it, but I need your opinion.

So far I implemented this new thing AsyncSCPFeature which is used by drivers, but it's not a driver itself. I need to put the file structure in order, I used folder name transport but it's not correct. Also I used the filename extension.py. Maybe that's not the best too.

About sync: I haven't even looked on the sync version for now. I'll be happy to make async work and then look at the other possibilities. Sync is working in a totally different way. We have the system ssh and ssh2. System should be fine with comand scp. I don't know ssh2. It's also true that we might start another thread or process to run the transfer. But that's for future.

[carlmontanari]

I was discussing this with the maintainer of asyncssh

Hell yeah! Ron is awesome!

I could schedule the mentioned _keepalive_network(). I could call it instead of self.send_command(""). Probably my solution is slower but I didn't have many options 😀 If you think, I can re-implement it, but I need your opinion.

I think (would have to confirm) that sending an empty string will send a newline which probably you dont want since there may be a prompt/something happening after the transfer is done/fails that you may be blindly sending a return at. This is why in that old "network keep alive" was like ctrl+e or something innocuous. I could be 100% wrong here, but figured id share what I think I remember :)

So far I implemented this new thing AsyncSCPFeature which is used by drivers, but it's not a driver itself. I need to put the file structure in order, I used folder name transport but it's not correct. Also I used the filename extension.py. Maybe that's not the best too.

Yeah -- I honestly dont know what makes the most sense here... I think maybe features is good? I think that we could add more in the future too since they are just a mixing class (or if it isn't I think it can be a mixin). Unclear if mixins for days is a good idea or not haha but it could work!

Sounds good on the sync stuff and everything else, thanks for keeping this going! -- lots of good info/work you've done here, nice to have it documented in the discussions!

Carl

[viktorkertesz]

Hi Carl @carlmontanari ,
I finished my async implementation and it works well for me but of course I didn't do very extensive testing. I tested with physical and virtual routers. Question: Do you want to merge it into community package? BTW, what are your requirements of merging? Like after implementing this for the sync/system transport? I'd need to work it out as I only started that part.
Tried to satisfy all your linting tools but had to force some of them (because it found some of the functions 'too complex') But I didn't want to split the coherent logic into small functions.

For remainder: The scp branch at my fork
New stuff are:

• features folder
• cisco drivers extended with scp feature

Some example on how to use it:
scrapli_scp_progress.py
scrapli-scp.py

Beside this feature I updated docs and in my drivers I used filename async_driver.py instead of _async.py. I guess it was to avoid namespace overlapping. Unfortunately pdoc doesn't pick up files starting by underscore. At least not for me 😒

Thanks!

Viktor

[carlmontanari]

Hey Viktor!

This looks like it came together nicely!

Question: Do you want to merge it into community package

Yeah I think that would be great if we can do that. I think having that feature place to break things out makes this fit into community very nicely! I'll probably want to request some changes though if thats ok with you (more on that in the rest of this post)

Tried to satisfy all your linting tools but had to force some of them (because it found some of the functions 'too complex') But I didn't want to split the coherent logic into small functions.

I'm ok with ignoring some stuff for sure (and definitely do so in scrapli) but generally I try to appease the linters where possible. Taking a quick stroll through the code I think there are possibly some things that can be shuffled around a bit that would appease linters (particularly the complexity checker) -- stuff like this could l likely be moved to its own function (though I could be wrong, again just a quick look so far!).

I'd probably also have some other nit pick type things like using more descriptive variables for stuff like this. Nothing wrong with what you've built of course, just trying to be consistent with other scrapli things and I tend to be probably too verbose in places hah.

Beside this feature I updated docs and in my drivers I used filename async_driver.py instead of _async.py. I guess it was to avoid namespace overlapping.

Yeah... I think that was my thinking but now I'm wondering what the hell namespace just async is!? I think I may have just been going crazy or something lol. Before this next pypi release (2021.07.30) I will try to rename all the stuff in community like how you did -- its just better your way 😁

Lastly I think I would want to have this start out as a "variant" -- since at some point I still have aspirations of moving the "core" platforms to community I think it would be good to leave space for those to be the "normal"/"default" driver, then have the asyncsp variant be what you've created here. Hopefully that makes sense!

So yes, this is freaking cool, and I'm totally down to get it merged in community but I may be a bit of a nit pick/annoyance in the process (just being honest!) -- so if you are interested in working through that with me then I'd be more than happy to work with ya on gettin that going. Otherwise, if that doesn't sound fun (dont blame you!) this is really cool and I'm happy to add a link to a blog/repo/video/whatever you want show casing this and putting it in the scrapli docs somewhere so folks can take advantage of the great work you did!

Carl

[viktorkertesz]

Hey Carl,
Sorry I put this aside for a long time. I checked scrapli-cfg and the way you use it along with existing connection. Do you think we could do that in a separate project like scrapli-scp and have something like this:

async with AsyncScrapli(**device) as conn:
    scp = AsyncScrapliSCP(conn=conn)
    result = await scp.file_transfer(operation='get', src="someios.bin", dst=".", overwrite=True, force_scp_config=False)

This way we can quickly release the async version. I lost a bit of interest implementing the sync version 🤔 Perhaps you prefer this separation from your core modules.

[carlmontanari]

Hey Viktor! No worries man!

I love the idea of having it behave like you've outlined. Think that makes it pretty nice and easy. Since this is just Cisco and just async, I wonder if we can just store it in a gist or a repo of "cool community things". That may be easy and we can disable issues for that repo/gist so there is no burden of maintenance, while still showing off the cool stuff you've built... thoughts?

[viktorkertesz]

Hi Carl!
I've created scrapli_scp repo with the initial version. I just converted it back from my work and combined with your scrapli-cfg approach. It was working in my initial testing. Of course it lacks proper documentation yet and only supports iosxe with asyncssh. I'll work on that from now on and leave your existing projects alone :)
Probably easier to maintain and extend it this way. Thanks for your projects, I learn a lot! ++for network automation talkshow!

[carlmontanari]

Awesome Viktor! I will add a link to your project in the docs somewhere! Thanks for all the great conversations!!