Skip to content

ovs: quote external-ids and other-config values (LP: #2070318)#512

Merged
daniloegea merged 1 commit intocanonical:mainfrom
daniloegea:ovs_fixes
Sep 4, 2024
Merged

ovs: quote external-ids and other-config values (LP: #2070318)#512
daniloegea merged 1 commit intocanonical:mainfrom
daniloegea:ovs_fixes

Conversation

@daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Aug 30, 2024 •
edited by slyon
Loading

For complex values, ovs-vsctl requires that they are quoted or it will error out. LP: #2070318

Interestingly, it seems to work from systemd units. But I added quotes there too.

I added a new test case to the integration test test_settings_tag_cleanup that will fail without quotes. It's based on the example provided in the bug report.

test_settings_tag_cleanup (__main__.TestOVS.test_settings_tag_cleanup) ... eth42 eth43 ovs0 ovs1
** (process:6306): WARNING **: 11:04:05.201: Permissions for /etc/netplan/01-main.yaml are too open. Netplan configuration should NOT be accessible by others.
ovs-vsctl: card-serial-number=MT42424242N8,enable-chassis-as-gw: unexpected "=" parsing set of 1 or more strings
Traceback (most recent call last):
  File "/usr/sbin/netplan", line 23, in <module>
    netplan.main()
...
  File "/usr/share/netplan/netplan_cli/cli/ovs.py", line 64, in _del_dict
    subprocess.check_call([OPENVSWITCH_OVS_VSCTL, 'remove', type, iface, column, key, _escape_colon(value)])
  File "/usr/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/ovs-vsctl', 'remove', 'Bridge', 'ovs0', 'external-ids', 'ovn-cms-options', 'card-serial-number=MT42424242N8,enable-chassis-as-gw']' returned non-zero exit status 1.
ERROR
test_vlan_maas (__main__.TestOVS.test_vlan_maas) ... eth42 ovs0 eth42.21 ok
test_zzz_ovs_debugging (__main__.TestOVS.test_zzz_ovs_debugging)
Display OVS logs of the previous tests ... skipped 'For debugging only'

======================================================================
ERROR: test_settings_tag_cleanup (__main__.TestOVS.test_settings_tag_cleanup)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.CeX2HX/tree/tests/integration/ovs.py", line 541, in test_settings_tag_cleanup
    subprocess.check_call(['netplan', 'apply', '--only-ovs-cleanup'])
  File "/usr/lib/python3.12/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['netplan', 'apply', '--only-ovs-cleanup']' returned non-zero exit status 1.

----------------------------------------------------------------------
Ran 13 tests in 221.779s

While here, add some debugging information so we can see the ovs-vsctl command executed by "netplan apply" with --debug.

I created a PPA for Noble with this patch: https://launchpad.net/~danilogondolfo/+archive/ubuntu/netplan.io/

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@daniloegea
ovs: quote external-ids and other-config values
824380e 
For complex values, ovs-vsctl requires that they are quoted or it will
error out. LP: #2070318

While here, add some debugging information so we can see the ovs-vsctl
command executed by "netplan apply" with --debug.
daniloegea
daniloegea commented Sep 2, 2024
View reviewed changes
@daniloegea daniloegea changed the title [WIP] ovs: quote external-ids and other-config values ovs: quote external-ids and other-config values Sep 2, 2024
@daniloegea daniloegea marked this pull request as ready for review September 2, 2024 13:13
@daniloegea daniloegea requested a review from slyon September 2, 2024 13:13
@slyon slyon changed the title ovs: quote external-ids and other-config values ovs: quote external-ids and other-config values (LP: #2070318) Sep 4, 2024
slyon
slyon approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a straight forward fix, LGTM!

Thank you very much for integrating the regression case form LP#2070318 into our autopkgtests. That makes me feel much more confident about the fix!

I also see we still have test values with colons, covering the removal of the _escape_colon method, e.g.:

ExecStart=/usr/bin/ovs-vsctl set Bridge br0 external-ids:netplan/global/set-controller="ptcp:,ptcp:1337,ptcp:1337:[fe80::1234%eth0],pssl:1337:[fe80::1],ssl:10.10.10.1,tcp:127.0.0.1:1337,tcp:[fe80::1234%eth0],tcp:[fe80::1]:1337,unix:/some/path,punix:other/path"

I guess we can give @dshcherb some time to confirm the fix, using your PPA. But IMO we don't necessarily need to block on his feedback, as we have a clear reproducer as part of the integration tests.

@daniloegea
Copy link
Contributor Author

daniloegea commented Sep 4, 2024

Thanks, Lukas. I'll go ahead and merge it then.

@daniloegea daniloegea merged commit 4d56591 into canonical:main Sep 4, 2024
@slyon slyon added the stable Might be merged in a stable branch label Sep 10, 2024
@dshcherb
Copy link

dshcherb commented Sep 13, 2024

@slyon @daniloegea Apologies, didn't notice a notification.

I'll give it a try - should be easy to check in a VM.

Thanks!

@dshcherb
Copy link

dshcherb commented Sep 13, 2024

It works correctly with the build in the PPA: https://bugs.launchpad.net/netplan/+bug/2070318/comments/2

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@slyon slyon slyon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stable Might be merged in a stable branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@daniloegea @dshcherb @slyon