BIND 10 #2854: memory manager framework

BIND 10 Development do-not-reply at isc.org
Mon Jun 24 06:16:20 UTC 2013


#2854: memory manager framework
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  shmem         |  reviewing
  manager                            |                    Milestone:
            Keywords:                |  Sprint-20130625
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  9             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.  I've addressed specific code comments, leaving
 some higher level discussions (generation ID and load command) open.
 I'll be offline for a while, so I'm giving the ticket back to you at
 this point.

 Replying to [comment:9 vorner]:

 > I think a changelog would be nice, even if the daemon is not of any use.
 The user might get interested in the fact that a new program exists, so we
 should say he should leave it alone. Also, we should update the component
 status wiki page with the new daemon.
 >
 > The tests fail for me:
 > {{{
 > Running test: datasrc_info_tests.py
 > ======================================================================
 > ERROR: test_production (__main__.TestDataSrcInfo)
 > Check the behavior closer to a production environment.
 > ----------------------------------------------------------------------
 > Traceback (most recent call last):
 >   File
 "/tmp/bind10/bind10-2/bind10-20130529/_build/../src/lib/python/isc/memmgr/tests/datasrc_info_tests.py",
 line 171, in test_production
 >     cmgr.reconfigure(datasrc_config)
 > TypeError: reconfigure() missing 1 required positional argument:
 'config_data'
 >
 > ----------------------------------------------------------------------
 > Ran 6 tests in 0.001s
 > }}}

 Oops, sorry.  I'm not sure how I left it.  It should now be fixed.

 > And, there's bunch of smaller things.
 >
 > Are these tabs?
 > {{{
 >                   src/bin/loadzone/tests/correct/Makefile
 > +              src/bin/memmgr/Makefile
 > +              src/bin/memmgr/tests/Makefile
 >                   src/bin/msgq/Makefile
 > }}}

 Right, untabified.

 > These two messages look like saying a very similar thing both. Is one a
 never version of the other?
 > {{{
 > $(man_MANS):
 >       @echo Man generation disabled.  Creating dummy $@.  Configure with
 --enable-generate-docs to enable it.
 >       @echo Man generation disabled.  Remove this file, configure with
 --enable-generate-docs, and rebuild BIND 10 > $@
 >
 > endif
 > }}}

 I see the point, but it seems to be derived from existing Makefile
 (probably for ddns).  And I'm not sure how it should be fixed - I
 believe it was originally written by Jeremy.  So my suggestion is to
 create a separate cleanup ticket for all these rules and give it to
 him.

 > This date seems outdated. What does it mean?
 > {{{
 >   <refentryinfo>
 >     <date>June 11, 2012</date>
 >   </refentryinfo>
 > }}}

 Looks like I forgot to update the year after a copy-paste-edit.  Fixed.

 > The word „sharable“ seems strange here. It sounds like something that
 can be shared, but commonly isn't. We probably want to say „shared“
 (because it usually would be shared, cases when it is created and not read
 by auth would be rare and usually either temporary or configuration
 error).
 > {{{
 >     <para>The <command>b10-memmgr</command> daemon manages sharable
 >       memory segments storing in-memory DNS zone data, and
 >       communicates with other BIND 10 modules about the segment
 >       information so the entire system will use these segments
 >       in a consistent manner.
 > }}}

 Okay, I've changed it to "shared".

 > Can this be removed completely, instead of commenting it out (in
 memmgr.py.in):
 > {{{
 > #from isc.log import DBGLVL_TRACE_BASIC
 > }}}

 Ack, removed.

 > Will this load all the local memory segments to memory, or do I confuse
 this `use_cache` with some other?
 > {{{#!python
 > DataSrcClientsMgr(use_cache=True)
 > }}}

 Hmm, I didn't think about the implication, but I believe (a copy of)
 local segments will be loaded for memmgr.  And I guess you meant it
 might be a waste and should be avoided - if so, I see the point, but
 I think it can be at least deferred in practice; if the amount of
 memory (and loading overhead itself) is substantial (and as long as
 memmgr is used), it would better be shared anyway.  If it's not that
 big, that's a waste but marginal.  So I suggest just creating a
 ticket, describing the issue, and see if we want to solve it later.

 > Can it happen this is not configured? Don't we have some default? I'm
 not saying we should not check for the error condition, but the
 description sounds like the user forgot to configure something, while it
 is probably much deeper problem.
 > {{{
 > % MEMMGR_NO_DATASRC_CONF failed to add data source configuration: %1
 > The memmgr daemon tried to incorporate data source configuration
 > on its startup but failed to do so.  The most likely cause of this
 > is that data sources are not simply configured.  If so, they must be.
 > The memmgr daemon cannot do any meaningful work without data sources,
 > so it immediately terminates itself.
 > }}}

 Okay, I've revised the description so it'll hopefully make more sense.

 > This bit of code looks somewhat strange. What is the reason for it?
 > {{{#!python
 >         except Exception:
 >             raise
 > }}}

 Ah, I guess (I forgot what exactly I was thinking at that time) I
 thought we need an explicit re-raise to perform the `finally` block
 and still re-raise the exception.  But we actually don't need it,
 so I removed it.

 > Should the `o` be there in `0o500`? Is it some strange python syntax for
 octal?
 > {{{#!python
 >         os.mkdir(self.__test_mapped_file_dir, 0o500) # drop writable bit
 > }}}

 Yes.  I don't know if there's a less ugly form to express octal
 numbers in Python than that.

 > This looks like twice the same code.
 > {{{#!python
 >         # If data source isn't configured it's considered fatal.
 >         self.__mgr.mod_ccsession.add_remote_exception = \
 >             isc.config.ModuleCCSessionError('faked exception')
 >
 self.assertRaises(isc.server_common.bind10_server.BIND10ServerFatal,
 >                           self.__mgr._setup_module)
 >
 >         self.__mgr.mod_ccsession.add_remote_exception = \
 >             isc.config.ModuleSpecError('faked exception')
 >
 self.assertRaises(isc.server_common.bind10_server.BIND10ServerFatal,
 >                           self.__mgr._setup_module)
 > }}}

 Not really, but I agree it might look so.  I updated the comment so it
 can help understand the difference.

 > What is the motivation to returning the JSON _string_ here? I know there
 are cases when we need to pass the string (usually to the C++ wrappers),
 but even from the description, we expect there'll be cases when we pass it
 as structures (when sending as the command parameters) and it is handled
 as structures in the tests. So, does it make sense to convert to string
 and then back?
 > {{{
 > It returns a json expression in string that contains parameters for
 > the specified type of user to reset a zone table segment with
 > }}}

 I guess (again I forgot what exactly I was thinking) I thought we want
 a string form so we can pass it to
 `ConfigurableClientList.reset_memory_segment()`.

 > Is the xor really the easiest way to write it? I'm not pushing it, but I
 find `1 - __writer_ver` more readable version for me:
 > {{{#!python
 > self.__writer_ver ^= 1
 > }}}

 Hmm, I do not necessarily think `x = 1 - x` is obviously better, but
 I'm not opposed to that either.  Changed it as suggested.

 > I believe this is wrong approach, since the test will show up as
 „passed“ on system without shared memory. We should use the decorator to
 skip the test (I think it is `@skip-if`), as it shows correctly the test
 is skipped.

 Assuming you meant the check for test_production in
 datasrc_info_tests, I've updated it.

 > Does a general server receive updates?
 > {{{#!python
 > % PYSERVER_COMMON_SERVER_STARTED %1 server has started
 > The server process has successfully started and is now ready to receive
 > commands and updates.
 > }}}

 Not in the bind10_server module, but in the user class of that mixin.
 I think the log description is okay as the observable behavior for end
 users should be the same.

 > In the `test_select_interrupted`, why doesn't the second call to select
 cause call to the `check_command`?

 Because select_wrapper returns empty lists (meaning there's no
 readable fileno) by default.  I clarified it in comments.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2854#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development


More information about the bind10-tickets mailing list