Bug 1828981 - [RFE] Add common argument parsing and configuration file for command line examples
Summary: [RFE] Add common argument parsing and configuration file for command line exa...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: ovirt-engine-sdk-python
Classification: oVirt
Component: Core
Version: 4.3.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ovirt-4.4.4
: ---
Assignee: Nir Soffer
QA Contact: Ilan Zuckerman
URL:
Whiteboard:
Depends On:
Blocks: 1848586
TreeView+ depends on / blocked
 
Reported: 2020-04-28 17:02 UTC by Nir Soffer
Modified: 2021-05-19 10:20 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-12-21 12:35:39 UTC
oVirt Team: Storage
Embargoed:
pm-rhel: ovirt-4.4?
pm-rhel: planning_ack?
pm-rhel: devel_ack+
pm-rhel: testing_ack+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
oVirt gerrit 109351 0 master ABANDONED examples: use common arg parser in upload_disk_snapshots 2021-02-19 17:57:12 UTC
oVirt gerrit 110807 0 master MERGED common: Read engine configuration from config file 2021-02-19 17:57:12 UTC
oVirt gerrit 110808 0 master MERGED upload_disk: Use common helpers 2021-02-19 17:57:12 UTC
oVirt gerrit 110809 0 master MERGED download_disk: Use common helpers 2021-02-19 17:57:11 UTC
oVirt gerrit 110810 0 master MERGED backup_vm: Use common helpers 2021-02-19 17:57:12 UTC
oVirt gerrit 110811 0 master MERGED upload_from_ova: Use common helpers 2021-02-19 17:57:12 UTC

Description Nir Soffer 2020-04-28 17:02:51 UTC
Description of problem:

