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

Bug in renumbering tool #660

Open
arnoldnipper opened this issue Mar 6, 2020 · 6 comments
Open

Bug in renumbering tool #660

arnoldnipper opened this issue Mar 6, 2020 · 6 comments
Labels

Comments

@arnoldnipper
Copy link
Contributor

@arnoldnipper arnoldnipper commented Mar 6, 2020

Obviously there is an issue in handling longer masks (see #304 ). I also noticed that the masks still have to be of the same size. That does not make sense. Typically ixes go from a /n to /n-1 or even /n-2.

20200306 PeeringDB 03

@peeringdb/pc please +1

@arnoldnipper arnoldnipper added this to the 1 Decide milestone Mar 6, 2020
@shane-kerr

This comment has been minimized.

Copy link

@shane-kerr shane-kerr commented Mar 6, 2020

+1

@grizz

This comment has been minimized.

Copy link
Member

@grizz grizz commented Mar 6, 2020

I'd have to look back and find the discussion on this, but I believe that we all decided renumbering to the same prefix length was the only thing that made sense, since what else can you do for an auto renumber? After renumbering, in this case a /25, you could then edit the prefix and make it a /24 for example. KISS theory. :)

I'm very confused by the output there, as it should obviously not be .128 for each. We'll look into that more.

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Mar 6, 2020

KISS:

  • old_mask >= new_mask
  • align the new addresses to the start address of the new range
@grizz

This comment has been minimized.

Copy link
Member

@grizz grizz commented Mar 6, 2020

KISS:

  • old_mask >= new_mask

I disagree that it's simpler, at worst it's just adding 1 extra step: renumber and then change the prefix. That one extra step for the AC member doing it makes the backend more complex and require more tests, etc.

  • align the new addresses to the start address of the new range

This is a clear bug that only happens with /25s. Yesterday, this tool had been used 6 times over the last two years, yet it's taken a lot of time discussing and creating. At some point all of our time is better spent just manually doing the handful of renumbers instead of trying to account for every corner case in a corner case tool. :)

I'm +1 and Minor for fixing the bug, any more time spent discussing how it should work is going to bump it to a Major in my opinion.

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Mar 7, 2020

@grizz

  • the tool is already checking masks. For whatever reason, it currently insists that old_mask == new_mask. Which is rubbish and confuses. So simply check that old_mask >= new_mask which makes sense

  • even when renumbering does not happen often, it greatly helps our users and AC and prevents from manual editing which is highly error-prone. Talking about corner cases of corner cases is thinking little of what AC does IMHO

@grizz

This comment has been minimized.

Copy link
Member

@grizz grizz commented Mar 9, 2020

Sold, +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.