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

Überbackend (questionable) improvements #15047

Closed
wants to merge 3 commits into from

Conversation

miodvallat
Copy link
Contributor

Short description

This is the result of my investigation of #15045.
A bugfix leads to another and another and eventually things appear to work again.

So, this attempts to fix two issues:

  • when an sql insertion fails, not as a severe problem (such as a database connection timeout) but simply because no insertion took place, this should be reported as an error, but was not.
  • when querying an Überbackend setup with more than one source, if the query of one source suceeds but returns no results, the remaining sources should be queried as well, so that another source returning success AND results can be preferred.

As a result of these fixes, "sql+bind-with-dnssec-db" setups can now properly make use of the bind dnssec database if the first backend (sql) reports errors (for writes) and no content (for reads) due to the domain being worked on being absent from that database, but present in bind.

However, I'm raising the "I don't know what I am doing" flag, this might break other multiple-backend setups I have not thought of. Reviewers welcome!

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

This lets ueberbackend try further backends for get operations if the
given backend reports success but results are empty.

This gives gsql+bind-with-dnssec-db a chance to reach the dnssec db.
@coveralls
Copy link

coveralls commented Jan 17, 2025

Pull Request Test Coverage Report for Build 12826166873

Details

  • 29 of 29 (100.0%) changed or added relevant lines in 4 files are covered.
  • 46 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.03%) to 64.924%

Files with Coverage Reduction New Missed Lines %
pdns/backends/gsql/gsqlbackend.hh 1 97.14%
pdns/tcpiohandler.cc 2 68.18%
modules/lmdbbackend/lmdbbackend.cc 2 72.75%
pdns/recursordist/test-syncres_cc2.cc 3 88.91%
pdns/packethandler.cc 3 72.48%
pdns/misc.cc 3 64.96%
pdns/recursordist/test-syncres_cc1.cc 5 89.41%
pdns/recursordist/syncres.cc 6 80.22%
pdns/recursordist/rec-tcp.cc 10 66.07%
pdns/ssqlite3.cc 11 63.11%
Totals Coverage Status
Change from base Build 12825997591: 0.03%
Covered Lines: 126463
Relevant Lines: 164098

💛 - Coveralls

@rgacogne rgacogne requested a review from Habbie January 17, 2025 09:52
@mind04
Copy link
Contributor

mind04 commented Jan 17, 2025

The return true means "function implemented" Ueberbackend will stop after the first backend that implemented the function.
This pull will change this to the first working backend. A temporary db failure, for example, may result in keys ending up in the wrong backend. This is similar to what happened with tsig a while back. This was rectified in #11759

@miodvallat
Copy link
Contributor Author

Thanks for the hint. Closing for now, back to the drawing board.

@miodvallat miodvallat closed this Jan 17, 2025
@miodvallat
Copy link
Contributor Author

Interestingly the überbackend changes turn out not to be necessary to solve my issue... will reopen, without the questionable commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants