public icon indicating copy to clipboard operation
public copied to clipboard

Change interface to queue mapping to leaf-ref

Open dplore opened this issue 2 years ago • 9 comments

Change Scope

  • This is a modeling change which clarifies that configuration of a queue on an interface is intended to reference an already existing queue in the /qos tree.
  • This change is not backwards compatible as a leaf type is changed.

Fixes #871

dplore avatar Jun 30 '23 19:06 dplore

No major YANG version changes in commit fb3c32b9f0a03cdaf65e602558a4ffb70c7543f3

OpenConfigBot avatar Jun 30 '23 19:06 OpenConfigBot

Per the comment that was in the description of this leaf, at some point we had considered having the ability to support queues that are implicitly created by the system -- which themselves would not necessarily be referenced in the configuration. Have we concluded that this isn't something that is needed?

In YANG 1.1, there is the ability to support a union of leafref and string, but we would need to check tooling compatibility if this is something that would be desirable (and in general cases would just accept any content because any invalid queue name would be a valid string).

robshakir avatar Jul 06 '23 18:07 robshakir

Per the comment that was in the description of this leaf, at some point we had considered having the ability to support queues that are implicitly created by the system -- which themselves would not necessarily be referenced in the configuration. Have we concluded that this isn't something that is needed?

In YANG 1.1, there is the ability to support a union of leafref and string, but we would need to check tooling compatibility if this is something that would be desirable (and in general cases would just accept any content because any invalid queue name would be a valid string).

I do agree that it is necessary to support queues which are implicitly created by the system. But IMO these should be exposed and addressable in OC. Is there some reason it is infeasible to expose system created queues?

Thinking about this a bit more, assuming these system created queues cannot be modified or deleted, perhaps the leaf-ref should be to queue/state/name instead of queue/config/name ?

dplore avatar Jul 06 '23 19:07 dplore

You cannot have a leafref from a config true leaf to a config false one -- and equally, this isn't a great idea because it means that to validate leafrefs a configuration producer needs to have access to state data.

I'm OK with saying that you expect a system that has its own defined queues to be exported in the /qos/queues/queue list -- I just wanted to highlight that this is the implication and check that we'd considered any impact that this might have.

robshakir avatar Jul 07 '23 17:07 robshakir

You cannot have a leafref from a config true leaf to a config false one -- and equally, this isn't a great idea because it means that to validate leafrefs a configuration producer needs to have access to state data.

I'm OK with saying that you expect a system that has its own defined queues to be exported in the /qos/queues/queue list -- I just wanted to highlight that this is the implication and check that we'd considered any impact that this might have.

Thanks for the review @robshakir. Now I understand why the TODO item has not been addressed so far.

So we could just leave this PR as is but also update the description in /qos/queues to assert that system defined queues are required to be present and are not expected to support a change in the name or be deleted.

As a though experiment, I am thinking about ways we can make this scenario more explicitly modeled vs. in descriptions.

Please let me know what you think: One idea that comes to mind is we could create a new leaf list for system created qos queues. This would help make the expectation of their use very explicit. We can make them state only. We'd end up having two leafrefs anywhere we want to refer to qos queues, one for configurable queues and one for system defined (static) queues. Two lists seems not very elegant (bad), but it is explicit (good).

Another idea could be to add a (boolean) leaf to /qos/interfaces/interface/input/queues/queue/state container to assert if the queue is system defined and therefore it's not mutable. This would open the option to use when statements if needed.

dplore avatar Jul 13 '23 21:07 dplore

Please let me know what you think: One idea that comes to mind is we could create a new leaf list for system created qos queues. This would help make the expectation of their use very explicit. We can make them state only. We'd end up having two leafrefs anywhere we want to refer to qos queues, one for configurable queues and one for system defined (static) queues. Two lists seems not very elegant (bad), but it is explicit (good).

This sounds complex to me -- if it's just a leaf-list of the names of these queues, I'm not sure that it really gives you that much (since you don't know anything about those queues). Creating another list means that if anything about those system defined queues actually is configurable, but has a default value, then they might end up in two different places,wchich causes more ambiguity.

Another idea could be to add a (boolean) leaf to /qos/interfaces/interface/input/queues/queue/state container to assert if the queue is system defined and therefore it's not mutable. This would open the option to use when statements if needed.

I'm not sure that "system-defined" and "immutable" are tightly coupled.

I think it's worth taking a step back and asking what problem we are trying to solve here. Are we trying to ensure that we are only referencing queues that actually exist on the system in configuration? Are we trying to ensure that there is a way to be able to determine that there are default queues? Essentially, let's think about what we are trying to make explicit and why and then from there consider what the modelling approaches to achieve that is.

robshakir avatar Jul 13 '23 22:07 robshakir

I think it's worth taking a step back and asking what problem we are trying to solve here. Are we trying to ensure that we are only referencing queues that actually exist on the system in configuration? Are we trying to ensure that there is a way to be able to determine that there are default queues? Essentially, let's think about what we are trying to make explicit and why and then from there consider what the modelling approaches to achieve that is.

The origin of this PR is to respond to #871. I think the desire is in fact to:

  1. ensure we are only referencing queues that exist
  2. ensure that system defined queues are exposed

@cosmos2k17 as the creator of #871 would you like to respond to robshakir's comment?

dplore avatar Jul 18 '23 21:07 dplore

I think it's worth taking a step back and asking what problem we are trying to solve here. Are we trying to ensure that we are only referencing queues that actually exist on the system in configuration? Are we trying to ensure that there is a way to be able to determine that there are default queues? Essentially, let's think about what we are trying to make explicit and why and then from there consider what the modelling approaches to achieve that is.

The origin of this PR is to respond to #871. I think the desire is in fact to:

1. ensure we are only referencing queues that exist

2. ensure that system defined queues are exposed

@cosmos2k17 as the creator of #871 would you like to respond to robshakir's comment?

yes, we should be referring to queues that exist which are system defined.

cosmos2k17 avatar Jul 26 '23 19:07 cosmos2k17

If this is the case, then we need implementations to create an entry in the /qos/queues/queue list for all system-defined queues. This is akin to how all interfaces are created within /interfaces/interface regardless of whether they are configured.

I think this needs a functional test in featureprofiles alongside this change.

robshakir avatar Aug 28 '23 19:08 robshakir