Opened 7 years ago

Closed 7 years ago

#2879 closed defect (fixed)

notify_out doesn't handle response to NOTIFY correctly

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

Description

I've noticed some odd log handling in notify lettuce tests.
See my comment in http://bind10.isc.org/ticket/1938#comment:15

It looks like _zone_notify_handler doesn't correctly cancel the
completed notify transaction after _EVENT_READ, and each response
immediately triggers another notify. It's obviously wrong and should
be fixed.

Subtickets

Attachments (1)

xfrin_notify_handling.feature.patch (6.0 KB) - added by naokikambe 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 7 years ago by muks

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

comment:3 Changed 7 years ago by naokikambe

Due to this issue, some statistics counters cannot be expected to be a accurate one in the lettuce test. If this issue is resolved, the lettuce test should be also cleaned up accordingly. For details, please see ticket:2252#comment:17 and the following commit

9d00667 [2252] update the note to more accurate one due to bug #2879

I'm also attaching a proposed patch.

Changed 7 years ago by naokikambe

comment:4 Changed 7 years ago by jinmei

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

comment:5 Changed 7 years ago by jinmei

I'm releasing this ticket for now (as #2831 is back to me).

I figured out the cause of the problem: _zone_notify_handler()
should compare the return value from _handle_notify_reply() against
_REPLY_OK, not just evaluate it as a boolean:

                if self._handle_notify_reply(zone_notify_info, reply, tgt):
                    self._notify_next_target(zone_notify_info)

According to the history this problem seems to have been there almost
since the introduction of notify_out.py three years ago. This also
means this method (or this module in general) is so poorly tested.
So I planned to make at least _handle_notify_reply() fully tested,
not just to fix this particular issue and add a test case for this
particular point. But I've not done any of these yet.

I've made a branch trac2879 and just committed some editorial cleanups.
If someone is in the state of looking for a new open task, feel free
to continue it based on the above analysis.

comment:6 Changed 7 years ago by jinmei

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

comment:7 Changed 7 years ago by jinmei

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

comment:8 Changed 7 years ago by jinmei

trac2879 is ready for review.

See my intermediate comment above. The branch basically did what I
planned.

Proposed changelog entry:

606.?	[bug]		jinmei
	b10-xfrout now correctly stops sending notify requests once it
	receives a valid response.  It previously handled it as if the
	requests are timed out and resent it a few times in a short
	period.
	(Trac #2879, git TBD)

comment:9 Changed 7 years ago by jinmei

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

comment:10 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 0.18

Hello

I discovered just this bit of probably wrongly used language. Should it be just „slave got the notify“ instead of „has got“?

from the slave, it means the slave has got the notify.

So, after fixing it, please merge.

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

Thanks for the review.

Replying to vorner:

I discovered just this bit of probably wrongly used language. Should it be just „slave got the notify“ instead of „has got“?

from the slave, it means the slave has got the notify.

The text wasn't introduced in this branch (I just reformatted it), so
I don't know the original intent. Personally I don't think it wrong
(or would rather think the present perfect form better than the pure
past tense in this context), but I don't mind the change either, so I
made the change as suggested.

So, after fixing it, please merge.

Okay, merge done, closing.

comment:13 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.18 to 5.18
Note: See TracTickets for help on using tickets.