roachtest: don't inhibit cluster reuse on DNS deletion errors

[andreimatei]

Before this patch, a failure to delete a cluster's DNS records resulted in roachtest refusing to reuse that cluster for other tests. In general, refusing to reuse a cluster that has not been completely wiped is a sane policy (on the argument that the next test running on that cluster might be impacted by the cluster's dirty state), but DNS records in particular don't matter. So, let's be more tolerant of such errors.

For people outside of CRL, that DNS deletion seems to always fail (probably because no DNS record was created in the first place) -- so this patch helps me in particular.

Epic: None
Release note: None

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[andreimatei]

cc @RaduBerinde @srosenberg - a one liner for your kind consideration

[RaduBerinde]

bors r+

[craig]

Build succeeded:

• linux_arm64_build
• macos_amd64_build
• acceptance
• linux_amd64_fips_build
• linux_amd64_build
• local_roachtest_fips
• local_roachtest
• lint
• windows_build
• examples_orms
• macos_arm64_build
• check_generated_code
• docker_image_amd64
• unit_tests

[srosenberg]

but DNS records in particular don't matter. So, let's be more tolerant of such errors.

They do now since DNS is also used as a discovery service for multi-tenant roachprod clusters. @herkolategan I seem to recall we had some issue(s) around cluster reuse and stale DNS service record(s)?

[renatolabs]

Yes, I believe reusing the cluster with stale records may cause the next test's call to c.Start to fail (multi-tenant or not). I think the better fix would have been to skip deleting DNS records if none were created.

[andreimatei]

Yes, I believe reusing the cluster with stale records may cause the next test's call to c.Start to fail (multi-tenant or not)

I have not observed that, fwiw.
But in any case, I'll send a revert; I didn't understand the implications.

[RaduBerinde]

Sorry for jumping the gun on merging.

[srosenberg]

I have not observed that, fwiw. But in any case, I'll send a revert; I didn't understand the implications.

No worries. If it's a real issue for you, I can send a PR to do what Renato suggested. We already have a hook in RegisterServices where we can store the fact, and use it to conditionally opt out from DestroyDNS on wipe. Do you happen to have a stack trace? It's not immediately clear which of the gcloud CLI commands (used by DestroyDNS) would fail if there were no records created.

Sorry for jumping the gun on merging.

No problem. Unfortunately, even seemingly trivial changes to roachtest/roachprod may have subtle side-effects. That's one of the reasons we usually kick off a run in CI with SELECT_PROBABILITY for smoke testing. Except in this case, it wouldn't have helped since the issue is non-deterministic. Quoting from an internal slack thread, @DarrylWong observed this rather subtle case of cluster reuse "interference" as a result of not invoking DestroyDNS,

I'm running into an issue with removing default port assumptions when two tests are run after each other on the same reused cluster. The first test (i.e. pgx) will randomly find a port (29000) and run fine. The second test (i.e. pop) relies on 26257 so it is explicitly specified. Run alone, pop passes but when run after pgx, it fails.
After some testing, it seems like what's happening is that in c.Start() we don't immediately create a new service at the specified port. Instead we first call DiscoverService to see if that node already has a service. If it does, then we forgo creating the service. In the above example, this means pop's call to c.Start() never creates a service on port 26257 because it see's we already have one on port 29000.
Long story short, my question here is if always using the found port regardless of what startOpts is intended behavior? If so then is the only way around this to disable cluster reuse when a port is specified? If not then I'm thinking we have to check if the discovered port is the same as the one specified and if it isn't then we have to delete the records?

[andreimatei]

Reverting in #124768

If it's a real issue for you, I can send a PR to do what Renato suggested. We already have a hook in RegisterServices where we can store the fact, and use it to conditionally opt out from DestroyDNS on wipe. Do you happen to have a stack trace? It's not immediately clear which of the gcloud CLI commands (used by DestroyDNS) would fail if there were no records created.

Let me look into it more, and understand exactly what wasn't working for me.

[andreimatei]

I've figured out my problem -- in #120340 there was confusion between the public DNS zone used for the A records, and the private DNS zone used for the SRV records. The DNS record creation (in particular, the SRV records creation) worked fine, but then the deletion tried to delete them from the public zone, instead of the private one, and failed.
This manifested in the smoke test that you've ran in failures to reuse clusters. So the smoke test succeeded, I believe, but took longer than it should have. In the _runner_logs files, you can see errors like:

[w7] 21:49:51 test_runner.go:641: Unable to reuse cluster: teamcity-15383971-1716500320-21-n1cpu4 due to: TRANSIENT_ERROR(dns_problem): output: ERROR: (gcloud.dns.record-sets.delete) HTTPError 403: Forbidden: exit status 1. Will attempt to create a fresh one

I'll push a fixed version of #120340, and this time it'll be good™.

---

We already have a hook in RegisterServices where we can store the fact, and use it to conditionally opt out from DestroyDNS on wipe. Do you happen to have a stack trace? It's not immediately clear which of the gcloud CLI commands (used by DestroyDNS) would fail if there were no records created.

I won't touch this any more myself :), but I think that there is a problem here that should probably be addressed. The SRV records are conditionally created (here), but unconditionally deleted (here). The deletion fails when the record doesn't exist ([1]). And when deletion fails, clusters are not reused.

Specifically, the conditional creation is inhibited when the test does not run on the default "provider" (i.e. GCE) or on the default GCE project.

[1]: The error I was getting locally was:

Unable to reuse cluster: andrei-1716924081-01-n4cpu4 due to: TRANSIENT_ERROR(dns_problem): output: ERROR: (gcloud.dns.record-sets.delete) HTTPError 404: The 'parameters.name' resource named '_system-sql._tcp.andrei-1716924081-01-n4cpu4.roachprod-managed.dataexmachina.dev.' does not exist.: exit status 1. Will attempt to create a fresh one

Notice the does not exist. I'm not entirely sure why your smoke test run showed a different error (Forbidden); maybe it has to do with how users/IAM are setup different for the CRL GCP project.

[srosenberg]

I've figured out my problem -- in #120340 there was confusion between the public DNS zone used for the A records, and the private DNS zone used for the SRV records.

Nice! Both zones are actually public but mutually exclusive. I am guessing Forbidden in our case was because of the wrong zone name.

I won't touch this any more myself :), but I think that there is a problem here that should probably be addressed. The SRV records are conditionally created (here), but unconditionally deleted (here). The deletion fails when the record doesn't exist ([1]). And when deletion fails, clusters are not reused.

I'll follow up with another PR. Let's first merge 102340, after the smoke test clears it.

[herkolategan]

If it does, then we forgo creating the service. In the above example, this means pop's call to c.Start() never creates a service on port 26257 because it see's we already have one on port 29000.

@srosenberg Getting caught up, but you are correct this is the main reason we destroy the records. In theory we should be throwing an error if we try to register the same service with a different port, or as @DarrylWong mentioned possibly re-register the service under the new port.

I won't touch this any more myself :), but I think that there is a problem here that should probably be addressed.

@andreimatei I agree this is a bug that should be addressed.