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

routes: fix metric rendering (LP: #2023681) #367

Merged
merged 2 commits into from Jun 20, 2023

Conversation

bengentil
Copy link
Contributor

Routes metric are rendered as signed integer instead of unsigned integer. This means all metric > 2147483647 is rendered as negative and is breaking the generated configuration.

Description

https://bugs.launchpad.net/netplan/+bug/2023681

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • [-] New/changed keys in YAML format are documented.
  • [-] (Optional) Adds example YAML for new feature.
  • [-] (Optional) Closes an open bug in Launchpad.

Routes metric are rendered as signed integer instead of unsigned integer.
This means all metric > 2147483647 is rendered as negative and is breaking the generated configuration.
@daniloegea
Copy link
Collaborator

Thanks so much for you contribution, @bengentil!

Your fix looks good. Do you think you can write a couple of unit test for it though?

You could add one for networkd and another for Network Manager in tests/generator/test_routing.py. Maybe calling them something like test_route_metric_rendering_lp2023681. It will ensure we'll not reintroduce this issue in the future.

Thanks again!

@daniloegea daniloegea self-requested a review June 15, 2023 13:23
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

Approved. Check my other comment, please. Maybe we could add a couple of unit tests as well.

@bengentil
Copy link
Contributor Author

bengentil commented Jun 19, 2023

Hi @daniloegea, I've pushed a couple of tests.

The tests are testing against 4294967294, which is not strictly the max value as the max value is used internally to set the metric as undefined (NETPLAN_METRIC_UNSPEC == G_MAXUINT == 4294967295)

For more details see:

This means technically 4294967295 can't be set as metric with netplan but this is another issue, here the goal is to avoid signed values as metrics.

@slyon slyon changed the title routes: fix metric rendering routes: fix metric rendering (LP: #2023681) Jun 20, 2023
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution. LGTM!

I'll leave merging to Danilo, when he feels his comments are resolved.

@daniloegea daniloegea merged commit bd00d37 into canonical:main Jun 20, 2023
10 checks passed
@daniloegea
Copy link
Collaborator

Thank you for your bug fix! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants