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

Handle duplication during parser second pass (LP: #2007682) #329

Merged
merged 5 commits into from
Mar 2, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Feb 28, 2023

Description

When the parser needs a second pass, some configuration is being added again to their respective lists. We handle these cases already in other places either by checking if the values are present in their data structures or checking if the number of nodes in the YAML subtree matches the number of values in the lists/hashes. The second approach doesn't work well though when the same interface is defined in another file with extra configuration (example LP: #2003061)

This PR addresses 5 cases of duplication:

  1. DNS domain search strings (LP: #2007682)
  2. DNS servers IPs (LP: #2007682)
  3. IP route rules
  4. OpenvSwitch protocols
  5. OpenvSwitch controllers

The example below will generate duplicated data for the routing-policy, nameservers addresses and search domains:

network:
  bonds:
    aggi:
      routing-policy:
        - table: 1
          from: 1.2.3.4
      nameservers:
        addresses:
        - 8.8.8.8
        search:
        - example.com
      interfaces:
      - ens3f0
  ethernets:
    ens3f0:
      dhcp4: true

Result:

[Match]
Name=aggi

[Network]
LinkLocalAddressing=ipv6
DNS=8.8.8.8
DNS=8.8.8.8
Domains=example.com example.com
ConfigureWithoutCarrier=yes

[RoutingPolicyRule]
From=1.2.3.4
Table=1

[RoutingPolicyRule]
From=1.2.3.4
Table=1

The example below will generate ovs commands with duplicated protocols and controller addresses:

network:
  bridges:
    br123:
      openvswitch:
        protocols:
          - OpenFlow10
          - OpenFlow11
          - OpenFlow12
        controller:
          addresses:
            - tcp:127.0.0.1:6653
            - tcp:127.0.0.2:6653
      interfaces:
        - nic1
  ethernets:
    nic1: {}

Result:

...
ExecStart=/usr/bin/ovs-vsctl set Bridge br123 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow10,OpenFlow11,OpenFlow12
ExecStart=/usr/bin/ovs-vsctl set Bridge br123 external-ids:netplan/protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow10,OpenFlow11,OpenFlow12
ExecStart=/usr/bin/ovs-vsctl set-controller br123 tcp:127.0.0.1:6653 tcp:127.0.0.2:6653 tcp:127.0.0.1:6653 tcp:127.0.0.2:6653
ExecStart=/usr/bin/ovs-vsctl set Bridge br123 external-ids:netplan/global/set-controller=tcp:127.0.0.1:6653,tcp:127.0.0.2:6653,tcp:127.0.0.1:6653,tcp:127.0.0.2:6653

This behavior is also visible with netplan get:

$ netplan get --root-dir /workspace/netplan-issues/fakeroot_duplicates
network:
  version: 2
  ethernets:
    ens3f0:
      dhcp4: true
    nic1: {}
  bridges:
    br123:
      interfaces:
      - nic1
      openvswitch:
        protocols:
        - OpenFlow10
        - OpenFlow11
        - OpenFlow12
        - OpenFlow10
        - OpenFlow11
        - OpenFlow12
        controller:
          addresses:
          - "tcp:127.0.0.1:6653"
          - "tcp:127.0.0.2:6653"
          - "tcp:127.0.0.1:6653"
          - "tcp:127.0.0.2:6653"
  bonds:
    aggi:
      nameservers:
        addresses:
        - 8.8.8.8
        - 8.8.8.8
        search:
        - example.com
        - example.com
      interfaces:
      - ens3f0
      routing-policy:
      - table: 1
        from: "1.2.3.4"
      - table: 1
        from: "1.2.3.4"

This is the new behavior:

$ LD_LIBRARY_PATH=build/src/  PYTHONPATH=. ./src/netplan.script get --root-dir /workspace/netplan-issues/fakeroot_duplicates
network:
  version: 2
  ethernets:
    ens3f0:
      dhcp4: true
    nic1: {}
  bridges:
    br123:
      interfaces:
      - nic1
      openvswitch:
        protocols:
        - OpenFlow10
        - OpenFlow11
        - OpenFlow12
        controller:
          addresses:
          - "tcp:127.0.0.1:6653"
          - "tcp:127.0.0.2:6653"
  bonds:
    aggi:
      nameservers:
        addresses:
        - 8.8.8.8
        search:
        - example.com
      interfaces:
      - ens3f0
      routing-policy:
      - table: 1
        from: "1.2.3.4"

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. LP#2007682

@daniloegea
Copy link
Collaborator Author

Well, it looks like the memory leak checker works! 😅

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.

Nice work! This is mostly looking good to me. One remark and some nitpicks inline.

If you could split & squash your final "memoryleak fixes" commit into the corresponding previous commits, and do the same for the fixes for my inline remarks, we could make this PR a rebase & merge, instead of a squash merge, as I think the commits have a really nice structure with one logical change per commit.

src/util.c Outdated Show resolved Hide resolved
src/parse.c Outdated Show resolved Hide resolved
src/parse.c Show resolved Hide resolved
src/parse.c Outdated Show resolved Hide resolved
@slyon slyon changed the title Handle duplication during parser second pass (LP: #2007682) Handle duplication during parser second pass (LP: #2007682, LP: #2007682) Mar 1, 2023
@slyon slyon changed the title Handle duplication during parser second pass (LP: #2007682, LP: #2007682) Handle duplication during parser second pass (LP: #2007682) Mar 1, 2023
It will prevent that routing rules with exactly the same parameters are
added to the list.
When the parser needs a second pass, DNS IP addresses and search domains
are inserted a second time in their respective lists. Avoid it by
checking if they are already in the list.
Now they are deduplicated by the parser.
The list of protocols and controllers will have duplicate items when the
parser needs a second pass.
@daniloegea daniloegea requested a review from slyon March 1, 2023 18:11
@slyon slyon merged commit ef29604 into canonical:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants