BIND 10 #2856: memory manager initialization
BIND 10 Development
do-not-reply at isc.org
Thu Jul 25 12:39:48 UTC 2013
#2856: memory manager initialization
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | vorner
Priority: medium | Status:
Component: shmem | reviewing
manager | Milestone:
Keywords: | Sprint-20130806
Sensitive: 0 | Resolution:
Sub-Project: DNS | CVSS Scoring:
Estimated Difficulty: 4 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| shared memory data source
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by muks):
* owner: muks => vorner
Comment:
Hi Michal
Replying to [comment:13 vorner]:
> Hello
>
> Replying to [comment:12 muks]:
> > > Also, the semantics of the `SegmentInfo` class seems strange to me.
It
> > > looks like a lot of different methods return the event to be
> > > handled. So the caller will have to check the return value in like 5
> > > different places.
> >
> > I didn't follow the last sentence.
>
> These methods:
> * complete_update()
> * sync_reader()
> * start_update()
> * remove_reader()
>
> Whenever they figure out a state change is needed, they return it as
> their result and pop from the queue. Or they return None.
>
> The caller then needs to check the return value of each of these
> methods, but probably handle it the same way. It doesn't sound like
> convenient API of the class.
>
> I know it is described in the ticket this way, though.
I guess we'll know better how to handle this once we start using these
methods in memmgr.
> > > There's a class that does a queue effectively in python (sorry, I
> > > don't remember the name now). When using list, it needs to copy all
> > > the items to the left when removed from the head.
> >
> > I am not able to find such a class. If you remember the name, please
let
> > me know. The `queue.Queue` class is a producer-consumer queue.
>
> collections.deque
The code has been updated to use the deque now.
> > > I'm little bit worried about the case when there's no update and no
> > > reader either. It seems it'll get to the `SYNCHRONIZING` state. But
as
> > > there's no reader, no reader will ever move from the old readers, so
> > > nothing will trigger the check for the emptiness of that set. It
seems
> > > like the manager will get stuck in that state forever.
> > > {{{#!python
> > > def complete_update(self):
> > > """This method should be called when memmgr is notified by
the
> > > builder of the completion of segment update. It changes the
> > > state from UPDATING to SYNCHRONIZING, and COPYING to READY.
In
> > > the former case, set of reader modules that are using the
> > > "current" reader version of the segment are moved to the set
> > > that are using an "old" version of segment. If there are no
such
> > > readers using the "old" version of segment, it pops the head
> > > (oldest) event from the pending events queue and returns it.
It
> > > is an error if this method is called in other states than
> > > UPDATING and COPYING."""
> > > if self.__state == self.UPDATING:
> > > self.__state = self.SYNCHRONIZING
> > > self.__old_readers.update(self.__readers)
> > > self.__readers.clear()
> > > return self.__sync_reader_helper(self.SYNCHRONIZING)
> > > elif self.__state == self.COPYING:
> > > self.__state = self.READY
> > > return None
> > > else:
> > > raise SegmentInfoError('complete_update() called in ' +
> > > 'incorrect state: ' +
str(self.__state))
> > > }}}
> >
> > What do you suggest we do for it?
>
> Skip the SYNCHRONIZING state directly?
This is now implemented. Please check the testcase too.
> > > Why is the switching of `__readers` and `__old_readers` so
> > > complicated? Won't this be simpler and also faster?
> > > {{{#!python
> > > self.__old_readers = self.__readers
> > > self.__readers = set()
> > > }}}
> >
> > The current code performs a set union. I'm not sure if the assignment
> > does the same. Does it?
>
> No, it overwrites. But, at the moment when it happens, isn't the target
empty?
You're right. I've updated this code to make it an assignment.
> > > Why can't `remove_reader` happen any time? I don't see a reason why
> > > the auth server couldn't terminate or crash at any time it finds
> > > appropriate. Will the manager be required to buffer the
> > > unsubscriptions?
> >
> > Hmmm... shall we allow this then? I implemented the description to the
> > letter, but didn't consider the bigger picture in this particular
> > ticket.
>
> I think we should allow it.
Reading this again, I wonder if memmgr is supposed to populate and
maintain readers (from the group) only after `start_update()` is issued?
And then it stops maintaining this list after it gets back to `READY`
state. Otherwise I don't follow why it is designed this way.
> > > Looking at this, it makes me wonder if it makes sense to test
anything
> > > at all on the memory manager if the shared memory is not
> > > available. Would it make sense to completely exclude the memory
> > > manager from build then?
> > > {{{#!python
> > > @unittest.skipIf(os.environ['HAVE_SHARED_MEMORY'] != 'yes',
> > > 'shared memory is not available')
> > > }}}
> >
> > I think we still allow local segments to the memory manager. There is
> > another ticket about it, but this is something we can do afterwards.
>
> Well, we didn't manage to get rid of local segments yet. But the
> usefulness of memmgr without the shared memory is just completely
> zero, because the local segments are just local in there.
I agree memmgr doesn't make any sense without shared memory, and we can
install it conditionally. Given the other memmgr tickets, would auth,
etc. work properly without memmgr running? We should keep this in mind
as we complete the shared memory work.
Shall I make another ticket for it, that conditionally installs memmgr?
> > > Is it really allowed to call `start_update` any time?
> >
> > I thought it was weird too, but thought maybe I misunderstood the
> > description. Shall I update it?
>
> I don't know. I don't have completely clear and detailed view of how
> the rest will look like, but I think the operation makes no sense in
> many contexts, so we probably should disallow it then.
This has been updated as suggested.
> > Did all the documentation look OK to you?
>
> I think it looked OK.
>
> Also, the tests fail for me:
>
> {{{
> EE.E
> ======================================================================
> ERROR: test_configure (__main__.TestMemmgr)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 120, in test_configure
> parse_answer(self.__mgr._config_handler({})))
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 76, in _config_handler
> logger.debug(logger.DBGLVL_TRACE_BASIC, MEMMGR_CONFIG_UPDATE)
> NameError: global name 'MEMMGR_CONFIG_UPDATE' is not defined
>
> ======================================================================
> ERROR: test_datasrc_config_handler (__main__.TestMemmgr)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 178, in test_datasrc_config_handler
> self.__mgr._datasrc_config_handler({}, cfg_data)
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 208, in _datasrc_config_handler
> logger.info(MEMMGR_DATASRC_RECONFIGURED, genid)
> NameError: global name 'MEMMGR_DATASRC_RECONFIGURED' is not defined
>
> ======================================================================
> ERROR: test_setup_module (__main__.TestMemmgr)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 181, in _setup_module
> 'data_sources', self._datasrc_config_handler)
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 41, in add_remote_config_by_name
> raise self.add_remote_exception
> isc.config.ccsession.ModuleCCSessionError: faked exception
>
> During handling of the above exception, another exception occurred:
>
> Traceback (most recent call last):
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/../src/bin/memmgr/tests/memmgr_test.py",
line 163, in test_setup_module
> self.__mgr._setup_module)
> File "/usr/lib64/python3.3/unittest/case.py", line 570, in
assertRaises
> return context.handle('assertRaises', callableObj, args, kwargs)
> File "/usr/lib64/python3.3/unittest/case.py", line 135, in handle
> callable_obj(*args, **kwargs)
> File
"/tmp/bind10/bind10-1/bind10-20130529/_build/src/bin/memmgr/memmgr.py",
line 183, in _setup_module
> logger.error(MEMMGR_NO_DATASRC_CONF, ex)
> NameError: global name 'MEMMGR_NO_DATASRC_CONF' is not defined
>
> ----------------------------------------------------------------------
> }}}
This was due to a library `.mes` file being named the same as another in
`src/bin/`. It has been fixed now.
> And, the copyright year in `isc/memmgr/logger.py` is wrong.
Also fixed.
--
Ticket URL: <http://bind10.isc.org/ticket/2856#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list