Opened 8 years ago

Closed 7 years ago

#1901 closed defect (fixed)

be consistent with bind10 or boss or bob name

Reported by: jreed Owned by: jelte
Priority: medium Milestone: Sprint-20130205
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 9 Add Hours to Ticket: 0
Total Hours: 4.66 Internal?: no

Description

1) The bind10 process log message identifier is "BIND10". That is consistent.

2) The name of the bind10 logger is "b10-boss". Note that the logger API documentation says "This should be set to the name of the program." It is not.

3) The child logger name is also "boss" (i.e. [b10-boss.boss]).

4) The bind10 specification is named bob.spec.

5) The bob.spec defines module name as "Boss".

6) The bob.spec command descriptions use a mix of "BIND 10" and "boss".

7) The code has a BoB class.

8) Some stats collected for bind10 are identified with "Boss" owner.

9) Control channel is identified as "boss".

10) API docs and code use "boss" term.

11) The bind10 log messages uses "boss" many times including once as part of a bind10 message identifier.

The "boss" or "bob" name is not based on process name and may be confusing to users.

We need to choose a name (b10-boss? bind10? bob? b10-bob? b10-bind10?)

Whatever we decide should be consistently used in logging, documentation, configurations, etc.

(jinmei and I discussed this a little in jabber.)

Subtickets

Change History (18)

comment:1 Changed 8 years ago by shane

  • Defect Severity changed from N/A to Medium

We can't use "bind10" as the module name, because frankly that is confusing. It was called "bob" to indicate the "boss... of BIND". I don't have any strong preference between "bob" or "boss", although I suspect that "boss" is the right answer.

The internal code is lesser importance, but consistency between process naming and logging and the like makes sense.

comment:2 Changed 8 years ago by shane

  • Milestone New Tasks deleted

comment:3 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:5 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by jelte

  • Milestone set to Sprint-20130205

comment:7 Changed 7 years ago by shane

Based on the outcome of a bind10-dev discussion, the result is:

  • We will call the process "b10-init".
  • This process will live in the same directory as the other binaries $(libexecdir)/@PACKAGE@
  • We will create a "bind10" script which will live in the sbin directory which will run that executable.

Mailing list thread starts here:

https://lists.isc.org/pipermail/bind10-dev/2013-January/004298.html

comment:8 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:9 Changed 7 years ago by jelte

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

Ok, I guess this is ready for review, due to all the references and uses of bob, Bob, BoB, boss, Boss, BOSS, bind10_src, etc. the diff has gotten insanely large, but most of it is advanced search&replace.

