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

[nexus] remove services queries & OMDB commands #5044

Closed
wants to merge 19 commits into from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Feb 12, 2024

Depends on #5033.

As described in #4947, we would like to remove the services table, as its only remaining use is DNS propagation. PR #5033 changes DNS propagation to no longer use the services table. Now that we're no longer consuming this table, this commit removes the one method which queries that table (Datastore::services_list_kind) and its two remaining callers, the OMDB commands that query the table. I've also removed the test for inserting and querying this table.

Note that I have not yet removed the code in RSS that actually populates this table when the rack is set up. I thought it might be desirable to still populate this data in case we have to roll back to a previous version of Nexus that uses the services table. If this isn't something we care about, I can remove that code as well, allowing us to remove the Datastore methods for inserting to services.

hawkw added 18 commits February 8, 2024 15:40
after a bunch of messing with it i made the executive decision to just
get rid of that test. the test for DNS propagation exercises it, and
it's harder to test now
Depends on #5033.

As described in #4947, we would like to remove the `services` table, as
its only remaining use is DNS propagation. PR #5033 changes DNS
propagation to no longer use the `services` table. Now that we're no
longer consuming this table, this commit removes the one method which
queries that table (`Datastore::services_list_kind`) and its two
remaining callers, the OMDB commands that query the table. I've also
removed the test for inserting and querying this table.

Note that I have *not* yet removed the code in RSS that actually
populates this table when the rack is set up. I thought it might be
desirable to still populate this data in case we have to roll back to a
previous version of Nexus that uses the `services` table. If this isn't
something we care about, I can remove that code as well, allowing us to
remove the `Datastore` methods for inserting to `services`.
@hawkw hawkw requested review from davepacheco and smklein February 12, 2024 20:16
@hawkw hawkw self-assigned this Feb 12, 2024
Base automatically changed from eliza/dns-in-dns to main February 12, 2024 21:25
@hawkw hawkw marked this pull request as ready for review February 13, 2024 19:54
Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

LGTM, though the github checks look like real failures to me. I think this is just an EXPECTORATE test failing within the omdb test suite -- try running the following locally to fix:

EXPECTORATE=overwrite cargo nt -p omicron-omdb

@smklein
Copy link
Collaborator

smklein commented Feb 26, 2024

Note that I have not yet removed the code in RSS that actually populates this table when the rack is set up. I thought it might be desirable to still populate this data in case we have to roll back to a previous version of Nexus that uses the services table. If this isn't something we care about, I can remove that code as well, allowing us to remove the Datastore methods for inserting to services.

I'm okay with doing this incrementally!

@jgallagher
Copy link
Contributor

Now that we're no longer consuming this table, this commit removes the one method which queries that table (Datastore::services_list_kind) and its two remaining callers, the OMDB commands that query the table. I've also removed the test for inserting and querying this table.

I ran into two other places that are consuming this table.

  1. omdb db network list-eips indirectly "joins" against service to attach a service kind to each service external IP. We could maybe use external_ip.description instead, although it looks like it's only populated for nexus and external-dns (??). Another option might be to join against the service_network_interface table, which has a service_id column, and we populate both the name and the description. From madrid:
/omicron> SELECT eip.ip,eip.name,eip.description,sni.name,sni.description FROM external_ip AS eip LEFT JOIN service_network_interface AS sni ON eip.parent_id = sni.service_id WHERE eip.is_service;
      ip      |                       name                        | description  |                       name                        |        description
--------------+---------------------------------------------------+--------------+---------------------------------------------------+----------------------------
  172.20.28.1 | external-dns-d3471b34-6c67-4aef-bf53-3fe70b65efb8 | external_dns | external-dns-d3471b34-6c67-4aef-bf53-3fe70b65efb8 | external_dns service vNIC
  172.20.28.6 | NULL                                              | NULL         | ntp-af51b1eb-1266-4235-b2fd-6526f6ef8122          | ntp service vNIC
  172.20.28.5 | NULL                                              | NULL         | ntp-aaef9e47-a704-4fcd-b333-027bfd7e1ac9          | ntp service vNIC
  172.20.28.2 | nexus-6b0cf0b4-b5c6-4eda-a57f-7d110cb46324        | nexus        | nexus-6b0cf0b4-b5c6-4eda-a57f-7d110cb46324        | nexus service vNIC
  172.20.28.3 | nexus-a365f750-1804-4021-ada8-e0532c973c32        | nexus        | nexus-a365f750-1804-4021-ada8-e0532c973c32        | nexus service vNIC
  172.20.28.4 | nexus-a87d43d8-8a94-4a18-95ff-12206ee833d9        | nexus        | nexus-a87d43d8-8a94-4a18-95ff-12206ee833d9        | nexus service vNIC
  172.20.28.7 | nexus-5acb41a9-f76c-4b3c-8313-2a74ce37c32f        | nexus        | nexus-5acb41a9-f76c-4b3c-8313-2a74ce37c32f        | nexus service vNIC
(7 rows)
  1. vpc_resolve_to_sleds joins the service_network_interface table against service to map service vNICs to sleds. I'm not sure off the top of my head what to do about this one.

Before we stop populating the service table, we probably need to try deleting it from schema.rs and seeing if there are other consumers lurking around.

@jgallagher
Copy link
Contributor

Closing this in favor of #5287

@jgallagher jgallagher closed this Mar 19, 2024
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.

3 participants