Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1288 closed task (complete)

Update XFROUT to use new interface

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111122
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket:
Total Hours: 21.08 Internal?: no

Description

This task is intended to focus on AXFR. It will depend on #1287.

Subtickets

Change History (28)

comment:1 Changed 8 years ago by jinmei

  • Component changed from data source to xfrout

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111025

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:4 Changed 8 years ago by vorner

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

comment:5 Changed 8 years ago by vorner

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

Putting it back, I didn't notice we're missing the #1287 in sprint. Taking that one instead.

comment:6 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:7 Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:9 Changed 8 years ago by jinmei

trac1288 is ready for review.

The branch resulted in a bit bigger than I expected, partly because
I didn't realize I would need to updte notify_out, too. Here are some
hints for the reviewer that hopefully mitigates the review load:

  • changes to xfrout and notify_out are completely independent. So, if the entire branch is deemed to be too big, we can separate these into separate (conceptual or real) sub tasks and review them separately.
  • commit 7a59033 is also independent from others, but is necessary for the change to work in an installed environment.
  • main changes for the topic of this task are marked in the commit logs as "switch to new API'. Others are basically related cleanups or small bug fixes piggy-backed on this ticket.
  • commit ca42fb6 is a bug fix, and it's not directly specific to this branch (this problem should exist in the current implementation, too), but using a generic datasource API may increase the possibility of getting exceptions unexpectedly, which will increase the possibility of this bug. So I decided to fix it in this branch. Again, if the branch seems to be too big, we can defer this part to a separate branch or task.
  • commit f01fb1d is an improvement on top of the changes. I believe this is useful and thought it was a good opportunity to make it happen, but once again this can be deferred.

comment:10 Changed 8 years ago by jinmei

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

comment:11 Changed 8 years ago by jinmei

And one more thing: Unfortunately we cannot completely switch to
the new API yet because notify_out uses the old API to get a list
of all available zones. To replace this part we need #1374.
(The current code logic seems to have a flaw, however, in that
it gets this list only at the initialization. But that's way
beyond the scope of this ticket anyway).

Nevertheless, there's no reason for holding off merging the currently
available changes (and we'll need it for xfrin to install diffs pretty
soon).

comment:12 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:13 follow-up: Changed 8 years ago by jelte

BTW, #1217 has been merged, so we should either add True to the call to get_iterator() (and update the Mock class) when merging this, or create a (very minor) ticket to do so, so that TTLs are kept upon xfrout.

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

The branch resulted in a bit bigger than I expected, partly because
I didn't realize I would need to updte notify_out, too. Here are some
hints for the reviewer that hopefully mitigates the review load:

That's OK, I've seen worse O:-).

Replying to jelte:

BTW, #1217 has been merged, so we should either add True to the call to get_iterator() (and update the Mock class) when merging this, or create a (very minor) ticket to do so, so that TTLs are kept upon xfrout.

I believe this might be better to do now, as it is really small. Creating a ticket would be more work than the actual coding ;-).

