Opened 9 years ago

Closed 9 years ago

#389 closed enhancement (fixed)

Configuration of b10-recurse trough the config manager

Reported by: vorner Owned by: vorner
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I modified the b10-recurse to be configured by the config manager. It is possible to configure the addresses and ports it listens on and the addresses it forwards queries to. If it has no address to forward to, it is in recursive mode, if it has, it is in forward mode.

It still has the -u and -v parameters. Currently, verboseness is global setting for single run of bind10 and will probably stay so until logging system is created. The -u will be dropped, since changing user is job for boss, not separate processes.

The bind10 still takes -f to tell it to run recurse instead of auth, changing this is a different task.

There are two problems. One is, it is impossible to change the listening sockets at runtime, as it is not possible to close the current ones (#388). The second is it is not possible to change the configuration trough bindctl, it doesn't handle properly configuring maps inside lists (#384). I suggest this should be left as separate tasks and this one revied and merged back to branches/trac327.

The current code is in branches/vorner-recursor-config, between r3317 to r3353.

I propose this changelog entry:

[func]    vorner
The b10-recursive is configured trough config manager.
It has "listen_on" and "forward_addresses" options.
(trac389, SVN rTBD)

Subtickets

Change History (9)

comment:1 Changed 9 years ago by vorner

  • Owner set to UnAssigned
  • Status changed from new to reviewing
  • Type changed from defect to enhancement

comment:2 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte
  • Status changed from reviewing to accepted

comment:3 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to vorner
  • Status changed from accepted to reviewing

recursor.cc:113: comment only talks about upstream_, not listen_
do these values really need to be public?

Regarding that comment at recursor.c:563: If we try to update the
interfaces we listen on, and it fails, and we try to revert, and
that fails too, we should probably just give up completely.

We could try to be a bit smarter (figure out if there are
addresses in both old and new, try to add new first, and then
remove the old ones, is one of them, if the adding failed, at least
we'd still be listening on the old ones) but I think giving up is
just as reasonable (and this is i guess also dependent on #384).

About changing the forwarders list with bindctl; it is actually possible, though way unuserfriendly; you have to know the internal data structure. It also doesn't work :) (the list is updated but the upstream_ value is not). This is because of another bug in the config data parser, which I mentioned in my comment #384. If that is fixed, we'll need to remove the '/' at the end of the keys on lines 499 and 501.

For now, apart from the minor issues stated at the beginning of this comment, I think it this code is good. With the note that once we change those key strings as part of #384.

comment:4 in reply to: ↑ 3 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Replying to jelte:

recursor.cc:113: comment only talks about upstream_, not listen_

Ah, right. Fixed.

do these values really need to be public?

Yes, because they are accessed directly. Creating setters/getters for private implementation class that lives only in one file seems like an overkill. Making it private and adding a friend to the only class that uses it (Recursor) doesn't really sound reasonably too.

Actually, what I use in these situations is nested private struct ‒ noone from outside can reach it, but the owning class has full access to it. But I didn't change it, as it already exists.

Regarding that comment at recursor.c:563: If we try to update the
interfaces we listen on, and it fails, and we try to revert, and
that fails too, we should probably just give up completely.

Done.

We could try to be a bit smarter (figure out if there are
addresses in both old and new, try to add new first, and then
remove the old ones, is one of them, if the adding failed, at least
we'd still be listening on the old ones) but I think giving up is
just as reasonable (and this is i guess also dependent on #384).

Well, I wanted to avoid that. I would have to keep track of what sockets are which address, allow removing them selectively and so on. It doesn't seem worth the effort when the addresses would be configured only once probably and the rollback should work anyway.

For now, apart from the minor issues stated at the beginning of this comment, I think it this code is good. With the note that once we change those key strings as part of #384.

There are 3 more commits on the branch.

Do you think this can be merged and notes added to the #384 and #388 of what needs to be updated then? Or still some problems?

Thank you

comment:5 Changed 9 years ago by vorner

I just accidentally found this:

http://www.boost.org/doc/libs/1_36_0/doc/html/boost_asio/reference/io_service.html#boost_asio.reference.io_service.stopping_the_io_service_from_running_out_of_work

Do you think it would be better to remove the hacks with if(!service.run) and use this instead? It looks cleaner and it will not surprise us when some other place runs out of work.

comment:6 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Oh that work object sounds like exactly what we need. I'll leave it to your discretion whether to add that to this ticket (and do one more review round), or create a new ticket for it.

The rest of the changes are fine. If the work-thingy is going to another ticket, I think this one is ready for merge.

comment:7 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

I changed the runnig out of work fix (r3422).

I also synced with trac327 and tried to solve all conflicts (r3423, r3431, r3432). The actual merge is in r3423, the other two are fixes of code so it compiles and works.

Could you have a look at it? I'd like to merge it before another set of conflicts appear O:-).

Thanks

comment:8 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Looks good, quick, merge it back :)

comment:9 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged, closing.

Note: See TracTickets for help on using tickets.