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

Support IPv4 prefixes smaller than /26 #318

Merged
merged 11 commits into from
Feb 13, 2024
Merged

Support IPv4 prefixes smaller than /26 #318

merged 11 commits into from
Feb 13, 2024

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Feb 1, 2024

This PR introduces the ability to support IPv4 prefixes smaller than /26. /26 prefixes are for the standard M-Lab site (1 switch and 4 servers), while smaller ones will be designed to reside on a single server. The notion is that we will support /28 prefixes, but these changes should be generic enough to also support a /27 prefix on a single server, albeit with far more IPv4 addresses wasted.

Since /28 (or /27) sites won't have a switch, such sites should set the default switch configuration to "null". When this is the case, DNS records for the non-existent switch will not be created.

Additionally, this PR removes the old, unused experiment aliases of "ndt-iuipui" and "neubot-mlab". We haven't used those quite a long time. This change should remove the DNS entries for those experiment aliases, effectively retiring them for good and in earnest.


This change is Reviewable

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 8 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)


cors-settings.json line 1 at r1 (raw file):

[

Is this a mistake? See: create_siteinfo_api.sh


formats/v1/sites/sites.json.jsonnet line 9 at r1 (raw file):

      local m = site.Machine(machine);
      // /26 sites support 12 experiments, while /28 sites only support 11.
      local expCount = if std.endsWith(site.NetworkCIDR('v4', machine), '26') then 12 else 11;

I recommend putting this logic in the site object file as a new public method. Maybe as a function on the Machine type, since it's the thing that will have some number of experiments.


lib/site.jsonnet line 41 at r1 (raw file):

    local i = $.MachineIndex(m),
    local v4net = $.network.ipv4.prefix,
    local drac_offset = if $._net_subnet(v4net) == 26 then 3 else 1,

I like 26 being explicit. I don't like 28 being implicit.

Maybe a variation on:

 (
  if x == 26 then 3
  else if x == 28 then 1
  else error 'foo'
)

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


formats/v1/sites/sites.json.jsonnet line 9 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I recommend putting this logic in the site object file as a new public method. Maybe as a function on the Machine type, since it's the thing that will have some number of experiments.

What I've done instead is to add a newexperiment_count field to site annotations, and this file now just references that values. A benefit of this is that it should allow us to somewhat seamlessly support /26, /27, /28 and even /29 prefixes without any code changes. For example, a /29 prefix would support 3 experiments, which today are "mask", "ndt" and "revtr". What do you think?


lib/site.jsonnet line 41 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I like 26 being explicit. I don't like 28 being implicit.

Maybe a variation on:

 (
  if x == 26 then 3
  else if x == 28 then 1
  else error 'foo'
)

It is intentional that /28 is implicit. The notion is that any prefix less than a /26 can be supported without modification to the code, and all of them would use the same IP addressing schema. IP assignments and experiment indexing would look the same for /27, /28, and possibly even /29 (but with fewer supported experiments).


cors-settings.json line 1 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Is this a mistake? See: create_siteinfo_api.sh

Yes, this was a mistake. I just realized that when I do builds in a local container within the repo, than when I run make clean there is rule to delete all JSON files. It just slipped past me this time that the file had been removed, and a subsequent git commit -a added the deletion to one of my commits. I have restored the file and will be careful about this. We may also decided at some point to make clean a bit less greedy in what it deletes.

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

@stephen-soltesz: PTAL?

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


formats/v1/sites/sites.json.jsonnet line 9 at r1 (raw file):

Previously, nkinkade wrote…

What I've done instead is to add a newexperiment_count field to site annotations, and this file now just references that values. A benefit of this is that it should allow us to somewhat seamlessly support /26, /27, /28 and even /29 prefixes without any code changes. For example, a /29 prefix would support 3 experiments, which today are "mask", "ndt" and "revtr". What do you think?

I removed the experiment_count field, and instead added a new ExperimentCount() function as part of the site object, which more closely matches your original suggestion.


lib/site.jsonnet line 41 at r1 (raw file):

Previously, nkinkade wrote…

It is intentional that /28 is implicit. The notion is that any prefix less than a /26 can be supported without modification to the code, and all of them would use the same IP addressing schema. IP assignments and experiment indexing would look the same for /27, /28, and possibly even /29 (but with fewer supported experiments).

These functions are still implicit with regard to offset, notwithstanding an explicit match against 26. That is, if the prefix is /26, do A, for everything else do B.

To make this more safe or explicit I added validation of the prefix size to the _net_subnet() function. If a call is made to that function with a prefix that isn't supported jsonnet will exit with an error.

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

This changes in this PR, as it stands right now, are reflected on mlab1-nuq0t.mlab-sandbox.measurement-lab.org. I build epoxy-images against these changes, and then signaled epoxy to update mlab1-nuq0t to flash a new NIC image. When the machine came back up everything looks as expected in terms of IP address assignments.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

Copy link
Contributor

@stephen-soltesz stephen-soltesz 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. I have a few high-level questions.

I think of experiment count as being a machine property, but we have some assumptions about how we manage things that may not make that possible. This may not be an issue with your PR but with how our site/machine/service model is evolving.

Reviewed 1 of 5 files at r2, 3 of 6 files at r4.
Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)


