openvswitch_db: Make 'key' parameter optional #42110

cubeek · 2018-06-29T14:07:02Z

The OVSDB schema consists of typed columns. The 'key' parameter is
required only for columns with type of a 'map'. This patch makes 'key'
an optional parameter to allow setting values for other column types
like int.

Fixes #42108

SUMMARY

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

openvswitch_db

ANSIBLE VERSION

ansible 2.7.0.dev0

ADDITIONAL INFORMATION

Before:

ansible-playbook 2.5.5
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/vagrant/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible-playbook
  python version = 2.7.5 (default, Apr 11 2018, 07:36:10) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]
Using /etc/ansible/ansible.cfg as config file
setting up inventory plugins
Parsed /etc/ansible/hosts inventory source with ini plugin
 [WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

Loading callback plugin default of type stdout, v2.0 from /usr/lib/python2.7/site-packages/ansible/plugins/callback/default.pyc

PLAYBOOK: test-playbook.yml *************************************************************************************************************************************************************
1 plays in test-playbook.yml

PLAY [openvswitch: run update_port] *****************************************************************************************************************************************************

TASK [Gathering Facts] ******************************************************************************************************************************************************************
task path: /home/vagrant/networking-ansible/test-playbook.yml:2
Using module file /usr/lib/python2.7/site-packages/ansible/modules/system/setup.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: vagrant
<127.0.0.1> EXEC /bin/sh -c 'echo ~vagrant && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315 `" && echo ansible-tmp-1530280901.98-145822743403315="` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315 `" ) && sleep 0'
<127.0.0.1> PUT /home/vagrant/.ansible/tmp/ansible-local-29171pVygKZ/tmpFnjIYb TO /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315/setup.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315/ /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315/setup.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python2 /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315/setup.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /home/vagrant/.ansible/tmp/ansible-tmp-1530280901.98-145822743403315/ > /dev/null 2>&1 && sleep 0'
ok: [localhost]
META: ran handlers

TASK [openvswitch_db] *******************************************************************************************************************************************************************
task path: /home/vagrant/networking-ansible/test-playbook.yml:5
Using module file /usr/lib/python2.7/site-packages/ansible/modules/network/ovs/openvswitch_db.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: vagrant
<127.0.0.1> EXEC /bin/sh -c 'echo ~vagrant && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014 `" && echo ansible-tmp-1530280902.48-128404859669014="` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014 `" ) && sleep 0'
<127.0.0.1> PUT /home/vagrant/.ansible/tmp/ansible-local-29171pVygKZ/tmpQBJLuq TO /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014/openvswitch_db.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014/ /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014/openvswitch_db.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'sudo -H -S -n -u root /bin/sh -c '"'"'echo BECOME-SUCCESS-rnyzkgtjykoxtnypsistbvemltyxwkpb; /usr/bin/python2 /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014/openvswitch_db.py'"'"' && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /home/vagrant/.ansible/tmp/ansible-tmp-1530280902.48-128404859669014/ > /dev/null 2>&1 && sleep 0'
fatal: [localhost]: FAILED! => {
    "changed": false,
    "invocation": {
        "module_args": {
            "col": "tag",
            "record": "port0",
            "state": "present",
            "table": "Port",
            "timeout": 5,
            "value": "1"
        }
    },
    "msg": "missing required arguments: key"
}
	to retry, use: --limit @/home/vagrant/networking-ansible/test-playbook.retry

PLAY RECAP ******************************************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=1

After:

ansible-playbook 2.5.5
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/vagrant/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible-playbook
  python version = 2.7.5 (default, Apr 11 2018, 07:36:10) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]
