Opened 8 years ago

Closed 7 years ago

#2023 closed defect (fixed)

xfrin doesn't send the right command to zonemgr after transfer

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

Description

From code inspection, it seems to send the 'notify' command
(= notify_out.ZONE_NEW_DATA_READY_CMD) while it should actually send
'zone_new_data_ready' (= zonemgr.ZONE_XFRIN_SUCCESS_COMMAND)
(see publish_xfrin_news()).

It should be fixed, and the fix should be trivial. The bigger problem
is that it seems to me yet another incident of the current xfr-related
things are so broken in various ways (ugly implementation, luck of
tests, ad hoc design architecture, command naming conflict, etc), but
for now, let's fix the immediate problem.

I also suspect this is a background reason for #1786 and #1834.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by muks

Lettuce test should check this change.

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by muks

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

Picking bug.

comment:4 Changed 8 years ago by muks

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

Up for review. I've also moved another xfrin->zonemgr notification into isc.notify.

I also want to add a lettuce check for it, to see that zonemgr is getting the correct notification, but my zonemgr is not logging for some reason. It just stops logging after ZONEMGR_STARTED.

ddns also uses ZONE_NEW_DATA_READY_CMD. I'm not sure if 'zone_new_data_ready' was really intended, or 'notify'. There are no ddns lettuce tests yet to verify (make check and existing lettuce tests pass, but that's not enough).

The ddns lettuce tests should be in master soon, at which point this can be tested. Until then, the branch can be reviewed.

comment:5 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to muks

Hello

It looks OK, so please merge.

Just out of curiosity, did you read the commit message why it was changed back then?

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

And, please check if this fix also fixes #1786 and #1834 as I guessed, and if so,
close them too.

comment:8 in reply to: ↑ 6 Changed 7 years ago by muks

Hi vorner

Replying to vorner:

Just out of curiosity, did you read the commit message why it was changed back then?

I don't know why it was changed, but I guess it was changed to handle the "notify" command in xfrout. Renaming the command itself seems to be a mistake looking back at it, as "notify" had other meanings in zonemgr.

comment:9 in reply to: ↑ 7 Changed 7 years ago by muks

Replying to jinmei:

And, please check if this fix also fixes #1786 and #1834 as I guessed, and if so,
close them too.

It should fix #1786. I'll add comments to the bugs.

comment:10 Changed 7 years ago by muks

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

Merged to master:

* 4286da1 [2023] Check that zonemgr receives the ZONE_NEW_DATA_READY_CMD notification in lettuce test
* cb8f6b5 [2023] Also move ZONE_XFRIN_FAILED to a single module
* 0c0107e [2023] Change back ZONE_NEW_DATA_READY_CMD to 'zone_new_data_ready'

Also pushed directly to master after merge:

* 380c6b2 [2023] Fix notify command name in the xfrout command_handler

Resolving as fixed. Thank you for the review vorner.

Note: See TracTickets for help on using tickets.