I recommend the reviewer takes it in chunks;

  • first commit (cfa26ab8df9b533d32b8dd5aa405bf237bb58f12) is the majority of file renaming (barring a few instances which I forgot at the time). This one breaks everything as internally nothing is updated
  • Then commits 826ac1b1e637a6aa8b5763c4b810755ac0551446 to 6aa1c3cc7dbb55d32304ada9b88037346ba929b0 are the replacements of code uses.
    • in general it's something like bind10_src -> b10-init, Boss and Bob to Init, local variable names such as bob and boss to b10_init
    • made a small import workaround since python doesn't like hyphens in code and we import b10-init in a number of tests
    • Left alone the 'bind10' directory name, and the BIND10_ log message prefix.
  • commit 476b3eff5488d89b03d7ac34830ad52973e9b0bd updates the config file version to 3, and automatically replaces Boss with Init in existing configurations. This is a one-way deal which we need to mention in the changelog
  • final few commits are some small cleanups (the bind10 script shane proposed doesn't work when installed as b10-init is in libexec, so I just take it from PATH, not sure if that is the correct way though)

Proposed changelog:
[func]* jelte
The main initializer script (formerly known as either 'bind10', 'boss', or 'bob'), has been renamed to b10-init (and Init in configuration). Configuring which components are run is henceforth done through '/Init/components', and the sbin/bind10 script is now simply a shellscript that runs b10-init. Existing configuration is automatically updated. NOTE: once configuration with this update has been saved (by committing any new change with bindctl), you cannot run older versions of BIND 10 anymore with this configuration.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

First, I have to admit that my check in review was not fully
word-by-word, so I may have missed something. But I think we don't
have to be perfect in this attempt, as long as it generally works
correctly.

general/others

  • distcheck failed for me. please check.
  • regarding the path to b10-init (under libexec), I think we should hardcode the path (autogenerated by ./configure from a .in file). both for the "bind10" run_bind10 scripts.
  • maybe we need a man page for the top level "bind10" script?
  • I assume in cfa26ab8df9b533d32b8dd5aa405bf237bb58f12 you only changed file name (with trivial Makefile.am fixups) without modifying the contents.
  • Is it a good idea to replace "Boss" in ChangeLog entries? I'd personally leave them as they were historical notes. besides, if we want to change ChangeLog, it's incomplete: there are still some occurrences of "boss" or "Bob" (and maybe "bind10" that would have to be replaced with "init")
  • in bind10/README, is the phrase of "Init of Bind" intentional?
    This directory contains the source for the "Init of Bind" program.
    
    Same comment applies to b10-init.py. Maybe okay, but I guess we need to define this term somewhere. We might also note that in doc etc we use other terms like just "init/Init" or "b10-init" interchangeably.
  • b10-init.xml: "init" should be capitalized?
    -      The <varname>Boss</varname> configuration commands are:
    +      The <varname>init</varname> configuration commands are:
    
  • init.spec: not really for the topic of this task, maybe rephrase "Master process", too?
    -    "module_name": "Boss",
    +    "module_name": "Init",
         "module_description": "Master process",
    
  • init_message.mes: is the "IN" garbage?
    -# Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
    +IN# Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
    
  • can't we name the Python source file just "init.py" and convert it to b10-init, just like xfrin, ddns, etc? Then we won't need the trick like that in bind10_test.py. (btw, 'bind10_test.py' may also have to be renamed)
  • bind10_test.py.in: not for this branch, but s/Hijact/Hijack/?
    -        Hijact the accept method of the boss.
    +        Hijact the accept method of the b10-init.
    

bindctl_main.py.in

  • Not for this branch, but I'd change 'Recurse' to 'Auth'. At least it shouldn't be 'Recurse' (which doesn't exist):
    DEFAULT_IDENTIFIER_DESC = "The identifier specifies the config item. Child elements are separated with the '/' character. List indices can be specified with '[i]', where i is an integer specifying the index, starting with 0. Examples: 'Init/start_auth', 'Recurse/listen_on[0]/address'. If no identifier is given, shows the item at the current location."
    

b10-ddns.xml

  • it's probably better to be "with the b10-init process".
    -            compatibility with the <command>bind10</command> Boss process.
    +            compatibility with the <command>bind10</command> Init process.
    

b10-stats-httpd.xml

  • remove "(<command>bind10</command>)"?
    -      by the BIND 10 boss process (<command>bind10</command>) and eventually
    +      by the b10-init process (<command>bind10</command>) and eventually
    

cfgmgr.py

  • I'm not sure how likely a config has both Boss and Init, but isn't it better to warn about it (in a log) if that happens?

lettuce tests

  • Shouldn't we update the version of the configurations? e.g. lettuce/configurations/bindctl/bindctl.config has this:
        "version": 2,
    

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:13 in reply to: ↑ 11 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

First, I have to admit that my check in review was not fully
word-by-word, so I may have missed something. But I think we don't
have to be perfect in this attempt, as long as it generally works
correctly.

ack.

general/others

  • distcheck failed for me. please check.

doh, needed to add to EXTRA_DIST

  • regarding the path to b10-init (under libexec), I think we should hardcode the path (autogenerated by ./configure from a .in file). both for the "bind10" run_bind10 scripts.

ack for 'bind10' itself, but I disagree about run_bind10, since it's purpose is to run from the source tree and not prefix/libexec :)

updated

  • maybe we need a man page for the top level "bind10" script?

added, but is is rather simple :p

  • I assume in cfa26ab8df9b533d32b8dd5aa405bf237bb58f12 you only changed file name (with trivial Makefile.am fixups) without modifying the contents.

yes

  • Is it a good idea to replace "Boss" in ChangeLog entries? I'd personally leave them as they were historical notes. besides, if we want to change ChangeLog, it's incomplete: there are still some occurrences of "boss" or "Bob" (and maybe "bind10" that would have to be replaced with "init")

No it is not, I wasn't sure about it, but had decided over the weekend that I should revert this :)

  • in bind10/README, is the phrase of "Init of Bind" intentional?
    This directory contains the source for the "Init of Bind" program.
    
    Same comment applies to b10-init.py. Maybe okay, but I guess we need to define this term somewhere. We might also note that in doc etc we use other terms like just "init/Init" or "b10-init" interchangeably.

Yeah that first one was wrong (there were bound to be a few of those with this many replacements :P)

But in general, I do indeed make a subtle distinction between init/Init and b10-init; I use the former one usually when talking about the class, or the abstract context of what 'the thing' does, while the latter tends to refer to the actual program, but I guess not in all contexts it is clear which one would be best to use. Not sure where we would canonically define this though.

  • b10-init.xml: "init" should be capitalized?
    -      The <varname>Boss</varname> configuration commands are:
    +      The <varname>init</varname> configuration commands are:
    

ack.

  • init.spec: not really for the topic of this task, maybe rephrase "Master process", too?
    -    "module_name": "Boss",
    +    "module_name": "Init",
         "module_description": "Master process",
    

changed to Init, but wondering whether this two-word description is useful in its current form :)

  • init_message.mes: is the "IN" garbage?
    -# Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
    +IN# Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
    

whoa. Yes. Interesting how that doesn't make things go boom...

  • can't we name the Python source file just "init.py" and convert it to b10-init, just like xfrin, ddns, etc? Then we won't need the trick like that in bind10_test.py. (btw, 'bind10_test.py' may also have to be renamed)

underscores are fine if that is what you meant

changed to do the same thing. I also updated a few more tests as it got a bit grumpy about local variables and modules (so the module is 'init' and vars are 'b10_init')

  • bind10_test.py.in: not for this branch, but s/Hijact/Hijack/?
    -        Hijact the accept method of the boss.
    +        Hijact the accept method of the b10-init.
    

while not for this branch per se, i see no reason not to fix these now, so done

bindctl_main.py.in

  • Not for this branch, but I'd change 'Recurse' to 'Auth'. At least it shouldn't be 'Recurse' (which doesn't exist):
    DEFAULT_IDENTIFIER_DESC = "The identifier specifies the config item. Child elements are separated with the '/' character. List indices can be specified with '[i]', where i is an integer specifying the index, starting with 0. Examples: 'Init/start_auth', 'Recurse/listen_on[0]/address'. If no identifier is given, shows the item at the current location."
    

changed

b10-ddns.xml

  • it's probably better to be "with the b10-init process".
    -            compatibility with the <command>bind10</command> Boss process.
    +            compatibility with the <command>bind10</command> Init process.
    

fixed. This one occurred in more xmls which i have also updated (<command>bind10</command> which should have been <command>b10-init</command>)

b10-stats-httpd.xml

  • remove "(<command>bind10</command>)"?
    -      by the BIND 10 boss process (<command>bind10</command>) and eventually
    +      by the b10-init process (<command>bind10</command>) and eventually
    

done.

cfgmgr.py

  • I'm not sure how likely a config has both Boss and Init, but isn't it better to warn about it (in a log) if that happens?

unlikely, and only if some external person or tool tried to be smart. Added a log message explaining what it does and why

lettuce tests

  • Shouldn't we update the version of the configurations? e.g. lettuce/configurations/bindctl/bindctl.config has this:
        "version": 2,
    

ohyeah :) done

comment:14 follow-up: Changed 7 years ago by jinmei

The code looks okay, but I found one issue: run_bind10.sh doesn't
work:

% ./run_bind10.sh
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-1901/src/bin/xfrin/b10-xfrin", line 36, in <module>
    from isc.server_common.auth_command import auth_loadzone_command
  File "/Users/jinmei/src/isc/git/bind10-1901/src/lib/python/isc/server_common/auth_command.py", line 21, in <module>
    from isc.log_messages.server_common_messages import *
EOFError: EOF read where not expected
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-1901/src/bin/xfrout/b10-xfrout", line 37, in <module>
    import isc.server_common.tsig_keyring
  File "/Users/jinmei/src/isc/git/bind10-1901/src/lib/python/isc/server_common/tsig_keyring.py", line 24, in <module>
    from isc.log_messages.server_common_messages import *
EOFError: EOF read where not expected
...(errors like these continue)

Is it only me?

And, I just notice that lettuce tests also failed.

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:16 in reply to: ↑ 14 Changed 7 years ago by jinmei

Replying to jinmei:

The code looks okay, but I found one issue: run_bind10.sh doesn't
work:

And, I just notice that lettuce tests also failed.

These turn out to be due to some intermittent garbage in my working
directory. These were resolved by rebuilding everything from
distclean.

So, the branch is okay for merge...but, just one last thing: now
b10-init is not so different from other b10-xxx programs, maybe
we should rename it its simplified form:

# Assign this process some longer name
isc.util.process.rename(sys.argv[0])

i.e., remove sys.argv[0]. But I'd leave it to you, and either way I
don't think I need to review it.

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 4.66

comment:18 Changed 7 years ago by jelte

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

Thanks! Merged, closing ticket.

Note: See TracTickets for help on using tickets.