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

lldp: Simplify rust structs and serde #2479

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

qinqon
Copy link
Member

@qinqon qinqon commented Dec 1, 2023

Currently the serialization/deserialization of LLDP attributes is done
using serialization and deserialization functions. This change move that
to a more declarative way using some structs.

This effort make easier to parse and understand the rust nmstate AST
tree of the interface.

@kubevirt-bot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/nmstate-nmstate-2479
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@qinqon qinqon force-pushed the simplify-lldp-serde branch 13 times, most recently from f4a3008 to 8983852 Compare December 4, 2023 12:34
@qinqon qinqon marked this pull request as ready for review December 4, 2023 12:53
@qinqon
Copy link
Member Author

qinqon commented Dec 4, 2023

@cathay4t @ffmancera this is ready, the idea os to remove the serializer functions to make it more declarative and easier to the future golang generator to visit the AST.

@qinqon qinqon marked this pull request as draft December 5, 2023 05:37
@qinqon qinqon mentioned this pull request Dec 5, 2023
10 tasks
@qinqon qinqon marked this pull request as ready for review December 5, 2023 15:31
@qinqon
Copy link
Member Author

qinqon commented Dec 5, 2023

@cathay4t Maybe it make more sense to keep common attributes at a base struct ? like type, subtype and oui, it may also help with the ast parsin and golang generator.

@cathay4t
Copy link
Member

cathay4t commented Dec 7, 2023

Since we are open to change the schema of LLDP query, I would like to propose:

interfaces:
- name: eth2
  type: ethernet
  lldp:
    enabled: true
    neighbors:
    - system-name: Summit300-48
      system-description: Summit300-48 - Version 7.4e.1 (Build 5)
      system-capabilities:
        - MAC Bridge component
        - Router
      mac-address: 00:01:30:F9:AD:A0
      interface-name: 1/1
      vlans:
        - name: v2-0488-03-0505
          id: 488
      mac-phy-conf:
        autogeg: true
        # TODO: https://www.iana.org/assignments/ianamau-mib/ianamau-mib
        operational-mau-type: 100BASE-TX full duplex mode
        # TODO: Convert this to human string base on IEEE 802.3ap
        pmd-autoneg-cap: 27648
      port-vlan-ids:
        - 0
      management-address:
        - address: 00:01:30:F9:AD:A0
          interface-number: 1001
      max-frame-size: 1522

This means we are fully parsing LLDP into human friendly data. No more raw integer data in our schema.

@qinqon
Copy link
Member Author

qinqon commented Dec 11, 2023

Since we are open to change the schema of LLDP query, I would like to propose:

interfaces:
- name: eth2
  type: ethernet
  lldp:
    enabled: true
    neighbors:
    - system-name: Summit300-48
      system-description: Summit300-48 - Version 7.4e.1 (Build 5)
      system-capabilities:
        - MAC Bridge component
        - Router
      mac-address: 00:01:30:F9:AD:A0
      interface-name: 1/1
      vlans:
        - name: v2-0488-03-0505
          id: 488
      mac-phy-conf:
        autogeg: true
        # TODO: https://www.iana.org/assignments/ianamau-mib/ianamau-mib
        operational-mau-type: 100BASE-TX full duplex mode
        # TODO: Convert this to human string base on IEEE 802.3ap
        pmd-autoneg-cap: 27648
      port-vlan-ids:
        - 0
      management-address:
        - address: 00:01:30:F9:AD:A0
          interface-number: 1001
      max-frame-size: 1522

This means we are fully parsing LLDP into human friendly data. No more raw integer data in our schema.

That's way better we don't need the enum either is a plain it can be struct with fields instead of a [][] of enum, that's way simpler.

@qinqon
Copy link
Member Author

qinqon commented Dec 11, 2023

Since we are open to change the schema of LLDP query, I would like to propose:

interfaces:
- name: eth2
  type: ethernet
  lldp:
    enabled: true
    neighbors:
    - system-name: Summit300-48
      system-description: Summit300-48 - Version 7.4e.1 (Build 5)
      system-capabilities:
        - MAC Bridge component
        - Router
      mac-address: 00:01:30:F9:AD:A0
      interface-name: 1/1
      vlans:
        - name: v2-0488-03-0505
          id: 488
      mac-phy-conf:
        autogeg: true
        # TODO: https://www.iana.org/assignments/ianamau-mib/ianamau-mib
        operational-mau-type: 100BASE-TX full duplex mode
        # TODO: Convert this to human string base on IEEE 802.3ap
        pmd-autoneg-cap: 27648
      port-vlan-ids:
        - 0
      management-address:
        - address: 00:01:30:F9:AD:A0
          interface-number: 1001
      max-frame-size: 1522

This means we are fully parsing LLDP into human friendly data. No more raw integer data in our schema.

That's way better we don't need the enum either is a plain it can be struct with fields instead of a [][] of enum, that's way simpler.

Is it worth it to apart from human readable forman have those as number, they are faster to compare and process in case of automation.

@qinqon
Copy link
Member Author

qinqon commented Dec 11, 2023

@cathay4t @ffmancera I will put a different PR with proposed schema, we have more or less an ACK from k-nmstate users.

@qinqon
Copy link
Member Author

qinqon commented Dec 13, 2023

@cathay4t @ffmancera at the end looks like we have big customers already consuming lldp neighbor as it is, so we cannot break it, let's just keep it and review this PR.

@qinqon qinqon force-pushed the simplify-lldp-serde branch 2 times, most recently from acd6da5 to 1cf8d80 Compare December 14, 2023 14:07
Copy link
Member

@ffmancera ffmancera left a comment

Choose a reason for hiding this comment

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

LGTM! Just a typo on the commit message Currently the serialization/deserialization of LLDP attributes is done are serialization and deserialization functions.

@qinqon
Copy link
Member Author

qinqon commented Dec 15, 2023

LGTM! Just a typo on the commit message Currently the serialization/deserialization of LLDP attributes is done are serialization and deserialization functions.

@ffmancera, fixed comments, can you review ?

@ffmancera ffmancera enabled auto-merge (rebase) December 15, 2023 11:50
Currently the serialization/deserialization of LLDP attributes is done
using serialization and deserialization functions. This change move that
to a more declarative way using some structs.

This effort make easier to parse and understand the rust nmstate AST
tree of the interface.

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
@ffmancera ffmancera merged commit ac1eb9d into nmstate:base Dec 18, 2023
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants