Opened 9 years ago

Closed 9 years ago

#615 closed task (complete)

custom configurations, ports,

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

Description

To be able to do the many functional and system-level tests, we should be able to run bind10 with different sockets, configurations, port numbers, etc. Using different installation prefix solves part of the problem, but we shouldn't require multiple installations for simultaneous tests.

Currently we can set BIND10_MSGQ_SOCKET_FILE in environment and copy a preconfigured b10-config.db in place and run each part individually.

We need a way to set the port for cmdctl if I am to run bind10 (versus running msgq, cfgmgr, cmdctl -p ...). One alternative could be having that in cfgmgr spec and then just replace the b10-config.db for that.

And we need to be able to choose path for b10-config.db. I can't see option for that. (Some scripts honor B10_FROM_SOURCE and B10_FROM_BUILD but those are wrongly named and not exactly the same.)

Subtickets

Change History (33)

comment:1 Changed 9 years ago by jreed

This is related to #623.

comment:2 Changed 9 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:3 Changed 9 years ago by jreed

It appears #606 duplicates some of this including "various changes to specify the location of config/temporary files so that the can be located under a test directory". It uses a B10_FROM_SOURCE_LOCALSTATEDIR for auth and xfrout "so that we can run multiple instances of bind10 system".
(Please note I didn't read the code yet.)

Please consider that making it so it is not specific to the "source" or build directory, but also may be used from an installation too, such as being able to choose the localstatedir.

comment:4 Changed 9 years ago by jreed

Some more thoughts based on light jabber discussion:

It also needs to know where sockets should be created. There is environment variable for msgq's socket and there does exist a command-line option to set it for msgq socket -- but it doesn't work.
And there is the auth_xfrout_conn socket which doesn't appear to have correct way to override (other than in source build tree or not).

NOTE: the auth_xfrout_conn socket is at different naming and location than msgq_socket
(LOCALSTATEDIR/ versus LOCALSTATEDIR/PACKAGE/) -- so auth_xfrout_conn could be renamed to auth_xfrout_socket and used in LOCALSTATEDIR/PACKAGE/. I have also been told maybe it would be removed.

Logging should have unique identifiers (including maybe PID of xfrout?) if logging to same place, etc.
Currently xfrout is only thing that logs: ... /var/bind10-devel/log/Xfrout.log

(Maybe some of this effort was duplicated in #606 -- but to be clear I want to be able to do this without the source/build tree.)

comment:5 Changed 9 years ago by jreed

Note that cmdctl port configuration is #656.

comment:6 Changed 9 years ago by vorner

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

Hello

I did part of the features needed to complete this, providing a way to specify a configuration file (both to b10-cfgmgr and to bind10). I'd like to get it reviewed, while I work on the other features in parallel, because they are unrelated (at last in the code, even when needed for the same purpose). I'd like to merge this and keep the ticket open for the other things (or, some of them already have separate tickets).

I tried to test whatever possible (eg. parsing of arguments). I didn't do full parsing tests for boss (only of the new options, the old ones didn't get the tests yet, should I add them as well now?). But there are things I have no idea how to test ‒ starting whole bind10 seems wrong, to see if the correct arguments are passed to subprocess. Any ideas how to mock that up?

I propose this changelog entry:

[func]    vorner
It is possible to specify a different directory where we look for configuration
files (by -p) and different configuration file to use (-c).

This code is at trac615, current head is 9dcb53261abe3c16324155fbc4c3436100b9e2ee. For the other tasks I will start new branches.

comment:7 Changed 9 years ago by vorner

Hmm, as nobody taken the review yet, I'm going to put another feature related to this ticket here. It's the ability to specify cmdctl's port on boss's command line. This is not replacement of #656, as this is for single run only, while #656 should be from configuration. But to configure it, it needs to be listening already.

I also put some test code that is shared between boss and cfgmgr to testutils.

The changelog could be updated like this:

func]    vorner
It is possible to specify a different directory where we look for configuration
files (by -p) and different configuration file to use (-c). Also, it is possible
to specify the port on which cmdctl should listen (--cmdctl-port).

comment:8 Changed 9 years ago by jreed

I played with this a little. The -p DATA_PATH, --data-path=DATA_PATH needs to be better explained. What configuration files?

Also when I started second server I got:

[b10-msgq] Closing socket fd 7
[b10-msgq] Receive error: EOF
Traceback (most recent call last):
[b10-msgq] Closing socket fd 12
  File "/home/reed/builder/work/BIND10/20110307141011-NetBSD5-i386/install/libexec/bind10-devel/b10-xfrout", line 591, in <module>
[b10-msgq] Receive error: EOF
    xfrout_server = XfroutServer()
[b10-msgq] Closing socket fd 13
  File "/home/reed/builder/work/BIND10/20110307141011-NetBSD5-i386/install/libexec/bind10-devel/b10-xfrout", line 484, in __init__
[b10-msgq] Receive error: EOF
    self._start_xfr_query_listener()
  File "/home/reed/builder/work/BIND10/20110307141011-NetBSD5-i386/install/libexec/bind10-devel/b10-xfrout", line 491, in _start_xfr_query_listener
    self._cc, self._log);
  File "/home/reed/builder/work/BIND10/20110307141011-NetBSD5-i386/install/libexec/bind10-devel/b10-xfrout", line 296, in __init__
    self._remove_unused_sock_file(sock_file)
  File "/home/reed/builder/work/BIND10/20110307141011-NetBSD5-i386/install/libexec/bind10-devel/b10-xfrout", line 402, in _remove_unused_sock_file
    self._log.log_message("error", "Fail to start xfrout process, unix socket file '%s'"
AttributeError: 'UnixSockServer' object has no attribute '_log'
[b10-msgq] Closing socket fd 8
[bind10] Process b10-xfrout (PID 10953) terminated, exit status = 256
[b10-msgq] Receive error: EOF

comment:9 Changed 9 years ago by jreed

My next problem is:

> config commit
Error: Module 'ConfigManager' not responding
Configuration not committed

That only happens when the other bind10 is running using alternative cmdctl port, configuration, datapath, and msgq. The bindctl has no options and is connected using the bind10 that was started with no options. Once I stopped the custom bind10 and stopped and started the bind10, I could do the commit (I didn't even have to restart bindctl -- it relogged in and already had the commit ready).

comment:10 Changed 9 years ago by jinmei

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

comment:11 follow-up: Changed 9 years ago by jinmei

general comments

It was not clear what if both config-file and data-path are specified
and the former is absolute. I finally found the answer in the pydoc
for cfgmr lib, but it should be nicer if it's also clarified in man
pages.

But I have a more fundamental question: why do we have to separate
data-path and config-file? Unless data-path is used for other
purposes than the path to config-file, it should be much simpler if we
only have config-file (which should include the path). Then we
wouldn't even bother to consider/document the conflicting case. After
reading the entire diff, I'm still not sure how/whether data-path can
be used for a different purpose.

I've also fixed minor editorial issues and directly pushed the change.

bind10.py

  • In BoB.init, maybe a bit more explanation about the two new parameters?
  • start_cfgmr(): maybe "args" is better than "opts"? (this variable is not only for options). same for start_cmdctl.


bind10.xml
s/bind/BIND/? Actually, I was not sure what "bind" means here...can
that also be used for other programs than cfgmgr?

          <para>The path where bind looks for configuration files.</para>

bind10_test.py

  • I'd test missing option parameter cases, too.

b10-cfgmgr.py.in

  • I personally think '-c' is more common for specifying a config file (and it's more consistent with other BIND programs)
  • maybe add space around '+' for readability?
    +                      "(default="+DATA_PATH+")", default=DATA_PATH)
    
  • I'd explicitly define a constant than hardcoding the default in the main code:
                          "(default=b10-config.db)", default="b10-config.db")
    

lib/config/cfgmgr.py

  • I think we should generally cleanup hardcoded "b10-config.db" from this file (now that we have the ability to specify it)
  • not specific to this patch, but this doc should probably be updated so that it doesn't hardcode "b10-config.db":
    class ConfigManager:
        """Creates a configuration manager. The data_path is the path
           to the directory containing the b10-config.db file.
    

lib/.../cfgmgr_test.py

comment:12 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Status changed from accepted to assigned

comment:13 Changed 9 years ago by vorner

  • Status changed from assigned to reviewing

Damn you, trac! Why clicking on „accept“ takes a ticket from review? :-P

(I'm going to look at the comments tomorrow, it's late here).

comment:14 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

It was not clear what if both config-file and data-path are specified
and the former is absolute. I finally found the answer in the pydoc
for cfgmr lib, but it should be nicer if it's also clarified in man
pages.

Should be better now.

But I have a more fundamental question: why do we have to separate
data-path and config-file? Unless data-path is used for other
purposes than the path to config-file, it should be much simpler if we
only have config-file (which should include the path). Then we
wouldn't even bother to consider/document the conflicting case. After
reading the entire diff, I'm still not sure how/whether data-path can
be used for a different purpose.

Well, I guess the original intention with data path would be that there would be all kinds of data ‒ the configuration file, the sqlite database file, some sockets, etc. And then, we could just have several directories for several instances of bind10, so the user doesn't have to specify --config-file, --socket-file, etc, etc…

But it is true that it's used for the configuration file only now. So, I'm open to discussion here.

The relevant changes can be showed by git diff dcaab4f4..4fd1b4c0.

Then I did a sync with master and added the unification of test option parser class we talked about on jabber. The merge did have some trivial collisions, but is uninteresting (5874ea28a). The unification is at 170485ef.

If you'd like to see the diff of whole branch, "git diff master..." (yes, 3 dots) should do it. Also, I discovered git log -p recently and find it handy for reviews sometime.

Thanks

comment:15 in reply to: ↑ 14 ; follow-up: Changed 9 years ago by jinmei

Replying to vorner:

But I have a more fundamental question: why do we have to separate
data-path and config-file?

Well, I guess the original intention with data path would be that
there would be all kinds of data ‒ the configuration file, the
sqlite database file, some sockets, etc. And then, we could just
have several directories for several instances of bind10, so the
user doesn't have to specify --config-file, --socket-file, etc, etc…

I actually wondered whether that might be the intent, and in that case
I have no objection to the idea. I was simply not sure from the
current code.

Other changes generally look okay, with a few minor points:

  • As for --data-path in bind10.xml, so you mean by "BIND" various BIND 10 programs in general (although currently b10-cfgmr is the user)? If so, I'm okay with the concept as I said above, but I'd clarify that more clearly: "The path where BIND 10 programs looks for various data files such configuration files, communication pipes, or log files. Currently, only b10-cfgmr uses the path to find its configuration file, but the usage might be extended for other programs and for other types of files in future".
  • in b10-cfgmgr.py.in, maybe "config_file" is better than "file_name" to store the -c/--config-filename option value to indicate what file it is.
  • in lib/.../cfgmgr.py, there are still some "b10-config.db"s hardcoded.
  • at the risk of being redundant, I'd clarify the relationship between config-filename and data-path in b10-cfgmgr.xml, too.

comment:16 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Other changes generally look okay, with a few minor points:

It should be handled, with the exception of:

  • in lib/.../cfgmgr.py, there are still some "b10-config.db"s hardcoded.

The hardcoded values should be in the default parameters of some functions. It's because they are used by some tests. Should I remove them as well and change the bunch of tests too?

Thanks

comment:18 in reply to: ↑ 17 Changed 9 years ago by jinmei

Replying to vorner:

Other changes generally look okay, with a few minor points:

It should be handled, with the exception of:

  • in lib/.../cfgmgr.py, there are still some "b10-config.db"s hardcoded.

The hardcoded values should be in the default parameters of some functions. It's because they are used by some tests. Should I remove them as well and change the bunch of tests too?

I prefer getting rid of the hardcoded path from the library, but I'm
okay with deferring it to a separate (new) ticket.

Other changes are okay.

comment:19 follow-up: Changed 9 years ago by vorner

There wasn't problem I would consider it too much work, it's not that much, I just did that. I wasn't sure if we want to do it, when the tests use it.

So, is this OK as well?

Thanks

comment:20 in reply to: ↑ 19 Changed 9 years ago by jinmei

Replying to vorner:

There wasn't problem I would consider it too much work, it's not that much, I just did that. I wasn't sure if we want to do it, when the tests use it.

Whan a sensible change requests an update to some tests, IMO we should
change the tests as well as the main code. So the question is the
change to the main code is sensible, and in this case I thought so.
If you were okay with the code change itself, I believe it should be
okay to update the test.

I also confirmed the default file name is tested in the daemon test,
which is good (we've not lost test cases for checking whether we use
the correct default).

So, is this OK as well?

Yes. I'm now okay to merge it.

comment:21 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:22 Changed 9 years ago by jreed

I tested this. I think it is okay to merge this. I did have some tracebacks that I have other tickets for but I think they are unrelated to this directly. I will open different tickets related to my feedback on this specific ticket but for now we should merge the basic functionality.

comment:23 Changed 9 years ago by jreed

Note that this makes it easy to reuse same msgq socket for different unrelated bind10 instances. For example, I had two different b10-msgq running but only one socket file -- and I had 44 file handles for that same file. I did have some msgq crashes. See tickets: #681 and #691

comment:24 Changed 9 years ago by jreed

Again I want to make my goal clear for this ticket: I want to easily be able to use same installation prefix to run multiple bind10 instances; for example run one with resolver on some ports and then auth on different ports (or interfaces) and use bindctl to configure, etc. This would make it more useful for various simultaneous tests.

comment:25 Changed 9 years ago by vorner

Thanks, merged.

I'm aware it doesn't achieve the full goal of the ticket, only part of the path there and I'm keeping the ticket open. I just wanted to have the changes merged ASAP instead of waiting for the branch to diverge too much. I'll have a look at more of the problems there soon.

comment:26 Changed 9 years ago by vorner

  • Status changed from reviewing to accepted

Maybe I should take it out of the review again now.

comment:27 Changed 9 years ago by stephen

  • Milestone A-Team-Task-Backlog deleted

Milestone A-Team-Task-Backlog deleted

comment:28 Changed 9 years ago by vorner

  • Milestone set to Sprint-20110405

comment:29 Changed 9 years ago by vorner

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

I added BIND10_XFROUT_SOCKET_FILE environment variable, to specify which socket to use for communication between auth and xfrout. I did run two instances without getting any errors (but I didn't know how to check if the xfrout works). Is there anything else that does not work yet?

I didn't add a command line option for boss for it, for two reasons:

  • I believe that users who want to run two different instances both with xfrout are rare.
  • I hope that if we get a msgq that can send file descriptors, we won't need the socket any longer.

Do we want a changelog for such nicely hidden feature?

[func]     vorner
The BIND10_XFROUT_SOCKET_FILE environment variable can be used to specify which socket should be used
for communication between b10-auth and b10-xfrout. Mostly for testing reasons.

comment:30 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to vorner

Is it intentional that that new env var is only used when B10_FROM_BUILD is not set? (one could imagine it could also come in handy if it is). If so, these changes by themselves are fine.

IF it is set, and we are not in a testing environment, communication between xfrout and b10-auth will fail now, so we either need some very big warnings to never set it unless you are testing xfrout, or also use the var in bin/auth.

On a more general note, we have a file src/lib/python/bind10_config.py[.in] for configure-time substitions in python, I think we should move all occurrences (including the ones here) to that file. Perhaps we should even move the whole init_paths() there, so that we can make a more general one, but just moving the substitutions there would also be fine for me, so that we don't need to generate .py files anymore (well, except for that one, of course).

We could make a bigger (but quite straightforward) task to do all of the occurrences for that at once, or do this one in this ticket and do the rest as they come along. In the case of the first, I think this change is fine.

comment:31 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Yes, it's intentional that all the B10_FROM_BUILD and such have a precedence. I could turn it over if someone wanted.

What do you mean that it'll fail? If the variable is set to the same value for both auth and xfrout, it should work. Or do you mean something else?

I didn't know about this file, but I think there are really many substitutions all over the place. So, should I open a cleanup ticket for them?

comment:32 in reply to: ↑ 31 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Yes, it's intentional that all the B10_FROM_BUILD and such have a precedence. I could turn it over if someone wanted.

OK, no problem, I jsut wasn't sure :)

What do you mean that it'll fail? If the variable is set to the same value for both auth and xfrout, it should work. Or do you mean something else?

My bad, I somehow got my checkouts mixed up, and though auth didn't use the value, I got
[b10-auth] Error in handling XFR request: socket connect failed: No such file or directory
But trying it again it does indeed work, sorry.

I didn't know about this file, but I think there are really many substitutions all over the place. So, should I open a cleanup ticket for them?

Done (#841)

This can be merged

comment:33 Changed 9 years ago by vorner

  • Estimated Difficulty changed from 0.0 to 13
  • Resolution set to complete
  • Status changed from reviewing to closed

Ok, thanks, closing.

Note: See TracTickets for help on using tickets.