Disk space calculations for controller file system are different in Python 2 and Python3

Bug #1862668 reported by Jessica Castelino
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
StarlingX
Fix Released
Low
Jessica Castelino

Bug Description

Brief Description
-----------------
This bug was uncovered while writing new test cases for sysinv API.

The below two calculations in controller file system return different values when run in Python2 and Python3:
 (a) Calculating the space for controller file system (cgtsvg_max_free_GiB)
 (b) Calculating the growth for controller filesystem (cgtsvg_growth_gib)

The division operations return different values.
Eg. 2/3 = 0 (Python 2)
    2/3 = 0.66 (Python 3)

In order to replicate the same Python 2 behavior, we can make the following change in Python 3.
    2//3 = 0 (Python 3)

Also, as the controller file system source code is undergoing modification, it would be a great idea to improve the logic in the first if-else block in _get_controller_cgtsvg_limit() which repeats the same lines of code in the else block. Eg. can use a dictionary to store the values of cgtsvg0_free_mib and cgtsvg1_free_mib.

Severity
--------
Minor

Steps to Reproduce
------------------
NA for unit test cases

Expected Behavior
------------------
The controller filesystem growth and space must return the same values in Python 2 and 3.

Actual Behavior
----------------
The division operations done for calculating controller filesystem growth and space yield different results in Python 2 and 3.

Reproducibility
---------------
100% reproducible

System Configuration
--------------------
One node system

Branch/Pull Time/Commit
-----------------------
2020-02-10_10-52-51

Last Pass
---------
Unknown as this is a new unit test case.

Timestamp/Logs
--------------
NA for unit test cases

Test Activity
-------------
Unit testing

Revision history for this message
Al Bailey (albailey1974) wrote :

Jessica, if you update the sysinv tox.ini for the py27 section and add:

setenv = {[testenv]setenv}
         PYTHON=python2.7 -3

then when you run "tox -e py27" it will display any other places in the sysinv code that also have potential errors like this, and should display:

DeprecationWarning: classic int division

(however it will display quite a few others as well)

ex:
sysinv/api/middleware/parsable_error.py:71: DeprecationWarning: classic int division
  if (state['status_code'] / 100) not in (2, 3):

Revision history for this message
Al Bailey (albailey1974) wrote :

Kristine may want to examine the algorithm being used.
Python2 uses integer math, but python3 uses floating point.

Example:

This unit test:
https://opendev.org/starlingx/config/src/branch/master/sysinv/sysinv/sysinv/sysinv/tests/api/test_controller_fs.py#L411

Python2 the result is:
Total target growth size 9 GiB for database (doubled for upgrades), platform, scratch, backup and extension exceeds growth limit of 0 GiB.

Python3, the result is:
Total target growth size 8.0 GiB for database (doubled for upgrades), platform, scratch, backup and extension exceeds growth limit of 1.540139

It is trivial to make the python3 work like python2, but the python2 formula should be re-examined, in case it is not accurate.

Revision history for this message
Ghada Khalil (gkhalil) wrote :

Assigning to Kristine Bujold as suggested by Al Bailey.
I've marked this as low / not gating for now given it's not clear whether python3 will be delivered in stx.4.0

tags: added: stx.config
tags: added: stx.python2
Changed in starlingx:
importance: Undecided → Low
status: New → Triaged
assignee: nobody → Kristine Bujold (kbujold)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to config (master)

Reviewed: https://review.opendev.org/707724
Committed: https://git.openstack.org/cgit/starlingx/config/commit/?id=34e410821b7b0699444b303fcdec1ab89d860cc6
Submitter: Zuul
Branch: master

commit 34e410821b7b0699444b303fcdec1ab89d860cc6
Author: Jessica Castelino <email address hidden>
Date: Thu Feb 13 15:38:51 2020 -0500

    Fix inconsistent disk space calculation

    Integer division in Python 2 behaves like floating-point
    division in Python 3. Thus, changes are made to rectify this
    behavior.

    Change-Id: I6a5905a4d97df5b9e73e165580801c865006f316
    Signed-off-by: Jessica Castelino <email address hidden>
    Closes-Bug: 1862668

Changed in starlingx:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.opendev.org/707713
Committed: https://git.openstack.org/cgit/starlingx/config/commit/?id=e6e37c949a39e4ee3d4f4c9407a85089e7514345
Submitter: Zuul
Branch: master

commit e6e37c949a39e4ee3d4f4c9407a85089e7514345
Author: Jessica Castelino <email address hidden>
Date: Mon Feb 10 16:26:13 2020 -0500

    Added unit test cases for host file system.

    Test cases added for API endpoints used by:
     1. host-fs-list
     2. host-fs-modify
     3. host-fs-show

    This commit also fixes the issue of Host FS disk space calculations
    yielding different values in Python 2 and Python 3.

    Change-Id: I50a1ca43c43c3bba30730c616b3788664920d0c9
    Story: 2007082
    Task: 38013
    Partial-Bug: 1862668
    Signed-off-by: Jessica Castelino <email address hidden>

Revision history for this message
Ghada Khalil (gkhalil) wrote :

The code was fixed to work with python3 by Jessica Castelino. Whether the formula is accurate is considered outside the scope of this bug.

Changed in starlingx:
assignee: Kristine Bujold (kbujold) → Jessica Castelino (jcasteli)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to config (f/centos8)

Fix proposed to branch: f/centos8
Review: https://review.opendev.org/716137

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to config (f/centos8)
Download full text (32.3 KiB)

Reviewed: https://review.opendev.org/716137
Committed: https://git.openstack.org/cgit/starlingx/config/commit/?id=cb4cf4299c2ec10fb2eb03cdee3f6d78a6413089
Submitter: Zuul
Branch: f/centos8

commit 16477935845e1c27b4c9d31743e359b0aa94a948
Author: Steven Webster <email address hidden>
Date: Sat Mar 28 17:19:30 2020 -0400

    Fix SR-IOV runtime manifest apply

    When an SR-IOV interface is configured, the platform's
    network runtime manifest is applied in order to apply the virtual
    function (VF) config and restart the interface. This results in
    sysinv being able to determine and populate the puppet hieradata
    with the virtual function PCI addresses.

    A side effect of the network manifest apply is that potentially
    all platform interfaces may be brought down/up if it is determined
    that their configuration has changed. This will likely be the case
    for a system which configures SR-IOV interfaces before initial
    unlock.

    A few issues have been encountered because of this, with some
    services not behaving well when the interface they are communicating
    over suddenly goes down.

    This commit makes the SR-IOV VF configuration much more targeted
    so that only the operation of setting the desired number of VFs
    is performed.

    Closes-Bug: #1868584
    Depends-On: https://review.opendev.org/715669
    Change-Id: Ie162380d3732eb1b6e9c553362fe68cbc313ae2b
    Signed-off-by: Steven Webster <email address hidden>

commit 45c9fe2d3571574b9e0503af108fe7c1567007db
Author: Zhipeng Liu <email address hidden>
Date: Thu Mar 26 01:58:34 2020 +0800

    Add ipv6 support for novncproxy_base_url.

    For ipv6 address, we need url with below format
    [ip]:port

    Partial-Bug: 1859641

    Change-Id: I01a5cd92deb9e88c2d31bd1e16e5bce1e849fcc7
    Signed-off-by: Zhipeng Liu <email address hidden>

commit d119336b3a3b24d924e000277a37ab0b5f93aae1
Author: Andy Ning <email address hidden>
Date: Mon Mar 23 16:26:21 2020 -0400

    Fix timeout waiting for CA cert install during ansible replay

    During ansible bootstrap replay, the ssl_ca_complete_flag file is
    removed. It expects puppet platform::config::runtime manifest apply
    during system CA certificate install to re-generate it. So this commit
    updated conductor manager to run that puppet manifest even if the CA cert
    has already installed so that the ssl_ca_complete_flag file is created
    and makes ansible replay to continue.

    Change-Id: Ic9051fba9afe5d5a189e2be8c8c2960bdb0d20a4
    Closes-Bug: 1868585
    Signed-off-by: Andy Ning <email address hidden>

commit 24a533d800b2c57b84f1086593fe5f04f95fe906
Author: Zhipeng Liu <email address hidden>
Date: Fri Mar 20 23:10:31 2020 +0800

    Fix rabbitmq could not bind port to ipv6 address issue

    When we use Armada to deploy openstack service for ipv6, rabbitmq
    pod could not start listen on [::]:5672 and [::]:15672.
    For ipv6, we need an override for configuration file.

    Upstream patch link is:
    https://review.opendev.org/#/c/714027/

    Test pass for deploying rabbitmq service on both ipv...

tags: added: in-f-centos8
Ghada Khalil (gkhalil)
tags: added: stx.python3
removed: stx.python2
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.