So, about the review:

  • For some reason I did not pursue, the tests segfault (yes, in python):
    TESTDATASRCDIR=/home/vorner/work/bind10/src/lib/python/isc/notify/tests/testdata/ \
    /usr/bin/python3 /home/vorner/work/bind10/src/lib/python/isc/notify/tests/$pytest || exit ; \
    done
    Running test: notify_out_test.py
    E/bin/sh: line 1: 24806 Segmentation fault      (core dumped) PYTHONPATH=/home/vorner/work/bind10/src/lib/python/isc/log_messages:/home/vorner/work/bind10/src/lib/python:/home/vorner/work/bind10/src/lib/python:/home/vorner/work/bind10/src/lib/dns/python/.libs TESTDATASRCDIR=/home/vorner/work/bind10/src/lib/python/isc/notify/tests/testdata/ /usr/bin/python3 /home/vorner/work/bind10/src/lib/python/isc/notify/tests/$pytest
    make[8]: Leaving directory `/home/vorner/work/bind10/src/lib/python/isc/notify/tests'
    

Should I try to look what happens or can you reproduce or guess?

  • Looking at this test function:
        def get_soa(self):  # emulate ZoneIterator.get_soa()
            if self._zone_name == Name('nosoa.example.com'):
                return None
            soa_rrset = RRset(Name('multisoa.example.com'), RRClass.IN(),
                              RRType.SOA(), RRTTL(3600))
            soa_rrset.add_rdata(Rdata(RRType.SOA(), RRClass.IN(),
                                      'master.example.com. ' +
                                      'admin.example.com. 1234 ' +
                                      '3600 1800 2419200 7200'))
            if self._zone_name == Name('multisoa.example.com'):
                soa_rrset.add_rdata(Rdata(RRType.SOA(), RRClass.IN(),
                                          'master.example.com. ' +
                                          'admin.example.com. 1300 ' +
                                          '3600 1800 2419200 7200'))
                return soa_rrset
            return soa_rrset
    

Doesn't it return a multisoa.example.com SOA even if a different one is asked? Shouldn't it be RRset(self._zone_name, …?

  • The same function, last two lines ‒ the return is the same, so the one inside the condition isn't needed, is it?
  • A docstring in tests, typo (teraDown):
    We just check it doesn't any unexpected disruption and (in teraDown)
    
  • Should the test_check_xfrout_available check a successful case?
  • There's a try-except in the _handle method where the exception is stored and left aside for a while. But, wouldn't a try-except-finally be better?
  • In the notifications, it constructs a client twice, once to get the NS, once to get the SOA. Couldn't it be done with a single client (and finder)?

Thanks

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

Replying to vorner:

BTW, #1217 has been merged, so we should either add True to the call to get_iterator() (and update the Mock class) when merging this, or create a (very minor) ticket to do so, so that TTLs are kept upon xfrout.

I believe this might be better to do now, as it is really small. Creating a ticket would be more work than the actual coding ;-).

Okay, I did it, but to make it workable I needed to merge this branch
with master, which caused a lot of conflicts. One of them is non trivial
(the conflict resolution change in bind10_src.py.in at commit a6fd03e),
so please carefully check that.

So, about the review:

  • For some reason I did not pursue, the tests segfault (yes, in python):
    TESTDATASRCDIR=/home/vorner/work/bind10/src/lib/python/isc/notify/tests/testdata/ \
    /usr/bin/python3 /home/vorner/work/bind10/src/lib/python/isc/notify/tests/$pytest || exit ; \
    done
    Running test: notify_out_test.py
    E/bin/sh: line 1: 24806 Segmentation fault      (core dumped) PYTHONPATH=/home/vorner/work/bind10/src/lib/python/isc/log_messages:/home/vorner/work/bind10/src/lib/python:/home/vorner/work/bind10/src/lib/python:/home/vorner/work/bind10/src/lib/dns/python/.libs TESTDATASRCDIR=/home/vorner/work/bind10/src/lib/python/isc/notify/tests/testdata/ /usr/bin/python3 /home/vorner/work/bind10/src/lib/python/isc/notify/tests/$pytest
    make[8]: Leaving directory `/home/vorner/work/bind10/src/lib/python/isc/notify/tests'
    

Should I try to look what happens or can you reproduce or guess?

Hmm, I couldn't reproduce it on my personal machine or on git.bind10
(a Linux box). So I have no clue. Could you try to get more details
about it? For example, printing debug logs may help.

  • Should the test_check_xfrout_available check a successful case?
  • There's a try-except in the _handle method where the exception is stored and left aside for a while. But, wouldn't a try-except-finally be better?

Do you mean like this?

        try:
            self.dns_xfrout_start(self._sock_fd, self._request_data, quota_ok)
        except Exception as e:
            logger.error(XFROUT_HANDLE_QUERY_ERROR, ex)
        finally:
            # Release any critical resources
            if quota_ok:
                self._server.decrease_transfers_counter()
            self._close_socket()

I didn't do this because the finally part isn't executed if
logger.error raises an exception.

  • In the notifications, it constructs a client twice, once to get the NS, once to get the SOA. Couldn't it be done with a single client (and finder)?

It (at least in theory) couldn't because these are running on
different threads, and a client object is not expected to be shared by
multiple clients. Also, I guess we'll revisit both the basic design
of notify_out (it involves many threads in rather ad-hoc way, making
the code very unreadable) and how to manage data source clients in
this module (it should be more generic rather than hardcoding
sqlite3). At that point we can reconsider a cleaner and possibly more
efficient way. For now, I just added some comment about the rationale.

I believe I addressed other comments.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Okay, I did it, but to make it workable I needed to merge this branch
with master, which caused a lot of conflicts. One of them is non trivial
(the conflict resolution change in bind10_src.py.in at commit a6fd03e),
so please carefully check that.

Hmm, I did expect it to be much easier. But taking most of it were the conflicts, it would have need to be done anyway.

Regarding the change, I've few notes:

  • You added the class for XfrOut?, but not to the dict at the bottom of the special_component.py. This way the installed program can't be started.
  • As xfrout shouldn't be started by start_simple any more, it should be removed from the test version of start_simple.
  • The name 'Xfrout' is inconsistent with the rest in the bob.spec (all others are all lover case).
  • More to the workaround itself, we now have two with the same workaround, with more to expect, probably? Would it be better to just pass the environment variable to anything started? Other things shouldn't be hurt by it anyway and it would remove two specials, making it easier for the user. Or, should we try talking about it tomorrow at the call?

Should I try to look what happens or can you reproduce or guess?

Hmm, I couldn't reproduce it on my personal machine or on git.bind10
(a Linux box). So I have no clue. Could you try to get more details
about it? For example, printing debug logs may help.

Neither I can any longer. So it probably was some kind of false alarm (memory corruption or inconsistency of gcc version between the python itself and the modules).

Do you mean like this?

        try:
            self.dns_xfrout_start(self._sock_fd, self._request_data, quota_ok)
        except Exception as e:
            logger.error(XFROUT_HANDLE_QUERY_ERROR, ex)
        finally:
            # Release any critical resources
            if quota_ok:
                self._server.decrease_transfers_counter()
            self._close_socket()

I didn't do this because the finally part isn't executed if
logger.error raises an exception.

I see.

Anyway, normal tests pass, but now a distcheck fails:

======================================================================
ERROR: test_send_notify_message_udp_ipv4 (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 251, in test_send_notify_message_udp_ipv4
    ('192.0.2.1', 53))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 460, in _send_notify_message_udp
    RRClass(zone_notify_info.zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 490, in _create_notify_message
    zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 501, in _get_zone_soa
    datasrc_config).find_zone(zone_name)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:sqlite3_ds.so: cannot open shared object file: No such file or directory

======================================================================
ERROR: test_send_notify_message_udp_ipv6 (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 258, in test_send_notify_message_udp_ipv6
    ('2001:db8::53', 53))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 460, in _send_notify_message_udp
    RRClass(zone_notify_info.zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 490, in _create_notify_message
    zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 501, in _get_zone_soa
    datasrc_config).find_zone(zone_name)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:sqlite3_ds.so: cannot open shared object file: No such file or directory

======================================================================
ERROR: test_send_notify_message_with_bogus_address (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 270, in test_send_notify_message_with_bogus_address
    ('invalid', 53))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 460, in _send_notify_message_udp
    RRClass(zone_notify_info.zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 490, in _create_notify_message
    zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 501, in _get_zone_soa
    datasrc_config).find_zone(zone_name)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:sqlite3_ds.so: cannot open shared object file: No such file or directory

======================================================================
ERROR: test_wait_for_notify_reply (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 165, in test_wait_for_notify_reply
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 434, in _zone_notify_handler
    self._send_notify_message_udp(zone_notify_info, tgt)
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 460, in _send_notify_message_udp
    RRClass(zone_notify_info.zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 490, in _create_notify_message
    zone_class))
  File "/home/vorner/work/bind10/bind10-devel-20111021/src/lib/python/isc/notify/notify_out.py", line 501, in _get_zone_soa
    datasrc_config).find_zone(zone_name)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:sqlite3_ds.so: cannot open shared object file: No such file or directory

======================================================================
FAIL: test_get_notify_slaves_from_ns (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 311, in test_get_notify_slaves_from_ns
    self.assertEqual(6, len(records))
AssertionError: 6 != 0

======================================================================
FAIL: test_init_notify_out (__main__.TestNotifyOut)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/bind10-devel-20111021/_build/../src/lib/python/isc/notify/tests/notify_out_test.py", line 346, in test_init_notify_out
    self._notify._notify_infos[('example.com.', 'IN')].notify_slaves)
AssertionError: Lists differ: [('3.3.3.3', 53), ('4:4::4:4',... != []

First list contains 3 additional elements.
First extra element 0:
('3.3.3.3', 53)

- [('3.3.3.3', 53), ('4:4::4:4', 53), ('5:5::5:5', 53)]
+ []

----------------------------------------------------------------------
Ran 16 tests in 0.058s

I guess a test file is missing from some variable of Makefile.am.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

Okay, I did it, but to make it workable I needed to merge this branch
with master, which caused a lot of conflicts. One of them is non trivial
(the conflict resolution change in bind10_src.py.in at commit a6fd03e),
so please carefully check that.

Hmm, I did expect it to be much easier. But taking most of it were the conflicts, it would have need to be done anyway.

Regarding the change, I've few notes:

  • You added the class for XfrOut?, but not to the dict at the bottom of the special_component.py. This way the installed program can't be started.
  • The name 'Xfrout' is inconsistent with the rest in the bob.spec (all others are all lover case).

Ah, okay, the original update was quite buggy. Unfortunately it's
difficult to catch bugs around these things via unittests. I believe
I've fixed them at commit bdde86c and 8a5b3e3. I've confirmed that by
installing the system and starting it by hand.

BTW, I've noticed via this bug that the current component
implementation can cause infinite recursive calls (which subsequently
lead to stack overflow and crash) if Component._start_internal
raises an exception: start->_start_internal->(exception)->failed->start->...

Maybe this will be automatically resolved as we implement delayed
restart, but if we cannot expect it to be very soon we should probably
need some intermediate workaround to avoid such catastrophic behavior.

(In any case this will be beyond the scope of this ticket)

  • As xfrout shouldn't be started by start_simple any more, it should be removed from the test version of start_simple.

This one is fixed b5553ef.

  • More to the workaround itself, we now have two with the same workaround, with more to expect, probably? Would it be better to just pass the environment variable to anything started? Other things shouldn't be hurt by it anyway and it would remove two specials, making it easier for the user. Or, should we try talking about it tomorrow at the call?

Apparently #1292 was resolved (I've not closely checked the solution
though). So we can probably just merge and then cleanup the
intermediate hacks for xfrin/out (but I'd defer the cleanup task to a
separate ticket as this ticket could be a blocker for the IXFR-out
support; I'd rather merge this branch first).

Anyway, normal tests pass, but now a distcheck fails:

[...]

I guess a test file is missing from some variable of Makefile.am.

Ah, I see xfrin test's Makefile has a workaround. I've done the same
thing for the notify test (commit 275d091). That doesn't happen on my
system, so could you recheck it fixes the issue?

(Note: we may also be able to clean up this workaround with #1292).

comment:19 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:20 in reply to: ↑ 18 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

BTW, I've noticed via this bug that the current component
implementation can cause infinite recursive calls (which subsequently
lead to stack overflow and crash) if Component._start_internal
raises an exception: start->_start_internal->(exception)->failed->start->...

Maybe this will be automatically resolved as we implement delayed
restart, but if we cannot expect it to be very soon we should probably
need some intermediate workaround to avoid such catastrophic behavior.

Yes, I know. It should be solved by the delayed restarts (as the restart must wait a while, it can't happen inside the current call). We said this will be done in this sprint and should be one of the higher-priority ones, so I guess it will be done before the release.

  • More to the workaround itself, we now have two with the same workaround, with more to expect, probably? Would it be better to just pass the environment variable to anything started? Other things shouldn't be hurt by it anyway and it would remove two specials, making it easier for the user. Or, should we try talking about it tomorrow at the call?

Apparently #1292 was resolved (I've not closely checked the solution
though). So we can probably just merge and then cleanup the
intermediate hacks for xfrin/out (but I'd defer the cleanup task to a
separate ticket as this ticket could be a blocker for the IXFR-out
support; I'd rather merge this branch first).

I'd like to see it resolved before the release, otherwise the user will have to reconfigure after we remove the special components. I don't care if in a separate ticket or not, though. How large do you guess it will be?

Anyway, normal tests pass, but now a distcheck fails:

[...]

I guess a test file is missing from some variable of Makefile.am.

Ah, I see xfrin test's Makefile has a workaround. I've done the same
thing for the notify test (commit 275d091). That doesn't happen on my
system, so could you recheck it fixes the issue?

Is it possible the Makefile in xfrout test needs that too? Because of this failure?

2011-11-15 10:39:31.532 ERROR [b10-xfrout.xfrout] XFROUT_HANDLE_QUERY_ERROR error while handling query: Failed to create DataSourceClient of type sqlite3:sqlite3_ds.so: cannot open shared object file: No such file or directory
E
======================================================================
ERROR: test_axfr_normal_session (__main__.TestXfroutSessionWithSQLite3)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 735, in test_axfr_normal_session
    response = self.sock.read_msg(Message.PRESERVE_ORDER);
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 62, in read_msg
    get_msg.from_wire(bytes(sent_data[2:]), parse_options)
pydnspp.MessageTooShort: Malformed DNS message (short length): 0

----------------------------------------------------------------------

Anyway, this error might show that it doesn't send even SERVFAIL. Is it possible?

Thank you

comment:21 in reply to: ↑ 20 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

  • More to the workaround itself, we now have two with the same workaround, with more to expect, probably? Would it be better to just pass the environment variable to anything started? Other things shouldn't be hurt by it anyway and it would remove two specials, making it easier for the user. Or, should we try talking about it tomorrow at the call?

Apparently #1292 was resolved (I've not closely checked the solution
though). So we can probably just merge and then cleanup the
intermediate hacks for xfrin/out (but I'd defer the cleanup task to a
separate ticket as this ticket could be a blocker for the IXFR-out
support; I'd rather merge this branch first).

I'd like to see it resolved before the release, otherwise the user will have to reconfigure after we remove the special components. I don't care if in a separate ticket or not, though. How large do you guess it will be?

It won't be too large, but that will be a system wide change and not
really specific to xfrout (xfrin will also need to be updated), so I
think it's a topic of a separate ticket. I don't see the point of
"will have to reconfigure" part, too, in the context of whether to
merge this ticket; first off, if the cleanup requires the user to do
something, we already have this problem due to xfrin, so that means
whether or now we merge this branch the issue would have already
existed. second, I really don't understand what the user "will have
to reconfigure". Right now, the special hack for xrin/xfrout are
hardcoded in the bob.spec and the source code, so, basically as long
as the user re-install everything and not heavily customize the
component settings such as specifying a special priority for
xfrin/xfrout (which I suspect is quite unlikely at least for a while),
the change should be transparent.

I'll see haw easy/difficult to make the cleanups anyway, but I still
prefer not delaying for merging this branch just for that reason.

Is it possible the Makefile in xfrout test needs that too? Because of this failure?

Ah, yes, probably. I updated Makefile.am for xfrout/tests, too.

Anyway, this error might show that it doesn't send even SERVFAIL. Is it possible?

That's what the original code does, but it would make more sense to
return SERVFAIL in such a case. I've updated the source to do it.
(I slightly hate to add more special cases in an ad hoc manner like
this, but as we repeatedly discussed we'll need heavy overhaul of xfr*
eventually, and I hope we can make it cleaner at that point)

comment:22 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:23 in reply to: ↑ 21 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I'd like to see it resolved before the release, otherwise the user will have to reconfigure after we remove the special components. I don't care if in a separate ticket or not, though. How large do you guess it will be?

It won't be too large, but that will be a system wide change and not
really specific to xfrout (xfrin will also need to be updated), so I
think it's a topic of a separate ticket. I don't see the point of
"will have to reconfigure" part, too, in the context of whether to
merge this ticket; first off, if the cleanup requires the user to do
something, we already have this problem due to xfrin, so that means
whether or now we merge this branch the issue would have already
existed. second, I really don't understand what the user "will have
to reconfigure". Right now, the special hack for xrin/xfrout are
hardcoded in the bob.spec and the source code, so, basically as long
as the user re-install everything and not heavily customize the
component settings such as specifying a special priority for
xfrin/xfrout (which I suspect is quite unlikely at least for a while),
the change should be transparent.

I didn't really mean to suggest we should hold of merging this branch because of it. I was more interested in if we could do the cleanup before the release as well, which would solve the problem with xfrin as well (which users didn't have a chance to see yet as well).

And with the transparentness, it's not completely true. The configuration keeps the whole Boss/components section if anything in it was changed. So if user changes anything regarding configuration of which processes start or how, he will have to change the configuration manually to remove the special part in the component. Anyway, it probably isn't so much of a problem yet.

Is it possible the Makefile in xfrout test needs that too? Because of this failure?

Ah, yes, probably. I updated Makefile.am for xfrout/tests, too.

All tests now pass, probably all needed workarounds are there already.

Anyway, this error might show that it doesn't send even SERVFAIL. Is it possible?

That's what the original code does, but it would make more sense to
return SERVFAIL in such a case. I've updated the source to do it.
(I slightly hate to add more special cases in an ad hoc manner like
this, but as we repeatedly discussed we'll need heavy overhaul of xfr*
eventually, and I hope we can make it cleaner at that point)

Thanks.

So, I believe this can be merged now.

comment:24 in reply to: ↑ 23 Changed 8 years ago by jinmei

Replying to vorner:

I didn't really mean to suggest we should hold of merging this branch because of it. I was more interested in if we could do the cleanup before the release as well, which would solve the problem with xfrin as well (which users didn't have a chance to see yet as well).

From a closer look at #1292 I realized it really hasn't solved the installed
version of the problem, so we first need to solve it anyway. I've
reopened #1292, and I've created a new ticket for the cleanup task (#1388).

So, I believe this can be merged now.

Okay, thanks for the review. Merge done, closing this one.

comment:25 Changed 8 years ago by jinmei

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

comment:26 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 17.33

comment:27 Changed 8 years ago by vorner

  • Add Hours to Ticket changed from 0 to 3.75

comment:28 Changed 8 years ago by vorner

  • Add Hours to Ticket 3.75 deleted
  • Total Hours changed from 17.33 to 21.08
Note: See TracTickets for help on using tickets.