Add remote_edit method into virttest/remote.py.

[yangdongsheng]

Signed-off-by: yangdongsheng yangds.fnst@cn.fujitsu.com

[yangdongsheng]

@cevich
Add a method remote_edit to remote module.
"""
Edit file on a remote host (guest) by the selected client.
This method rely on copy_files_from and copy_files_to.
"""

please take a look and give me a some feedback.

[ldoktor]

Hi @yangdongsheng do you have some examples where this function would be used? Because it all looks too complicated to me, but maybe I'm wrong...

Additionally is this targeted for small files and few patterns, or do you intend to modify large files too? Because the code is not what I'd call optimized.

[yangdongsheng]

Thanx @ldoktor for your comment.

Yes, in some conditions we need to edit a remote file.
e.g I need to edit /etc/sysconfig/libvirtd to let a remote libvirt --listen in test for virsh connect with transport tcp or tls.
And there are some more cases to use this function, such as grub.conf in remote guest, hosts in remote, /etc/libvirt/libvirtd.conf in remote host and so on. Lots of code have the same purpose.

I think I should add a edit_file function in virttest/utils_misc.py at first to edit file on local host,
and then implement the remote_edit by calling it.

Yes, the pattern&value method to edit file is not convenient to users, but the functionality to edit remote file is necessary I think. Do you agree?

Could you give me some suggestion to make this function more pretty?

[ldoktor]

Yes I agree these functions might be valuable.

My first thought was sed. It's a nice way to modify files directly on linux guests (yes, I did lots of bash scripting before I learned python so my view is probably affected by this). For fancy modifications (multiple/advanced substitution, ...) I'd probably rather modify the file directly in the test.

Anyway after a decent consideration I can see the benefit of having unified way for all OSs. In order to make the call simple I'd imagine those variants:

• append - simple append
• delete - delete matching lines using line in patterns (for advanced matching sub variant might be used
• sub - re.sub() - we should be able to set re flags

using of re.sub() and re.match is a common way and should be a readable enough in test so I guess we could use that.

Also from what you describe it's going to be used on cfg files so the optimization is not a big issue (although to speed things up you might use re.compile() and use the re directly on the content of the file (using .read).

Examples:

cat /etc/exports
/home/medic     192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)
APPEND, '/tmp/a/         192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)'
SUB r'\/tmp\/a\S+.*\n?'
DEL '/home/medic     192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)'
cat /etc/exports

[ldoktor]

Also one more thing, the backup files needs to be renamed in case we backup the same file from multiple guests or the same file multiple times...

[cevich]

I think there will always be cases where this or that tool or method is better suited for some operation. Also, many modifications are dependant on success of prior operations. A one-by-one approach with many conditional calls to this function and keeping track of multiple backups, possibly from several hosts, could quickly become quite cumbersome.

@yumingfei and I have been discussing this problem off-and-on for a while now. Based on that, and my tinkering around, I think extremely simple solutions are best here. How about a function or class that behaves like this:

0. remote_login() to host and backup all files to-be-modified.
1. copy_files_to() sends a script (language independent)
2. remote_login's session executes script passing in arbitrary parameter values
3. if exit code is non-zero
   3a) Script is killed if it's still running
   3b) backed up files are restored
   3c) exit code returned to caller
4. if exit code is zero, return 0 to caller.

We can stick these scripts into a test-specific directory under shared/scripts/libvirt/.... The disadvantage to this, is extra the effort to deal with another set of files. However, an advantage could be that extending the scripts would be easy, for example if it's basename is referenced from params.

A slightly more complicated alternative is to use something like the inspect module to pull the code out of a class defined within the test-module, ship that over to the remote system, and execute it (somehow). I did a POC on this, hooked up to a SimpleXMLRPCServer instance running on the guest. It was "cool", and provided a shared namespace, but I think simple is probably better.

[ldoktor]

👍 for simple solution with one concern. If I understand it correctly you want to create script which would modify the file on the target machine. If you wanted to do it on windows, you'd have to use windows cmd script (which is not that rich as linux bash) as if I'm not mistaken python is not automatically installed in windows.

About the multiple backups I can imagine another solution. We could use a class. You'd specify the changes and would be able to revert back to the original file. Also on __del__ it could call this revert automatically. This way you'd have one class for each modified file.

Anyway I agree that simplest solution is always the best as everybody who just wants to know what this test does should understand it and not study autotest internals to understand that this weird call just changes the default initlevel to 5.

[yangdongsheng]

Hi ldoktor https://github.com/ldoktor
On 06/11/2013 06:40 PM, Lukáš Doktor wrote:

Yes I agree these functions might be valuable.

My first thought was |sed|. It's a nice way to modify files directly
on linux guests /(yes, I did lots of bash scripting before I learned
python so my view is probably affected by this)/. For fancy
modifications (multiple/advanced substitution, ...) I'd probably
rather modify the file directly in the test.

Anyway after a decent consideration I can see the benefit of having
unified way for all OSs. In order to make the call simple I'd imagine
those variants:

• append - simple append
• delete - delete matching lines using |line in patterns| (for
  advanced matching |sub| variant might be used
• sub - |re.sub()| - we should be able to set re flags

Do you mean we should support three actions for user that append, delete
and sub. But what's the mean of sub? searching a pattern?
Actually, In my code, I supported three actions named add, remove and
replace.
What do you think about it??

using of re.sub() and re.match is a common way and should be a
readable enough in test so I guess we could use that.

Also from what you describe it's going to be used on cfg files so the
optimization is not a big issue (although to speed things up you might
use |re.compile()| and use the re directly on the content of the file
(using |.read|).

Examples:

cat /etc/exports
/home/medic 192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)

APPEND, '/tmp/a/ 192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)'
SUB r'/tmp/a\S+.*\n?'
DEL '/home/medic 192.168.122.0/24(rw,all_squash,anonuid=1000,anongid=1000,async,insecure)'

cat /etc/exports

—
Reply to this email directly or view it on GitHub
#526 (comment).

[yangdongsheng]

Yes, rename backup files is necessary. Thanx

On 06/11/2013 06:41 PM, Lukáš Doktor wrote:

Also one more thing, the backup files needs to be renamed in case we
backup the same file from multiple guests or the same file multiple
times...

—
Reply to this email directly or view it on GitHub
#526 (comment).

[yangdongsheng]

@cevich @yumingfei and @ldoktor
I agree with doctor. The reason to rely on copy_files_* when i did this function at first is I think
we need reduce the dependence to remote environment which is out of our control.
And I will add a edit method in utils_misc.py to edit local file. remote_edit will call this function.
Is that ok to add it in utils_misc.py????
And is there some sugguestions for the design of this method??
Some advice to simplify the params in the V1???

[ldoktor]

I'd support the methods I described in #526 (comment)

• ADD - appends $data at the end of the file
• SUB - executes re.sub using $data as regext
• DEL - copy only lines which doesn't match re.match($data)

The function should return the path to the backup. For matching use compiled regexp. (a = re.compile, a.sub(...)). But that's only my idea, don't take it too seriously.

[cevich]

Ahh ya, forgot about Windows and cmd. I would love it if we could reduce out of control dependencies :D Seriously though, we do have some things we can count on, including a python environment. But I'm not sure we have many tests which must run using exactly the same parameters on multi-OS. I think it's okay to have different variants and scripts for different OSs, as long as they meet the same requirements for test. If that makes sense.

[yumingfei]

I'd rather copy to local and modify it, because we may lose control to modified files if remote host is disconnect during executing scripts.

[yangdongsheng]

@cevich @yumingfei and @ldoktor
I did some update on this branch.
A class named RemoteFile in added into remote module. And there are some unittest for it.
Please give me some comments.
Thanx~ :)

[ldoktor]

My first impression based on review is very nice. I only pointed out that using NamedTemporaryFile could be better, but apart from that I like it. Still have to test it physically. Thank you for the changes.

For consideration we could add libguestfs support later to modify non-running VMs (eg. when we screw-up some setting and the VM won't boot afterwards...)

[yangdongsheng]

Thanx @ldoktor for your review and helpful comment.
I Add a fix commit for it. Some other suggestion?

[yangdongsheng]

Hi maitainers:
If this patch looks good to you, please merge it. My another pull request is
waiting for it applied.

[ldoktor]

OK I tested it on a boot test and it worked fine. People just have to be careful to use list of strings and not strings itself (I'd maybe consider check for simple string and in that case warn user or even throw exception as it's currently it accepts it and iterates over chars... (that's why I first thought it's broken)

Anyway it works fine as it is now and we can add new features later. @cevich you already saw this one, would you please take a look on it? (Additionally might I ask you to take care of the merge? I'll work from train tomorrow and I might not be able to connect)

Acked-by: Lukáš Doktor ldoktor@redhat.com

[ldoktor]

@ypu @FengYang you too might be interested in this one.

[ldoktor]

^^ check_patch also found indentation mistake which needs to be taken care of during merge Reindenting virttest/remote.py

[yangdongsheng]

@ldoktor
:( My bad. I add a signed-off in last commit. Please check.

[yangdongsheng]

Hi @ldoktor , would you take a look at this pull request? I think it is ready to go.
If it looks good to you, please merge it.

Thanx :)

[ypu]

This patch looks good to me.

checked it with check_patch.py, and run the unit test for it. Everything seems fine. And as it already Acked by @ldoktor. So merge it.

Thanks @yangdongsheng