Lot of examples use the same engine configuration options:
- engine url (e.g. https://my-engine/
- username (e.g. admin@internal)
- password 
- cafile (e.g. /etc/pki/vdsm/certs/cacert.pem on a host)
- insecure (do not verify server certs)

Most examples require changing the example code to set the right parameters.
This means that the examples files are likely not tested properly because
testing require modifying the code before you post a patch. Since there are
no tests for the example code, it is likely to be broken.

Some examples (upload_disk.py, download_disk.py, backup_vm.py) provide
a command line interface to set these values:

> python3 /root/ovirt-engine-sdk/sdk/examples/upload_disk.py \
>     --engine-url "https://`hostname`/" \
>     --username admin@internal \
>     --password-file pssword  \
>     --cafile /tmp/ca.pem \

This make these examples testable and real useful command line tool, and
these are very useful for developers.

However for users such long command line is too much pain to use. We need 
to move the common engine config to single configuration file, so we can
use:

    python3 /root/ovirt-engine-sdk/sdk/examples/upload_disk.py \
        -c my-engine.conf

Or even:

    python3 /root/ovirt-engine-sdk/sdk/examples/upload_disk.py ...

Reading the configuration file from documented location like:
~/.config/ovirt-engine-sdk/my-engine.conf


We need common code which should be part of the SDK for reading the
configuration file and for creating a command line parser populated
with the common options that all examples should have.

    from sdk4 import cli

    def main():
        # Return argparse.ArgumentParser populated with common options.
        parser = cli.argument_parser()

        # Add example specific arguments
        parser.add_argument("--foo", help="specific example argument")

        args = parser.parse_args()

        # Create connections from standard configuration file.
        connection = sdk.Connection.fromconf(args.engine_config)

Configuration file format:

# ovirt-engine-sdk configuration file

[engine]
# engine url without the "ovirt-engine/" path component.
url = https://my.engine/

# supported types:
# password:value - embed password in the config file (insecure)
# file:filenae - read password from file
# leave empty to read password from stdin (using getpass)
password = 

# engine username
username = admin@internal

# cafile - path to engine certificate
# can be downloaded from
# https://my.engine/ovirt-engine/services/pki-resource?resource=ca-certificate&format=X509-PEM-CA
# Leave empty to use host trusted CA
cafile = 

# insecure - do not verify server certificate (not recommendded)
insecure = false


The default search path for the configuration can be:
.config/ovirt-engine-sdk/engine.conf

User can keep multiple configurations if they need to work with multiple
engines.


With this it should be easy to add testable and useful examples without 
duplicating the boilerplate for every example.

This can also help users to write consistent command line tools using
ovirt engine sdk.


This can be implemented by for other language bindings.

Comment 1 Martin Perina 2020-06-18 13:57:37 UTC
Examples in SDK should show a usage of SDK, but they are not mean to be a real command line tools. Closing due to capacity

Comment 2 Nir Soffer 2020-06-18 14:14:53 UTC
Martin, we already work on this here:
https://gerrit.ovirt.org/c/109609/
https://gerrit.ovirt.org/c/109351/

Of course we plan to move this code to the new ovirt-img command, but
we did start to work on it yet, and the code in the examples is used
by QE and developer, and going to be used by backup vendors soon.

Comment 3 Nir Soffer 2020-08-19 11:27:14 UTC
This is implemented in the attached patches.

Comment 4 Nir Soffer 2020-08-19 11:31:02 UTC
Since the affected examples are only storage, and used by storage
automated tests, I think this should be tested by storage QE.

Shir, can you confirm?

Comment 5 Nir Soffer 2020-08-19 11:38:41 UTC
Here is an example usage with the new configuration:

$ cat /home/nsoffer/.config/ovirt.conf 
[engine3]
engine_url = https://engine3
username = admin@internal
password = enginepassword
cafile = /home/username/certs/engine3.pem

$ ./upload_disk.py -c engine3 --sd-name nfs1 --disk-sparse fedora-32.raw 
[   0.0 ] Checking image...
[   0.0 ] Image format: raw
[   0.0 ] Disk format: raw
[   0.0 ] Disk content type: data
[   0.0 ] Disk provisioned size: 6442450944
[   0.0 ] Disk initial size: 6442450944
[   0.0 ] Disk name: fedora-32.raw
[   0.0 ] Disk backup: False
[   0.0 ] Connecting...
[   0.0 ] Creating disk...
[   4.2 ] Disk ID: 157494d3-3f93-401a-88b7-792f0ea395c1
[   4.2 ] Creating image transfer...
[   5.5 ] Transfer ID: 7647c957-7c1b-4f7d-998b-25a11399942d
[   5.5 ] Transfer host name: host4
[   5.5 ] Uploading image...
[ 100.00% ] 6.00 GiB, 2.82 seconds, 2.13 GiB/s                                 
[   8.3 ] Finalizing image transfer...
[  11.3 ] Upload completed successfully

Comment 6 Martin Perina 2020-08-20 08:54:39 UTC
(In reply to Nir Soffer from comment #2)
> Martin, we already work on this here:
> https://gerrit.ovirt.org/c/109609/
> https://gerrit.ovirt.org/c/109351/
> 
> Of course we plan to move this code to the new ovirt-img command, but
> we did start to work on it yet, and the code in the examples is used
> by QE and developer, and going to be used by backup vendors soon.

As I told several times, examples in SDK should be used as a code pieces to create custom applications. Those examples should not be used as a command line tools in production. Just please cosinder also the effort needed to maintain this additional code you want to get into SDK release, because exactly command line parsing or reading configuration file should not be included in examples, because it has nothing to do with SDK. So I expect that all those code will be moved to ovirt-img-tool and SDK examples will really only contain example how to call a specific function ...

Comment 7 Nir Soffer 2020-08-20 19:46:09 UTC
(In reply to Martin Perina from comment #6)
Ok, will move this bug to imageio. The posted patched will be part
of ovirt-img.

Comment 8 Eyal Shenitzky 2020-11-17 17:27:13 UTC
The patches already merged and tested with the following SDK version - python3-ovirt-engine-sdk4-4.4.3-1

Moving to VERIFIED

Comment 9 Sandro Bonazzola 2020-12-21 12:35:39 UTC
This bugzilla is included in oVirt 4.4.4 release, published on December 21st 2020.

Since the problem described in this bug report should be resolved in oVirt 4.4.4 release, it has been closed with a resolution of CURRENT RELEASE.

If the solution does not work for you, please open a new bug report.


Note You need to log in before you can comment on or make changes to this bug.