Skip to content

Add stage ids for DynPipeline and ability to retrieve stages with concrete type #320

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

Merged
merged 4 commits into from
Mar 10, 2025

Conversation

mvachhar
Copy link
Contributor

No description provided.

@mvachhar mvachhar requested a review from a team as a code owner February 28, 2025 17:01
@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch from 3c181d6 to 0e6bc46 Compare February 28, 2025 17:04
@mvachhar mvachhar added the dont-merge Do not merge this Pull Request label Feb 28, 2025
@mvachhar
Copy link
Contributor Author

I'm going to try to use the Borrow trait to fix the FIXME in the last commit.

Also, I forgot to add docs.

Copy link
Contributor

@Fredi-raspall Fredi-raspall left a comment

Choose a reason for hiding this comment

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

Looks good to me @mvachhar .
I'd keep a human-readable name for the NF nevertheless (#290) , even if we don't use it for lookups.

@mvachhar
Copy link
Contributor Author

Looks good to me @mvachhar . I'd keep a human-readable name for the NF nevertheless (#290) , even if we don't use it for lookups.

I'm hoping to figure out how to get the compiler to tell me the concrete type name so that I can stash that separately, so there is no need for additional info. Haven't gotten around to that yet though.

@Fredi-raspall
Copy link
Contributor

Looks good to me @mvachhar . I'd keep a human-readable name for the NF nevertheless (#290) , even if we don't use it for lookups.

I'm hoping to figure out how to get the compiler to tell me the concrete type name so that I can stash that separately, so there is no need for additional info. Haven't gotten around to that yet though.

Getting the name of a type is not difficult (https://doc.rust-lang.org/beta/std/any/fn.type_name.html , though you cannot rely on it as an id). I did not mean the name of the type but the name of the instance. The NF type may be informative, but you may have several NFs of the same type in the same pipeline (FWs, Nat, Routing), so IMO, what is most informative is the instance name. The type name should be easy to get once you downcast, right ?

@mvachhar
Copy link
Contributor Author

Getting the name of a type is not difficult (https://doc.rust-lang.org/beta/std/any/fn.type_name.html , though you cannot rely on it as an id). I did not mean the name of the type but the name of the instance. The NF type may be informative, but you may have several NFs of the same type in the same pipeline (FWs, Nat, Routing), so IMO, what is most informative is the instance name. The type name should be easy to get once you downcast, right ?

I don't think there is any reason to force the NetworkFunction to have an instance name, that is why I went with the id when it is put in the pipeline. The Pipeline owns the stage and you find it by id. It is always possible to implement stages that have instance names, but I'm not sure if it should be required.

@Fredi-raspall
Copy link
Contributor

Getting the name of a type is not difficult (https://doc.rust-lang.org/beta/std/any/fn.type_name.html , though you cannot rely on it as an id). I did not mean the name of the type but the name of the instance. The NF type may be informative, but you may have several NFs of the same type in the same pipeline (FWs, Nat, Routing), so IMO, what is most informative is the instance name. The type name should be easy to get once you downcast, right ?

I don't think there is any reason to force the NetworkFunction to have an instance name, that is why I went with the id when it is put in the pipeline. The Pipeline owns the stage and you find it by id. It is always possible to implement stages that have instance names, but I'm not sure if it should be required.

I believe there is a use case for that:

  • at some point you may want to know the composition of a pipeline; i.e. what stages (instances) it is made of.
  • So, it should be possible to query a pipeline and ask what stages it has.
  • With the present Id work, it shall be possible to provide a list of Ids. So far so good.
  • However, if we want names (a bit more user-friendly; I'd prefer to see stage "NAT44-stateless" rather than Id = 54", then the NF should have a trait method to provide a name:
  • True, it is always possible to let stages have names and implement some non-trait get_name() methods. However, if you need to iterate over the pipeline and step on stage Id = 54, in order to call the get_name() method to get the name you would first need to provide the right type to downcast to, right? ... whereas if the NF trait has a get_name() method (which would call the type-specific get_name()), then the downcast "guesswork" is not needed ? In other words: without a trait method, discovering the name of a NF requires knowing its original type (well, or all types and try until success) ?

No big deal if we don't want to have this introspection. But if we do ....

@mvachhar
Copy link
Contributor Author

mvachhar commented Mar 3, 2025

I don't think there is any reason to force the NetworkFunction to have an instance name, that is why I went with the id when it is put in the pipeline. The Pipeline owns the stage and you find it by id. It is always possible to implement stages that have instance names, but I'm not sure if it should be required.

I believe there is a use case for that:

  • at some point you may want to know the composition of a pipeline; i.e. what stages (instances) it is made of.
  • So, it should be possible to query a pipeline and ask what stages it has.
  • With the present Id work, it shall be possible to provide a list of Ids. So far so good.
  • However, if we want names (a bit more user-friendly; I'd prefer to see stage "NAT44-stateless" rather than Id = 54", then the NF should have a trait method to provide a name:
  • True, it is always possible to let stages have names and implement some non-trait get_name() methods. However, if you need to iterate over the pipeline and step on stage Id = 54, in order to call the get_name() method to get the name you would first need to provide the right type to downcast to, right? ... whereas if the NF trait has a get_name() method (which would call the type-specific get_name()), then the downcast "guesswork" is not needed ? In other words: without a trait method, discovering the name of a NF requires knowing its original type (well, or all types and try until success) ?

No big deal if we don't want to have this introspection. But if we do ....

We'll get the ability to have nice user-readable names once I am done with this thing, just have to figure out how to deal with the restrictions on Borrow. Annoyingly, the lifetime of the borrowed key type has to be the same as the actual key, which causes a problem in my current implementation. The Rust discord had no responses, so @daniel-noland will brainstorm a solution today, I hope.

@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch from 0e6bc46 to 300973e Compare March 4, 2025 16:37

This comment was marked as outdated.

@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch from 300973e to 31505cd Compare March 4, 2025 17:00

This comment was marked as outdated.

@mvachhar mvachhar mentioned this pull request Mar 7, 2025
@qmonnet

This comment was marked as outdated.

@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch 2 times, most recently from db25bcc to 0afce3e Compare March 7, 2025 17:37
@mvachhar
Copy link
Contributor Author

mvachhar commented Mar 7, 2025

I'm going to try to use the Borrow trait to fix the FIXME in the last commit.

Also, I forgot to add docs.

So the Borrow stuff I wanted to do cannot work, so I went with @daniel-noland's dataplane-id crate that was added.

Still need docs.

Copy link
Contributor

github-actions bot commented Mar 7, 2025

⚠️ One or more optional CI steps have failed!

click here to lament thy folly

@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch from 0afce3e to f1f57ab Compare March 10, 2025 15:17
Copy link
Contributor

⚠️ One or more optional CI steps have failed!

click here to lament thy folly

@mvachhar mvachhar force-pushed the pr/mvachhar/stage_ids branch from f1f57ab to 54226b3 Compare March 10, 2025 15:37
@mvachhar mvachhar removed the dont-merge Do not merge this Pull Request label Mar 10, 2025
Copy link
Contributor

⚠️ One or more optional CI steps have failed!

click here to lament thy folly

@mvachhar mvachhar added this pull request to the merge queue Mar 10, 2025
@mvachhar mvachhar removed this pull request from the merge queue due to a manual request Mar 10, 2025
@mvachhar mvachhar added this pull request to the merge queue Mar 10, 2025
Merged via the queue into main with commit f53f004 Mar 10, 2025
16 of 17 checks passed
@mvachhar mvachhar deleted the pr/mvachhar/stage_ids branch March 10, 2025 15:52
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.

3 participants