Thanks for the task @aborrero

Yeah the goal here will be to extend the provision script, so that selecting "cloud" under "vlan type" (I'll probably rephrase the menu names) will automatically add the cloud-private IP on the host side, and the trunk setup on the switch.

We'll probably need a few options in terms of combos:

That will cover most things, some like cloudgw and cloudnet may need some manual tinkering but we rarely configure those and netops are likely to be involved. For the rest those options should allow DC-Ops to add them without anyone changinig netbox after.

Change 983268 had a related patch set uploaded (by Cathal Mooney; author: Cathal Mooney):

[operations/software/netbox-extras@master] Refactor server provision script to select params based on profile

https://gerrit.wikimedia.org/r/983268

@ayounsi, @Volans I have uploaded the above patch to add the functionality as described.

As discussed previously on irc I took the approach of selecting host vlan settings (and other elements) based on the hostname. The current iteration of the patch only has a few example host profiles, but I will start adding the rest now. I have not attempted to support devices with multiple physical links, as there are only two which I know of (lvs and cloudceph), both of which we intend to move to single link.

The patch is a fairly significant refactor. There are elements from the current script, but given the number of changes it might be easier to review it as a completely new file without considering exactly what changed from before. The host profiles dict is also likely to get quite large, so I think we could move that to a YAML file added to the netbox hosts by puppet.

The change should get us closer to the goal of pushing all network configuration from Netbox. It covers all host network variants (with a single-link) I know of, and should eliminate the need for manual reservation of additional IPs for these right away. Longer term we can look at creating the entire host network config from this data in netbox (once we work out how to deal with the interface name - see T347411). We can then remove the various role-specific puppet classes teams are currently using to add sub-interfaces, bridges, additional IPs etc.

I added a brief wikitech article on how the host profiles work and some other details:

https://wikitech.wikimedia.org/w/index.php?title=Netbox_ProvisionServerNetwork_Script

I'm sure there will be constructive feedback and potentially changes, but I'm hopeful we can agree that the overall approach is worthwhile.

Thanks for the patch, it would take a bit to do a full pass given the size. I agree with the general direction. I think we should also think a bit more about what format the data should have and where it should reside, if in Puppet or another repo.

Big and needed change, thanks !

Looking at the doc at https://wikitech.wikimedia.org/wiki/Netbox/ProvisionServerNetwork_Script who is the audience ? I worry it's too dense for anyone to read.

Then I agree with Riccardo to focus on the data model (especially what you defined as HOST_PROFILES) first before fully reviewing the code.

Puppet seems like a good short to middle term location as it would allow service owners to maintain it for their own services, as well a reuse that data in other locations (I can think of cookbooks and Netbox reports for now)

I'd also suggest to add another level of abstraction for example a dict named "network_profile", to remove code duplication from the main HOST_PROFILES and make the latter more user friendly.
For example network profiles could be named "public", "cloud-hosts-private", "private-no-v6", or the same as host profiles if there is only one, like "ganeti"

Maybe also agree on what we call the text part of a hostname as we name it differently across the infra. Is it a group, is it a prefix, is it a cluster ?

About Cassandra let's see what's up with T269328: Cassandra instance DNS records - are they needed? hopefully we could keep it out to keep it simple.

Another approach, would be to separate the base config from the "extra" config.

For example we provision a Cassandra hosts as a regular "private vlan" host. And then call a "cassandra" Netbox script (which takes the host and a digit as parameter) to add the extra IPs. This should greatly help the long term maintainability of such critical automation (and ease this review :) )

The same could go for any hosts needing addition "trunked" vlans.

About bridges I'm wondering if we shouldn't keep it out for now to keep it simpler as well. As if I understand correctly this will still be synced from PuppetDB. The transition from Puppet driven to Netbox driven will happen in another phase.

@ayounsi thanks for the feedback!

In T346428#9418490, @ayounsi wrote:

Looking at the doc at https://wikitech.wikimedia.org/wiki/Netbox/ProvisionServerNetwork_Script who is the audience ? I worry it's too dense for anyone to read.

That's a good point. I guess I was trying to document the script, so mostly for our team to aid with the discussion on how it worked / should work. The end bit about the CSV is more targeted at DC-Ops (they seem ok with that section having reviewed). Perhaps I should split it up?

Then I agree with Riccardo to focus on the data model (especially what you defined as HOST_PROFILES) first before fully reviewing the code.

Puppet seems like a good short to middle term location as it would allow service owners to maintain it for their own services, as well a reuse that data in other locations (I can think of cookbooks and Netbox reports for now)

To me that is the logical place, if we in Infra Foundations are in agreement then I can move it to there for now.

I'd also suggest to add another level of abstraction for example a dict named "network_profile", to remove code duplication from the main HOST_PROFILES and make the latter more user friendly.

I had it done that way originally, and then changed it as I feared people would say it's too complex when reviewed :)

Always something of a balancing act between the model having too much indirection vs. less duplication. But yes I do agree that would be sensible, @Volans what do you think?