formats/v1/sites/sites.json.jsonnet line 8 at r4 (raw file):

    nodes: [
      local m = site.Machine(machine);
      local expCount = site.ExperimentCount();

I'm wrestling with whether this is a machine or site property.

Practically, some machines have constraints either b/c of their netblock size or our management policy (1-service per VM).

In general, we organize around sites, machines per site, and services per machine.

Is this a site property?


lib/site.jsonnet line 70 at r4 (raw file):

    local v6net = $.network.ipv6.prefix,
    local drac = $.DRAC(m),
    local bcast_offset = if $._net_subnet(v4net, 'v4') == 26 then 63 else 15,

+15 only works for a /28 right?


lib/site_virtual.jsonnet line 95 at r4 (raw file):

    // practically speaking only records for ndt will be generated since it is
    // the only experiment that has "cloud_enabled=true".
    12

Our deployment model today is one service per VM. And we assign the VM host IP to the service in DNS. Could we set cloud_enabled=true for any other service and have things WAI?

We haven't used experiment names like ndt-iupui, or neubot-iupui for a long
time now, and we no longer need those aliases in the experiments file, which
means we will no longer create DNS records with those names either.
The changes presuppose that there are only 2 supported prefix lengths, 26 and
28. If the prefix isn't 26, then it is presumed to be 28.
New /28 sites will not have a switch. In those cases, the "switch" element for
a site is set to null. If this is true, then don't attempt to include switch
information in the the switches format.
/28 sites will not have a switch, so no switch DNS records should be created
for these sites.
The default value is 12, and can be overridden on a site-by-site basis. For
example, on sites using a /28 IPv4 prefix this should be set to 11, since a /28
can only support 11. This also gives us the ability to possibly even support a
/29, which would only support 3 experiments.
Additionally, configures _net_subnet() function to accept a new proto parameter
and enforces supported prefix lengths per protocol.

Tries to be more explicit than implicit about prefix length and experiment
count, as well as other uses of prefix length.

Adds IPv6 prefix for nuq0t (ISC site).
Virtual sites have a /32 prefix (a single address).
Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

@stephen-soltesz: Thank you for the feedback. PTAL?

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


formats/v1/sites/sites.json.jsonnet line 8 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I'm wrestling with whether this is a machine or site property.

Practically, some machines have constraints either b/c of their netblock size or our management policy (1-service per VM).

In general, we organize around sites, machines per site, and services per machine.

Is this a site property?

Today, experiment definitions are global in this repository. While it is true that experiments run on machines which themselves run inside of sites, we always run the same group of experiments on every machine at a site. Indeed, the only exception to this today is virtual sites, which only run NDT. It is possible that in the future we could support running different experiments on different machines at a site, but there is no mechanism to do this today. Do you think it would make sense to try to add this functionality now, along with these changes? It may not be too hard.


lib/site.jsonnet line 70 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

+15 only works for a /28 right?

True, and thank you for catching this. I updated the formulate to account for /26, /28 and /29.


lib/site_virtual.jsonnet line 95 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Our deployment model today is one service per VM. And we assign the VM host IP to the service in DNS. Could we set cloud_enabled=true for any other service and have things WAI?

I think the build of siteinfo would probably continue to work, but it would be publishing incorrect data. It would publish DNS records and experiment information for a virtual sites that doesn't actually exist. Addition of this function to this file is a no-op placeholder so that the function that actually doesn't anything in lib/site.jsonnet can exist. It just replicates the current behavior.

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @nkinkade)


formats/v1/sites/sites.json.jsonnet line 8 at r4 (raw file):

Previously, nkinkade wrote…

Today, experiment definitions are global in this repository. While it is true that experiments run on machines which themselves run inside of sites, we always run the same group of experiments on every machine at a site. Indeed, the only exception to this today is virtual sites, which only run NDT. It is possible that in the future we could support running different experiments on different machines at a site, but there is no mechanism to do this today. Do you think it would make sense to try to add this functionality now, along with these changes? It may not be too hard.

I'm not close enough to this to know what the right thing is. I'm just noting that something feels funny. And, I still have the impression that "experiment count" is a property of a machine given how we manage them. It doesn't have to be resolved in this PR, I'm just noting it.

@nkinkade nkinkade merged commit d28b1d1 into main Feb 13, 2024
3 checks passed
@nkinkade nkinkade deleted the sandbox-kinkade branch February 13, 2024 20:28
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