Test container 1.4.0

[resmo]

Seems the failing tests are related to random zone order return by by the API: also see https://github.com/apache/cloudstack/issues/3934

[codecov[bot]]

Codecov Report

Merging #40 (4fc4717) into master (43ec879) will decrease coverage by 6.67%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   84.46%   77.78%   -6.68%     
==========================================
  Files          53       32      -21     
  Lines        5509     3642    -1867     
  Branches     1246      846     -400     
==========================================
- Hits         4653     2833    -1820     
- Misses        429      492      +63     
+ Partials      427      317     -110     

Impacted Files	Coverage Δ
.../ngine_io/cloudstack/plugins/modules/cs_project.py	45.34% <0.00%> (-38.38%)	:arrow_down:
...ine_io/cloudstack/plugins/modules/cs_sshkeypair.py	50.43% <0.00%> (-36.53%)	:arrow_down:
...ions/ngine_io/cloudstack/plugins/modules/cs_vpc.py	68.57% <0.00%> (-22.86%)	:arrow_down:
...ngine_io/cloudstack/plugins/modules/cs_template.py	39.53% <0.00%> (-20.94%)	:arrow_down:
..._io/cloudstack/plugins/modules/cs_securitygroup.py	87.50% <0.00%> (-8.34%)	:arrow_down:
...e_io/cloudstack/plugins/module_utils/cloudstack.py	64.26% <0.00%> (-8.26%)	:arrow_down:
...ons/ngine_io/cloudstack/plugins/modules/cs_zone.py	86.58% <0.00%> (-4.88%)	:arrow_down:
.../ngine_io/cloudstack/plugins/modules/cs_account.py	81.06% <0.00%> (-1.52%)	:arrow_down:
...ngine_io/cloudstack/plugins/modules/cs_instance.py	70.84% <0.00%> (-0.69%)	:arrow_down:
...ons/ngine_io/cloudstack/plugins/modules/cs_host.py	85.03% <0.00%> (-0.69%)	:arrow_down:
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 43ec879...9a5eb5c. Read the comment docs.

[rvalle]

mmm now I feel bad because when I run the integration tests with the patched image they passed. I wonder what happened...

[rvalle]

after reviewing https://github.com/apache/cloudstack/issues/3934

I think it is possible that this has never worked, or worked by chance.

The restructuring of the cloudstack db query utility does not seem trivial.

We could also redefine the way in which ansible modules work and take the zone with lower ID if no zone is provided. We could also include a warning alerting that the behaviour has changed, as other ansible modules do.

Unless you are certain that the sortKey,id worked before, chances are that the lower ID was returned first by chance and our new default behaviour will not introduce changes.

[rvalle]

I think this is it: https://github.com/apache/cloudstack/pull/3242/commits/20f0ebe81bf0a3e61a6c151f9206c9be1e2fefd3 it seems that before sort key was introduced the rows were returned in no particular order. It would be up to the DB to decide but my best guess is that they were returned in the order in which they were created, which would match the ID order.

[resmo]

What I can say I never experienced a random zone returned since the last 6 years.

[rvalle]

sure, and it is since they introduced the sort column that order has changed.

but before there was no sort clause in the query, thus it would be up to the DB to pick the order. with such a small table, in practice that would mean order equal to creation order which, since ID is autoincremented, would be equal to ID order, which is what you expect now.

The point is that if we make the default zone the one with the smaller ID we would not be breaking anything.

[rvalle]

we could take the old simulator, create 2 zones: 1 and 2. then go and update the db and replace the id of 1 by 10. listing the zones would return 10,2.

I would bet a beer or two on that.

[resmo]

we could take the old simulator, create 2 zones: 1 and 2. then go and update the db and replace the id of 1 by 10. listing the zones would return 10,2.

I would bet a beer or two on that.

sounds like a plan,

[rvalle]

@resmo I lost my bet.

rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_1_start_image.sh 
2d555669028a88772b0ca8a1dc3b56dbc3755a1549e7f72129edb91f70e61965
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_2_get_zone_list.sh 
{
  "count": 2, 
  "zone": [
    {
      "allocationstate": "Enabled", 
      "dhcpprovider": "VirtualRouter", 
      "dns1": "8.8.8.8", 
      "id": "feb11a84-a093-45eb-b84d-7f680313c40b", 
      "internaldns1": "8.8.8.8", 
      "localstorageenabled": false, 
      "name": "Sandbox-simulator-basic", 
      "networktype": "Basic", 
      "securitygroupsenabled": true, 
      "tags": [], 
      "zonetoken": "967322cd-bbc1-3ebd-b0dd-10b18146d1c9"
    }, 
    {
      "allocationstate": "Enabled", 
      "dhcpprovider": "VirtualRouter", 
      "dns1": "10.147.28.7", 
      "guestcidraddress": "10.1.1.0/24", 
      "id": "09d0ea06-a44f-4be4-9ede-cec66eeedd0a", 
      "internaldns1": "10.147.28.7", 
      "localstorageenabled": false, 
      "name": "Sandbox-simulator-advanced", 
      "networktype": "Advanced", 
      "securitygroupsenabled": false, 
      "tags": [], 
      "zonetoken": "8bac6e10-14ea-3c27-810c-0e273e456430"
    }
  ]
}
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_3_stop_acs.sh 
cloudstack: stopped
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_4_patch_db.sh 
id  uuid    name
2   09d0ea06-a44f-4be4-9ede-cec66eeedd0a    Sandbox-simulator-advanced
10  feb11a84-a093-45eb-b84d-7f680313c40b    Sandbox-simulator-basic
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ 
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_5_start_acs.sh 
cloudstack: started
rafael@AsusRaf:~/Development/oss/cloudstack-zone-ip$ ./step_6_zone_list.sh 
{
  "count": 2, 
  "zone": [
    {
      "allocationstate": "Enabled", 
      "dhcpprovider": "VirtualRouter", 
      "dns1": "10.147.28.7", 
      "guestcidraddress": "10.1.1.0/24", 
      "id": "09d0ea06-a44f-4be4-9ede-cec66eeedd0a", 
      "internaldns1": "10.147.28.7", 
      "localstorageenabled": false, 
      "name": "Sandbox-simulator-advanced", 
      "networktype": "Advanced", 
      "securitygroupsenabled": false, 
      "tags": [], 
      "zonetoken": "8bac6e10-14ea-3c27-810c-0e273e456430"
    }, 
    {
      "allocationstate": "Enabled", 
      "dhcpprovider": "VirtualRouter", 
      "dns1": "8.8.8.8", 
      "id": "feb11a84-a093-45eb-b84d-7f680313c40b", 
      "internaldns1": "8.8.8.8", 
      "localstorageenabled": false, 
      "name": "Sandbox-simulator-basic", 
      "networktype": "Basic", 
      "securitygroupsenabled": true, 
      "tags": [], 
      "zonetoken": "967322cd-bbc1-3ebd-b0dd-10b18146d1c9"
    }
  ]
}

For some reason the ID order is preserved. I am not sure where that is implemented, but not in the Filter/Query that the DB utility is using.

In any case, we could still adopt the fact that the zone with the lower ID is in fact the default zone. This test also points in that direction.

The order key does not seem like a reliable ID, as it is not unique/enforced, etc. I think it is designed more as an UI preferred sorting key than any other thing.

[rvalle]

@resmo As a recap about this issue:

as discussed in https://github.com/apache/cloudstack/issues/3934:

SortKey was introduced to allow sorting on the Zones in the User Interface.

Prior to this Zones where returned in the API by ID order, and Ansible Modules made the first returned zone as "default zone".

sortKey introduced randomization of API order because it is not a primary key.

Using sortKey,ID as orderBy would made the order deterministic. but requires a bit of re-architecture in ACS.

@resmo , I still wonder if we should allow that changes in "sortKey" redefine what the default zone is. I wonder if someone could change the order of zones in the UI, without understanding that all playbooks will run against a different zone from now on.

We need to decide if we push for ACS to use Sortkey+ID as orderBy or we change how we define the default zone.

How do you want to proceed?

[resmo]

How do you want to proceed?

Because we can not rely on the order any longer, it is probably a good idea to turn zone into a required parameter. Would you be okay with that?

[rvalle]

Agree. I think that is the best option.

[resmo]

I decided to deprecate the default zone selection behavior first, so we won't break backwards compatibility