Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Aug 30, 2024

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.

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 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
@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
Copy link
Collaborator

@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
Collaborator Author

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

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

@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

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
Labels
stable Might be merged in a stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants