ovirt-engine-hyper-upgrade: general improvements #1

dougsland · 2017-10-24T21:18:16Z

The initial draft code was impressive but required some additional work. This patch is a re-factoring of initial code.

Using shell=True can be a security hazard. More info: https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

Helps clarify the code flow.

Options added initially: --check-upgrade-rhv-4-0 --check-upgrade-rhv-4-1 --list-enabled-repos

In case system have no repo available, display the msg to users.

Yedidyah Bar David

Merge check_rhv_40_repos and check_rhv_41_repos into check_rhv_repos

- Add docstring to get_enabled_repos - replace """ to '''

remove duplicate code

Requires: engine-setup, engine-upgrade-check and subscription-manager

Show programs output and commands being executed.

engine-setup and yum update requires

…-repos

- add print_err - print messaging - return returncode for the execute_cmd

it's not used at all

- no need to use print_err, cmds will show the message - extend yum update cmd

- use subprocess.check_output to get id from subscription-manager and parse - remove list-enabled-repos (not needed)

didib · 2017-10-31T11:09:44Z

ovirt-engine-hyper-upgrade.spec.in

+
+%build
+%if 0%{?enable_autotools}
+autoreconf -if


You also add autogen.sh, perhaps use it? If there is a specific reason not to, perhaps add a comment

In spec file we usually don't use helper scripts.

didib

Some comments inline.

Also, is this tool intended to handle also hosts, one day? If not, why do we have texts with 'RHV environments' instead of just 'engine' (or 'RHV Manager', which is how it's called in RHV)?

didib · 2017-10-31T11:12:56Z

ovirt-engine-hyper-upgrade.spec.in

@@ -76,6 +76,10 @@ make %{?_smp_mflags}
 %install
 make %{?_smp_mflags} install DESTDIR="%{buildroot}"

+%post
+ln -sf %{_sbindir}/ovirt-engine-hyper-upgrade %{_sbindir}/engine-hyper-upgrade
+ln -sf %{_mandir}/man8/ovirt-engine-hyper-upgrade.* %{_mandir}/man8/engine-hyper-upgrade.8.gz


Perhaps we can come up with a single name? We didn't commit to current name anywhere. Can even create a new repo with a new name and migrate. If you add this link because we also have 'engine-setup', then I am not sure that is a good choice either...

Added because of engine-setup. @didib @sandrobonazzola please let me know if you guys want a different name. Having 'ovirt-'engine-hyper-upgade because of upstream and engine-hyper-upgrade because of downstream is fine for me. On the other hand, the tool is downstream only but lives upstream.

didib · 2017-10-31T13:13:17Z

src/ovirt-engine-hyper-upgrade

@@ -16,16 +16,35 @@ import argparse
 import subprocess
 import sys

+from enum import Enum


Please add python-enum34 to the spec file. This is already an indirect dep of the engine, should be ok.

In python 2.7 contain enum, don't think it's required @didib.

cat /etc/redhat-release

Red Hat Enterprise Linux Server release 7.4 (Maipo)

python

Python 2.7.5 (default, May 3 2017, 07:55:04)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-14)] on linux2
Type "help", "copyright", "credits" or "license" for more information.

from enum import Enum
class MsgType(Enum):
... INFO = 1
... WARNING = 2
... ERROR = 3
...
print MsgType.INFO
MsgType.INFO

didib · 2017-10-31T13:16:03Z

src/ovirt-engine-hyper-upgrade

-       type
-           INFO - The message will be print as green
-           WARNING - The message will be print as yellow
+    fh = logging.FileHandler("/var/log/engine-hyper-upgrade.log")


Perhaps make this optional, can be done later

Perhaps have in a constant somewhere for now

Perhaps put in /var/log/ovirt-engine/something

Perhaps name ovirt-engine-hyper-upgrade.log, or rename also the package and binary. See other comment about symlinks...

I have added a const and under /var/log/ovirt-engine but about the name I will wait a suggestion from you guys :-)

didib · 2017-10-31T13:19:27Z

src/ovirt-engine-hyper-upgrade

-           WARNING - The message will be print as yellow
+    fh = logging.FileHandler("/var/log/engine-hyper-upgrade.log")
+    fh.setLevel(logging.DEBUG)
+    debug_fmt = logging.Formatter("%(asctime)s %(message)s",


Perhaps use same format as the engine, which wasn't very easy to get right.
otopi was even more complex, because I need to have it running well without being installed from rpm (so without extra deps).

For engine, check: packaging/pythonlib/ovirt_engine/service.py

See also: https://bugzilla.redhat.com/1408193

didib · 2017-10-31T13:23:15Z

src/ovirt-engine-hyper-upgrade

+
+        if type == MsgType.ERROR:
+            self.logger.error((
+                "\033[91m[ ERROR  ]\033[00m {msg}".format(msg=message)


Perhaps keep the escape sequence strings in named variables/constants for better readability

didib · 2017-10-31T13:41:31Z

src/ovirt-engine-hyper-upgrade

-        except subprocess.CalledProcessError as e:
-            sys.exit(2)
+        # We need to parse output of subscription-manager, using check_output
+        for line in execute_cmd_check_output(rhsm_cmd).splitlines():


I'd rather have:

output = something
for line in output.splitlines():

So that if the command fails, it's easier to understand that from the stack trace.

didib · 2017-10-31T13:57:45Z

ovirt-engine-hyper-upgrade.py

+        elif version == '4.1':
+            required = set(self.RHV_41_REPOS)
+        else:
+            raise RuntimeError('Version parameters not supported!')


I think text is not ideal.

Perhaps use something like check_releases you have below in a global dict/list and use that. Then you can simplify above code, and make the error message something like:

'Detected RHV version is {v}. This tool only supports the following versions: {versions}'.format(...)

or something like that.

didib · 2017-10-31T15:03:26Z

ovirt-engine-hyper-upgrade.py

-        output = execute_cmd(
-             self.SUBSCRIPTION_MANAGER_GET_ENABLED_REPOS
-        ).splitlines()
+        rhsm_cmd = list(self.SUBSCRIPTION_MANAGER)


IIUC you use 'list' because you want a copy. Some other options:

Use plain '+' - execute_cmd(self.SUBSCRIPTION_MANAGER + ['repos', '--list-enabled'])

Or a more efficient variation 'itertools.chain' - probably an overkill here - https://stackoverflow.com/questions/1720421/how-to-concatenate-two-lists-in-python

If you do want to copy, I know it's common to use a slice, self.SUBSCRIPTION_MANAGER[:] . Not sure why it's common, but someone tested and said it's the fastest: https://stackoverflow.com/questions/2612802/how-to-clone-or-copy-a-list/2612990#2612990

didib · 2017-10-31T15:08:16Z

src/ovirt-engine-hyper-upgrade

+        yum_update_engine_cmd = list(self.YUM_UPDATE_CMD)
+        yum_update_engine_cmd.extend(
+            'ovirt-engine-setup*',
+            'ovirt-engine-dwh-setup*'


I think we used to suggest:

yum update ovirt-engine*setup*

At the time, this included also ovirt-engine-reports. Perhaps today these are identical.

During my tests if I don't update ovirt-engine-dwh-setup* the engine-setup fails requiring an updated ovirt-engine-dwh-setup package. @didib

didib · 2017-10-31T15:20:10Z

ovirt-engine-hyper-upgrade.py

    )

    parser.add_argument(
        '--check-upgrade-rhv-4-1',
        action='store_true',
-        help='Check if RHV 4.1 channels have upgrade available',
+        help='Check if RHV 4.1 channels have zstream upgrade available. '
+             'Also enable 4.2 channels',


Are the texts for these two options intentionally different?

I am not sure we need them. Perhaps better to have a single option "--upgrade-to-version=", default to 4.2 currently. Not sure I understand the intention, though.

I am not sure about if we intend to support hosts in the future, @didib @sandrobonazzola might know.

dougsland · 2017-11-01T19:24:19Z

Talked with Didi via irc, merging now and I will work in the comments in separate patch.

dougsland added 30 commits October 18, 2017 13:13

code style: make flake8 happy

a246a73

license: Add license header

b02ee9f

python3: make 2to3 tool happy

9bc2e24

AUTHORS: Recognize people's work

9b687ec

typo: fix typo for subscription-manager

f777d6f

python: subprocess.check_output and shell=True

2628a42

Using shell=True can be a security hazard. More info: https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

UpgradeHelper is a class

412db37

use self for constants in the UpgradeHelper (now) class

0560143

using special method __name__ for main

266be2a

Adding some spaces

664bae2

Helps clarify the code flow.

python: Add argparse

90ba4f8

Options added initially: --check-upgrade-rhv-4-0 --check-upgrade-rhv-4-1 --list-enabled-repos

list-enabled-repos: Show error msg

b35141d

In case system have no repo available, display the msg to users.

AUTHORS: Add didi

e4928e0

Yedidyah Bar David

man: Adding initial manpage

2244183

help: improve --help message

933dbad

check_rhv_repos: remove duplicate code

5f79b1b

Merge check_rhv_40_repos and check_rhv_41_repos into check_rhv_repos

docstring: Adding docstring

8e4abae

- Add docstring to get_enabled_repos - replace """ to '''

merge enable_repo and disable_repo into repository

e800f15

remove duplicate code

packaging: Add autoconf and rpm

66c22fd

ovirt-engine-hyper-upgrade: remove duplicate code

826c7f3

spec: Add requirements

32ee4b4

Requires: engine-setup, engine-upgrade-check and subscription-manager

Add action into repository docstring

e38634c

ovirt-engine-hyper-upgrade: Display ovirt-engine-setup pkgs to update

9615eb3

ovirt-engine-hyper-upgrade: Add vim syntax in the end of file

8ff9c07

ovirt-engine-hyper-upgrade: execute_cmd show output

8a4cb06

Show programs output and commands being executed.

ovirt-engine-hyper-upgrade: enable interactive mode for cmds

2071867

engine-setup and yum update requires

ovirt-engine-hyper-upgrade: use shell=False and simplify list-enabled…

f2435db

…-repos

ovirt-engine-hyper-upgrade: make pep8 happy

bdc73bc

ovirt-engine-hyper-upgrade: few improvements

80c5eb0

- add print_err - print messaging - return returncode for the execute_cmd

ovirt-engine-hyper-upgrade: add env LC_ALL=C

c00a68c

dougsland added 14 commits October 23, 2017 14:26

ovirt-engine-hyper-upgrade: remove LIST_RPM_PKGS

437fc5c

it's not used at all

ovirt-engine-hyper-upgrade: more improvements

c145e9a

- no need to use print_err, cmds will show the message - extend yum update cmd

remove print in list enable repos

cce14e8

make pep8 happy

ac7b287

ovirt-engine-hyper-upgrade: fix typo

88a93d6

general improvements

f03f7c3

- use subprocess.check_output to get id from subscription-manager and parse - remove list-enabled-repos (not needed)

remove print_output parameter

7ed32fd

general improvements

8e07034

ovirt-engine-hyper-upgrade: no need to ask users, let yum do

384c01d

ovirt-engine-hyper-upgrade: Use execute_cmd

3c7565d

link to engine-hyper-upgrade

c2163ab

adding logging support

6008ec6

trivial improvements

dca5903

more logging

b6846b1

dougsland assigned dougsland and sandrobonazzola and unassigned dougsland Oct 24, 2017

dougsland added the enhancement label Oct 24, 2017

dougsland requested a review from sandrobonazzola October 24, 2017 21:19

sandrobonazzola requested a review from didib October 25, 2017 13:15

didib reviewed Oct 31, 2017

View reviewed changes

dougsland merged commit d349345 into oVirt:master Nov 1, 2017

ovirt-engine-hyper-upgrade: general improvements #1

ovirt-engine-hyper-upgrade: general improvements #1

dougsland commented Oct 24, 2017

didib Oct 31, 2017

dougsland Oct 31, 2017

didib left a comment

didib Oct 31, 2017

dougsland Nov 4, 2017

didib Oct 31, 2017

dougsland Nov 4, 2017

didib Oct 31, 2017

dougsland Nov 4, 2017

didib Oct 31, 2017

didib Oct 31, 2017

didib Oct 31, 2017

dougsland Nov 4, 2017

didib Oct 31, 2017

didib Oct 31, 2017

didib Oct 31, 2017

dougsland Nov 4, 2017

didib Oct 31, 2017

dougsland Nov 4, 2017

dougsland commented Nov 1, 2017

ovirt-engine-hyper-upgrade: general improvements #1

ovirt-engine-hyper-upgrade: general improvements #1

Conversation

dougsland commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cat /etc/redhat-release

python

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougsland commented Nov 1, 2017