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

Routed Subinterface enhancements #833

Merged
merged 6 commits into from
Nov 4, 2021
Merged

Conversation

preetham-singh
Copy link
Contributor

@preetham-singh preetham-singh commented Aug 7, 2021

* Per sub port interface admin status config
- Kernel subinterface netdev admin UP can be performed only if parent interface netdev is admin UP.
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Aug 30, 2021

Choose a reason for hiding this comment

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

Do we have any db-migrator section to describe for long subinterface name to short interface name conversion in config-db across releases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have to handle subinterface name conversion in db_migrator script during upgrade & downgrade.
Updated in next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to support both short name and long name format for creating subinterfaces. With this no need to handle DB conversion while performing upgrade.

MTU inherited from parent physical port or port channel.
If subinterface MTU is configured, MTU on subinterface will be configured with:
- If Subinterface MTU <= parent port MTU, configured subinterface MTU will be applied.
- If Subinterface MTU > parent port MTU, parent port MTU will be applied.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dont we throw an error in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in review meeting, user can change MTU on parent interface. Restricting this update depending on MTU of subinterfaces will be hard for user since there could be 100s of subinterfaces under the parent interface.

### 2.1.1 config_db.json
### 2.1.1 Naming Convention for sub-interfaces:

Since Kernel has netdevice name length restriction to 15, port channel sub-interfaces(Physical interfaces as well in case interface number > 99) cannot follow the same nomenclature as physical sub-interfaces. Hence Long name to short name conversion needs to be performed for the subinterfaces.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's 99 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If parent physical interface has interface number which exceeds 2 digits, subinterface name length will exceed 15 characters with existing subinterface naming convention. This results in failure of subinterface netdev creation in kernel.

To be more specific, this subinterface netdev name length issue is not specific to PortChannel subinterfaces, but also applicable to physical subinterfaces.


mtu of a sub port interface is inherited from its parent physical port or port channel, and is not configurable in the current design.
In Click CLI, user will be able to configure the vlan id associated with the sub-interface. If vlan_id is not provided, sub-interface ID will be used as vlan id.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, we should mandate vlan_id configuration even from Click CLI and we should not assume sub-interface id as vlan-id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in next commit where vlan id is mandatory for short name format subinterfaces. But for existing long name format subinterfaces, there is no vlan id hence its not applicable.

@prsunny
Copy link
Contributor

prsunny commented Aug 31, 2021

@wendani for review

Coexistence with existing long name format subinterfaces
@prsunny
Copy link
Contributor

prsunny commented Sep 18, 2021

Please capture all associated PRs in description for ease of tracking.

@preetham-singh
Copy link
Contributor Author

Please capture all associated PRs in description for ease of tracking.

Updated Description with code PRs.

@preetham-singh
Copy link
Contributor Author

Please capture all associated PRs in description for ease of tracking.

Updated Description with code PRs.

Done.

under swss library instead of swss-common library.
@prsunny
Copy link
Contributor

prsunny commented Nov 16, 2021

As discussed in the community meeting today(11/16), please also plan to add sonic-mgmt tests for this feature. This is required for all features that is to be added to release notes.

@liat-grozovik
Copy link
Collaborator

@prsunny and @preetham-singh which sonic-mgmt test covers this enhancement?

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.

4 participants