Using /etc/ansible/ansible.cfg as config file
setting up inventory plugins
Parsed /etc/ansible/hosts inventory source with ini plugin
 [WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

Loading callback plugin default of type stdout, v2.0 from /usr/lib/python2.7/site-packages/ansible/plugins/callback/default.pyc

PLAYBOOK: test-playbook.yml *************************************************************************************************************************************************************
1 plays in test-playbook.yml

PLAY [openvswitch: run update_port] *****************************************************************************************************************************************************

TASK [Gathering Facts] ******************************************************************************************************************************************************************
task path: /home/vagrant/networking-ansible/test-playbook.yml:2
Using module file /usr/lib/python2.7/site-packages/ansible/modules/system/setup.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: vagrant
<127.0.0.1> EXEC /bin/sh -c 'echo ~vagrant && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697 `" && echo ansible-tmp-1530281209.23-239244825372697="` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697 `" ) && sleep 0'
<127.0.0.1> PUT /home/vagrant/.ansible/tmp/ansible-local-299095rrz15/tmpIDVMKC TO /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697/setup.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697/ /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697/setup.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python2 /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697/setup.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.23-239244825372697/ > /dev/null 2>&1 && sleep 0'
ok: [localhost]
META: ran handlers

TASK [openvswitch_db] *******************************************************************************************************************************************************************
task path: /home/vagrant/networking-ansible/test-playbook.yml:5
Using module file /usr/lib/python2.7/site-packages/ansible/modules/network/ovs/openvswitch_db.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: vagrant
<127.0.0.1> EXEC /bin/sh -c 'echo ~vagrant && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363 `" && echo ansible-tmp-1530281209.76-49723031212363="` echo /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363 `" ) && sleep 0'
<127.0.0.1> PUT /home/vagrant/.ansible/tmp/ansible-local-299095rrz15/tmpJBwk9c TO /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363/openvswitch_db.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363/ /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363/openvswitch_db.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'sudo -H -S -n -u root /bin/sh -c '"'"'echo BECOME-SUCCESS-kninhinyephefmmxeszabvdjfkjdplwv; /usr/bin/python2 /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363/openvswitch_db.py'"'"' && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /home/vagrant/.ansible/tmp/ansible-tmp-1530281209.76-49723031212363/ > /dev/null 2>&1 && sleep 0'
changed: [localhost] => {
    "changed": true,
    "commands": [
        "/usr/bin/ovs-vsctl -t 5 set Port port0 tag=1"
    ],
    "invocation": {
        "module_args": {
            "col": "tag",
            "key": null,
            "ovs-vsctl": "/usr/bin/ovs-vsctl",
            "record": "port0",
            "state": "present",
            "table": "Port",
            "timeout": 5,
            "value": "1"
        }
    }
}
META: ran handlers
META: ran handlers

PLAY RECAP ******************************************************************************************************************************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0

ansibot · 2018-06-29T14:16:17Z

cc @Qalthos @ganeshrn @gdpak @gundalow @privateip @rcarrillocruz @trishnaguha
click here for bot help

gdpak · 2018-07-05T12:04:13Z

lib/ansible/modules/network/ovs/openvswitch_db.py

    col_value_to_dict = {}
-    if col_value and col_value != '{}':
+    if MAP_RE.match(col_value):


What is the need for this re again ? 'is_map' can be used to figure if column value is a map ?

That's to decide whether the column is a non-empty map. is_map doesn't take into account the content of the map

gdpak · 2018-07-05T12:05:25Z

lib/ansible/modules/network/ovs/openvswitch_db.py

+
+    # Map types require key argument
+    has_key = module.params['key'] is not None
+    is_map = col_value[0] == '{'


is_map should be done using global re 'MAP_RE' so we have one way to identify map in a column value.

gdpak · 2018-07-05T12:07:22Z

lib/ansible/modules/network/ovs/openvswitch_db.py

+    has_key = module.params['key'] is not None
+    is_map = col_value[0] == '{'
+    if is_map and not has_key:
+        module.fail_json(msg="missing required arguments: key")


Probably this error message can be more informative by saying missing required argument : key for map type of column

gdpak · 2018-07-05T12:33:33Z

lib/ansible/modules/network/ovs/openvswitch_db.py

@@ -102,8 +113,16 @@ def map_obj_to_commands(want, have, module):
            templatized_command = "%(ovs-vsctl)s -t %(timeout)s remove %(table)s %(record)s " \
                                  "%(col)s %(key)s=%(value)s"
            commands.append(templatized_command % module.params)
+        elif module.params['key'] is None:


This check is confusing. why not have simple 'else'. Is there a path where have.keys() do not have 'key' and module.params['key'] is not None ? If no then putting simple else will make code much readable .

Probably not, you're right.

There is a case when you try to delete a record that is non-existent. So we need to keep this condition.

ansibot · 2018-07-09T08:52:19Z

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/network/ovs/openvswitch_db.py:108:1: E302 expected 2 blank lines, found 1

click here for bot help

gdpak

Hi @cubeek Thanks for making changes . looks good to me. Please push these changes to this PR.

The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes ansible#42108

cubeek · 2018-07-09T11:44:24Z

I force-pushed to my branch, the changes should be available in this PR as far as I can tell. If I should not be doing force-push, please let me know and I can stack the changes.

The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes ansible#42108 (cherry picked from commit 26b0908)

The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes ansible#42108 (cherry picked from commit 26b0908) (cherry picked from commit 0109771)

The patch adds simple test where LLC information is passed to a Neutron logical port. This triggers Ansible mechanism driver that is configured to talk to local Open vSwitch database. The port on OVS side should be configured with VLAN tag that is taken from Neutron logical network segmentation ID. VLAN tag is validated using ovsdbapp querying Port OVS table. update_port method for openvswitch provider is added in order to be able to talk to OVS DB. NOTE: Current Ansible contains bug in openvswitch_db module, the PR is pending review at the time of pushing this patch at ansible/ansible#42110 This is not a real tempest test as it assumes all-in-one and checks OVS installed on the local machine from which tempest is run. This is because we're in time pressure and such test adds a big value and there is no better testing framework at this moment. Change-Id: Ic73065175746a10ebef1ec731f947ee1c0919e72

The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes #42108 (cherry picked from commit 26b0908)

* openvswitch_db: Split key-value pairs correctly (#33335) Map values can contain commas, e.g. - name: Configure OVN bridge mapping openvswitch_db: state: present table: open_vswitch record: . col: external_ids key: ovn-bridge-mappings value: '"vmnet-static:br-vmnet-st,vmnet-dynamic:br-vmnet-dyn"' Previous behaviour was splitting the value and raised an exception. (cherry picked from commit 3c53e2f) * openvswitch_db: Make 'key' parameter optional (#42110) The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes #42108 (cherry picked from commit 26b0908) (cherry picked from commit 0109771)

gundalow · 2018-10-27T09:54:01Z

Hi,
I'm randomly selecting some PRs to see how it was for you, with the aim of making it easier for future contributors. As a new contributor I'm particularly interested as you don't have any prior knowledge of the Ansible process.

Generally speaking, how was the process for you?
Did it make sense what to do to progress the PR?
Did you read any of the developer (dev_guide) or contributor documentation?
What in the documentation was useful or confusing, missing or could be improved
Did you know you could run ansible-test locally?
What areas would you be interested in contributing to?

ansible/community#353

cubeek · 2018-11-02T10:45:16Z

Hi,
Hi :)

I'm randomly selecting some PRs to see how it was for you, with the aim of making it easier for future contributors. As a new contributor I'm particularly interested as you don't have any prior knowledge of the Ansible process.

Generally speaking, how was the process for you?
It was pretty smooth, Ansible has a good contribution documentation with detailed steps on how to contribute. Was very interesting to see how the CI is done, pretty cool. The template for reporting the issue is also good.

Did it make sense what to do to progress the PR?
Yes, I got feedback quickly and it was clear what's expected from me.

Did you read any of the developer (dev_guide) or contributor documentation?
Yes.

What in the documentation was useful or confusing, missing or could be improved
I didn't find anything what would be unclear.

Did you know you could run ansible-test locally?
Yes, I found it by reading the coding guideline.

What areas would you be interested in contributing to?
I'm working closely with ansible networking modules these days.

ansible/community#353

gundalow · 2018-11-13T20:03:11Z

@cubeek Thank you for taking the time to provide this feedback, it truly is useful.

I'm glad to hear the Issue and PR templates are user friendly.

s-hertel removed the needs_triage Needs a first human triage before being processed. label Jun 29, 2018

trishnaguha assigned gdpak Jul 4, 2018

gdpak suggested changes Jul 5, 2018

View reviewed changes

cubeek force-pushed the issue/42108 branch from 20eccda to 8b377e0 Compare July 9, 2018 08:43

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 9, 2018

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 9, 2018

cubeek force-pushed the issue/42108 branch from 8b377e0 to 20eccda Compare July 9, 2018 08:55

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 9, 2018

gdpak approved these changes Jul 9, 2018

View reviewed changes

openvswitch_db: Make 'key' parameter optional

93fdb6c

The OVSDB schema consists of typed columns. The 'key' parameter is required only for columns with type of a 'map'. This patch makes 'key' an optional parameter to allow setting values for other column types like int. Fixes ansible#42108

cubeek force-pushed the issue/42108 branch from 20eccda to 93fdb6c Compare July 9, 2018 11:41

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 9, 2018

gdpak merged commit 26b0908 into ansible:devel Jul 9, 2018

cubeek mentioned this pull request Jul 9, 2018

openvswitch_db: Make 'key' parameter optional (#42110) #42517

Merged

cubeek deleted the issue/42108 branch July 10, 2018 17:37

cubeek mentioned this pull request Aug 1, 2018

openvswitch_db: Make 'key' parameter optional (#42110) #43552

Merged

gundalow mentioned this pull request Aug 9, 2018

openvswitch_db: ovs-vsctl: "True" is not a valid boolean (use "true" or "false") #43858

Closed

gundalow added the contrib_follow_up https://github.com/ansible/community/issues/353 label Oct 27, 2018

ansible locked and limited conversation to collaborators Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openvswitch_db: Make 'key' parameter optional #42110

openvswitch_db: Make 'key' parameter optional #42110

cubeek commented Jun 29, 2018

ansibot commented Jun 29, 2018

gdpak Jul 5, 2018

cubeek Jul 9, 2018

gdpak Jul 5, 2018

cubeek Jul 9, 2018

gdpak Jul 5, 2018

cubeek Jul 9, 2018

gdpak Jul 5, 2018

cubeek Jul 9, 2018

cubeek Jul 9, 2018

ansibot commented Jul 9, 2018

gdpak left a comment

cubeek commented Jul 9, 2018

gundalow commented Oct 27, 2018

cubeek commented Nov 2, 2018

gundalow commented Nov 13, 2018

openvswitch_db: Make 'key' parameter optional #42110

openvswitch_db: Make 'key' parameter optional #42110

Conversation

cubeek commented Jun 29, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jun 29, 2018

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

ansibot commented Jul 9, 2018

gdpak left a comment

Choose a reason for hiding this comment

cubeek commented Jul 9, 2018

gundalow commented Oct 27, 2018

cubeek commented Nov 2, 2018

gundalow commented Nov 13, 2018