[vlan multiplexing] add vlan tag for vlan supported kernel mechanism

[pperiyasamy]

Signed-off-by: Periyasamy Palanisamy [email protected]

Description

Issue link

How Has This Been Tested?

• Added unit testing to cover
• Tested manually
• Tested by integration testing
• Have not tested

Types of changes

• Bug fix
• New functionallity
• Documentation
• Refactoring
• CI

[pperiyasamy]

/cc @JanScheurich

[JanScheurich]

Does that mean the VLAN-multiplexing will only work with local kernel mechanism? That would exclude VLAN multiplexing on SRIOV vfio devices in NSE pods. For Mellanox SmartVFs it's OK as they are kernel mechanisms as well as DPDK devices.

[Bolodya1997]

@pperiyasamy
Please add some WithVLAN option and use VLAN tags only if it is selected. I suppose that there are cases when we don't need VLAN tags for kernel mechanism.

[edwarnicke]

@pperiyasamy Would it possibly make more sense to have a separate 'vlan' chain element that handles these details? (similar to how vni is handled) This would give nice modularity, and allow easy use of vlan multiplexing across different mechanisms.

[pperiyasamy]

@edwarnicke @JanScheurich where do you want 'vlan' chain element to be placed ? currently only kernel mechanism has vlan parameter support, so can we place it under sdk/pkg/networkservice/common/mechanisms/kernel/vlan for now ?

[edwarnicke]

@pperiyasamy Sounds about right :)

[pperiyasamy]

@pperiyasamy Sounds about right :)

done. please review it now.

[edwarnicke]

@pperiyasamy This looks good.

@JanScheurich A brief comment here on 'working with other non kernel mechanisms'. As written, the implementation @pperiyasamy has provided only works for kernel mechanism... but it has the right architectural whitespace to be easily extended to any Vlan supporting mechanism.

I'll talk through how briefly, and you can all decide when you'd like to make those extensions :)

Right now the constants 'SupportsVLAN' and 'VLAN' are only defined in api/networkservice/mechanisms/kernel/constants.go. As you can see just above there, we've in the past defined some constants by reference to 'common' because those constants get used in multiple mechanisms.

vlan multiplexing is seen as a cross-cutting concern... you could treat it as such by introducing pkg/api/networkservice/mechanisms/vlanmultiplexing/constants.go (to act as a home for the constants, similar to what commons does) and pkg/api/networkservice/mechanisms/vlanmultiplexing/helpers.go to introduce its own ToMechanism and accessors (which could be reused in the kernel mechanism). This way other mechanisms could do likewise, and the vlan chain element could support whatever mechanism provides SupportsVlan.

Thoughts?

[JanScheurich]

Right now the constants 'SupportsVLAN' and 'VLAN' are only defined in api/networkservice/mechanisms/kernel/constants.go. As you can see just above there, we've in the past defined some constants by reference to 'common' because those constants get used in multiple mechanisms.

vlan multiplexing is seen as a cross-cutting concern... you could treat it as such by introducing pkg/api/networkservice/mechanisms/vlanmultiplexing/constants.go (to act as a home for the constants, similar to what commons does) and pkg/api/networkservice/mechanisms/vlanmultiplexing/helpers.go to introduce its own ToMechanism and accessors (which could be reused in the kernel mechanism). This way other mechanisms could do likewise, and the vlan chain element could support whatever mechanism provides SupportsVlan.

Thoughts?

Makes sense. The current implementation in the kernel mechanism is sufficient for our OVS forwarder + SmartVF use case. We can leave the generalization to a future contributor.

[edwarnicke]

@JanScheurich

Makes sense. The current implementation in the kernel mechanism is sufficient for our OVS forwarder + SmartVF use case. We can leave the generalization to a future contributor.

This is what I expected :) That's why I merged this PR and just left my comment in case I misread the situation :)

[edwarnicke]

The good news is its actually pretty each and non-impactful to generalize from @pperiyasamy 's implementation here :)

[pperiyasamy]

@Bolodya1997 @edwarnicke @JanScheurich Thanks all for your inputs !