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

Set weights in route manager #309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LostKobrakai
Copy link
Contributor

Adding this was proposed on slack. I couldn't find tests for that genserver. I'm not sure if this is fine to add like that.

@fhunleth
Copy link
Member

fhunleth commented Aug 4, 2021

Thanks! I'm adding this to my todo list for a vintage_net pass next week. I understand the motivation and agree that this is needed.

@fhunleth
Copy link
Member

I'm looking through the interface prioritization code. I'm thinking that I'd prefer a way to move this into VintageNet's configuration rather than making it changeable at runtime. The reason is to remove the case where interfaces are prioritized one way and then at a later time they're prioritized another way. I think this might lead to some confusion if there's TCP connection that can be made before or after the priority change based on lucky timing.

I suppose the question is whether you know at compile time whether you want eth0 prioritized over eth1 or the reverse. By default, VintageNet will prioritize eth0 over eth1 since its number is lower. I'm assuming you want eth1 prioritized over eth0 because of the PR. Is that right?

@fhunleth
Copy link
Member

See #332. My thought now is that for your case where you don't want the default way that you supply a callback function that returns the metric. I think that the code that you'll need will be pretty short and easier to understand.

Something like:

defmodule MyMetric do
  # highest to lowest priority
  def compute_metric("eth1", %{status: :internet}), do: 0
  def compute_metric("eth0", %{status: :internet}), do: 1
  def compute_metric("eth1", %{status: :lan}), do: 2
  def compute_metric("eth0", %{status: :lan}), do: 3
  def compute_metric(_, _), do: :disabled
end

While I didn't do it in that PR, I'm tempted to remove the weight feature since as I was working on this, it was feeling really easy to mess up. This might be me being paranoid, but given how hard routing issues are to debug, I'm leaning in that direction. On the other hand, it could be convenient to be able to pass a value in a network interface's configuration that gets propagated to the metric calculation function. This is half-baked, so I'll punt until I learn of a use case that really needs it.

@LostKobrakai
Copy link
Contributor Author

LostKobrakai commented Aug 19, 2021

The changes seem like a nice improvement over the current implemenatation.

On the other hand, it could be convenient to be able to pass a value in a network interface's configuration that gets propagated to the metric calculation function. This is half-baked, so I'll punt until I learn of a use case that really needs it.

That's something I was about to ask. In the system I'm currently working on there's the ability for users to choose their own prioritization between network interfaces (some might prefer ethernet, some cellular, …). Therefore we need not only a way to customize the priorities statically, but also dynamically. We can probably use our own global state in combination with the new route_metric_fun, but it would be really nice if it could just receive the interface config vintage net already handles. This will also easy the fact that those changes shall be persisted and reapplied on restart.

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.

2 participants