Maybe also agree on what we call the text part of a hostname as we name it differently across the infra. Is it a group, is it a prefix, is it a cluster ?

Agreed. Unsure also what is best. "Cluster" is good but I wonder might it cause confusion with sub-sets of hosts with a common prefix that are in separate clusters (at least at separate sites, but possible within sites they are grouped differently). Prefix is a bit generic, perhaps "host prefix"?

About Cassandra let's see what's up with T269328: Cassandra instance DNS records - are they needed? hopefully we could keep it out to keep it simple.

Yeah. I didn't want to remove any functionality that we already have to start off. I'll talk to Eric and see what he thinks. It is messy, so it would be good if we could drop it. Although if the word comes back that it's needed for now I'd prefer not to make the work to remove the requirement a per-requisite for moving this one forward.

Another approach, would be to separate the base config from the "extra" config.

For example we provision a Cassandra hosts as a regular "private vlan" host. And then call a "cassandra" Netbox script (which takes the host and a digit as parameter) to add the extra IPs. This should greatly help the long term maintainability of such critical automation (and ease this review :) )

Certainly an option. The question I'd have, though, is if we are able to define the number of instances based on "host prefix" then why not do it at once? DC-Ops have already articulated they think there are too many steps in the process, my personal sense is not to add more if they can be avoided. If the number cannot be generalized based on the host prefix then sure, that sounds like a good way to go.

FWIW the Cassandra code here is almost 100% copy/paste from before, only has small changes to support the VLAN_DOMAINS bit.

The same could go for any hosts needing addition "trunked" vlans.

The problem there is we're back to a situation where our team, or someone familiar with the network setup on a given host type, needs to manually go in and make changes in Network between running this script and reimaging. The intent was to standardize such setups, to avoid the need for us to be in the workflow and ensure mistakes aren't made. So tbh I'd not really be in favour of moving that out of this script.

About bridges I'm wondering if we shouldn't keep it out for now to keep it simpler as well. As if I understand correctly this will still be synced from PuppetDB. The transition from Puppet driven to Netbox driven will happen in another phase.

Ultimately the main goal is to drive our network configuration from Netbox. Importing from PuppetDB is an anti-pattern as far as I am concerned, the last remaining elements after taking care of trunked vlans are host-side bridges and the interface naming. So overall I think it should be here.

If it will aid review I can certainly remove that code for now though, and we can consider it separately as part of a subsequent patch? But I do think creating those structures in advance in Netbox is the only sensible long-term approach.

Potentially right now I could refactor and remove the following:

• CSV part
• Trident3 port check part
• Bridge config

That would leave just the "custom" script and the profile-based one to review. I would be anxious to add the above back in fairly quickly after, but I understand it's a lot to digest.

Thanks!

To follow up only on the Cassandra usecase, my proposal here is to actually remove steps from DCops as for them they would only deploy a regular private host. It's then on the service owner to add additional IPs. I don't think the burden of this odd Layer 3 config should be on DCops or I/F. Even though the "specific to cassandra" code is just a copy paste, it would be better to live off this already too complex code. It will make removing that edge case much simpler in the future as well.

For the additional trunked vlans setup it's indeed more of an idea, but I'm not suggesting to make it manual. I'm suggesting to offload any odd setup to a different script/cookbook for code maintainability. It would still take its config from the previously defined data model.

For the bridges I agree it's the good approach medium/long term, but I suggest we keep that part out for now to make this patch easier to review/test.

The Trident3 check should probably be a custom validator to also work on manual interface edits.

In T346428#9418800, @ayounsi wrote:

To follow up only on the Cassandra usecase, my proposal here is to actually remove steps from DCops as for them they would only deploy a regular private host. It's then on the service owner to add additional IPs. I don't think the burden of this odd Layer 3 config should be on DCops or I/F.

Well the goal is to provide a framework to our teams that will configure the network as they require it. The advantage of adding the required elements to Netbox at the provision stage is that we can build the correct network configuration from that data on the first reimage. That's obviously a future goal, but to put in context of where the intent here is coming from.

I'll try and get a sense of the current status of the Cassandra thing anyway and feed back.

For the additional trunked vlans setup it's indeed more of an idea, but I'm not suggesting to make it manual. I'm suggesting to offload any odd setup to a different script/cookbook for code maintainability. It would still take its config from the previously defined data model.

I don't think the tagged vlans being set here adds too much complexity to the code. Even if we define the requirements in one place, having separate scripts means DC-ops need to remember that for certain hosts the provisioning requires an extra script. I can't see why we would do that if we know in advance what is required. It leaves open the possibility that it's forgotten (as is the case now) and the reimage fails.

For the bridges I agree it's the good approach medium/long term, but I suggest we keep that part out for now to make this patch easier to review/test.

Sure. Short term :)

The Trident3 check should probably be a custom validator to also work on manual interface edits.

Ok yeah that's a good idea.

T358096: Automation to add extra IPs to servers for the Cassandra/extra IPs usecase.