Page MenuHomePhabricator

Drop most of mwopenstackclients.DnsManager in favour of designateclient
Open, Needs TriagePublic

Description

<Krenair> So I was just digging into how https://phabricator.wikimedia.org/T224000 occurred
<Krenair> Managed to get devstack etc working on a little test instance I made in the openstack project
<Krenair> tried client.recordsets.create('f4dd2c05-fef8-48c3-852d-a21f68cb28f4', "asd.{u'status': u'ACTIVE'}.alextest.wmflabs.org.", 'A', ['127.0.0.1'])
<Krenair> got designateclient.exceptions.BadRequest: Provided object is not valid. Got a ValueError error with message Host name asd.{u'status': u'ACTIVE'}.alextest.wmflabs.org. is not match
<Krenair> I realised this script that broke was using mwopenstackclients instead of designateclient directly, ok
<Krenair> So I dug into that and found... this: https://gerrit.wikimedia.org/r/plugins/gitiles/operations/puppet/+/refs/heads/production/modules/openstack/files/clientpackages/mwopenstackclients.py#202
<Krenair> :|
<Krenair> we're actually sending directly to the designate API without any designateclient in sight
<Krenair> I thought I got X-Auth-Sudo-Tenant-ID working in designateclient once, back when we would've been on jessie or trusty...
<Krenair> but anyway, in stretch, python-designateclient is 2.3.0-2
<Krenair> jessie seems to have 1.0.3-1 so I guess that explains that

Related Objects

StatusAssignedTask
OpenNone
Resolvedaborrero
Resolvedaborrero
Resolvedaborrero
ResolvedPapaul
ResolvedJHedden
Resolvedaborrero
Resolvedaborrero
ResolvedPapaul
Resolvedaborrero
Resolvedaborrero
Resolvedaborrero
Resolvedaborrero
ResolvedAndrew
Resolvedaborrero
Resolvedaborrero
ResolvedAndrew
Resolvedaborrero
Resolvedaborrero
ResolvedAndrew
ResolvedMarostegui
Resolvedaborrero
ResolvedAndrew
OpenNone
ResolvedAndrew
ResolvedAndrew
InvalidJHedden

Event Timeline

Krenair created this task.May 31 2019, 2:13 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 31 2019, 2:13 AM

There are some parts of DnsManager that are worth keeping, but we should definitely replace the _* private methods that are hacking in support for the X-Auth-Sudo-Tenant-ID header with the proper upstream client library.

Well that would leave these:

def zones(self, name=None, params=None):
def create_zone(
def ensure_zone(
def recordsets(self, uuid, name=None, params=None):
def create_recordset(
def update_recordset(
def ensure_recordset(
def delete_recordset(self, uuid, rs):

All of this is replaceable with the stuff under designateclient.zones and designateclient.recordsets, except the ensure stuff which probably belongs in applications rather than the library.

Furthermore there is no reason to have all these extra functions for designate if we're not going to both complete it to cover all functionality of the Designate API and to do the same for every other OpenStack service we have installed.

Looked at which stuff runs jessie - based on parsing the dhcpd files it's, cloudservices1003, and a bunch of cloudvirt/labpuppetmaster/labmon/labstore hosts
Users of mwopenstackclients.DnsManager are wmcs-dns-floating-ip-updater.py which runs on control hosts, and the util::admin_scripts scripts which all seem to run on control too.

So actually this might all already be okay to do?

All of this is replaceable with the stuff under designateclient.zones and designateclient.recordsets, except the ensure stuff which probably belongs in applications rather than the library.

The ensure stuff is definitely what I would want to keep. I don't understand the "probably belongs in applications" part of your argument. Libraries exist to reduce code duplication and make proper implementation of algorithms easier. ensure_recordset is used in 3 utility tools. I don't see why that code should be copied rather than living in our shim library that makes using the upstream libraries easier.

Furthermore there is no reason to have all these extra functions for designate if we're not going to both complete it to cover all functionality of the Designate API and to do the same for every other OpenStack service we have installed.

I'm not personally convinced that this argument holds. We can easily expose an initialized instance of the upstream designate client via a new mwopenstackclients.Clients.designateclient method mirroring the helpers for other upstream libraries. DnsManager can be reworked to use designateclient internally and remove the unneeded private methods. I don't understand why it would be necessary for this utility class to be extended to cover the entire designate api preemptively nor why we would need to add utility wrappers for all other openstack apis. There probably are other nice utility methods that you, Andrew, and others have made sprinkled throughout our various openstack automation scripts and hook handlers. It would be great to promote them up to the mwopenstackclients library, but I don't see how that having not been done yet bars us from continuing to use work that has already been done to move the DnsManager wrapper I originally wrote for wmcs-wikireplica-dns.py from the mwopenstackclients library.

Krenair added a comment.EditedMay 31 2019, 8:55 AM

All of this is replaceable with the stuff under designateclient.zones and designateclient.recordsets, except the ensure stuff which probably belongs in applications rather than the library.

The ensure stuff is definitely what I would want to keep. I don't understand the "probably belongs in applications" part of your argument. Libraries exist to reduce code duplication and make proper implementation of algorithms easier. ensure_recordset is used in 3 utility tools. I don't see why that code should be copied rather than living in our shim library that makes using the upstream libraries easier.

You're right. It would be out of place in designateclient as it does not map directly to a single request but it is a utility function.
ensure_zone is unused but ensure_recordset has 3 users.
On an unrelated note the current implementation of ensure_recordset appears dangerous as type is not taken into account when determining whether a record exists, should be updated, etc. I wouldn't want to ensure_recordset on tools.wmflabs.org SPF and have it overwrite the tools.wmflabs.org A record if that happens to get returned as the first item.

Furthermore there is no reason to have all these extra functions for designate if we're not going to both complete it to cover all functionality of the Designate API and to do the same for every other OpenStack service we have installed.

I'm not personally convinced that this argument holds. We can easily expose an initialized instance of the upstream designate client via a new mwopenstackclients.Clients.designateclient method mirroring the helpers for other upstream libraries. DnsManager can be reworked to use designateclient internally and remove the unneeded private methods. I don't understand why it would be necessary for this utility class to be extended to cover the entire designate api preemptively nor why we would need to add utility wrappers for all other openstack apis. There probably are other nice utility methods that you, Andrew, and others have made sprinkled throughout our various openstack automation scripts and hook handlers. It would be great to promote them up to the mwopenstackclients library, but I don't see how that having not been done yet bars us from continuing to use work that has already been done to move the DnsManager wrapper I originally wrote for wmcs-wikireplica-dns.py from the mwopenstackclients library.

If we're just going to keep ensure_recordset and maybe ensure_zone then ok.

Krenair renamed this task from Once all our stuff runs stretch, drop mwopenstackclients.DnsManager in favour of designateclient to Drop most of mwopenstackclients.DnsManager in favour of designateclient.May 31 2019, 8:57 AM

Change 513752 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/puppet@production] openstack mwopenstackclients: Add designateclient

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

Paladox added a subscriber: Paladox.Jun 2 2019, 9:00 PM

Change 513909 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/puppet@production] openstack mwopenstackclients: Use designateclient in ensure functions

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

Change 513910 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/puppet@production] openstack scripts: Use designateclient where possible

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

Change 513911 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/puppet@production] openstack mwopenstackclients: Remove unused methods provided by designateclient now

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

Change 513752 merged by Andrew Bogott:
[operations/puppet@production] openstack mwopenstackclients: Add designateclient

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

Change 513909 merged by Andrew Bogott:
[operations/puppet@production] openstack mwopenstackclients: Use designateclient in ensure functions

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

bd808 moved this task from Backlog to Designate on the Cloud-VPS board.Sun, Nov 10, 11:44 PM