Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4211 closed defect (fixed)

Handle DUID write failures in the unit tests

Reported by: marcin Owned by: marcin
Priority: high Milestone: Kea1.0-beta
Component: dhcp6 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

After merge of #3874 there is a regression in distcheck. For example see here: https://jenkins.isc.org/job/Ubuntu_12.04_32_distcheck/349/console

The reason for this is that the server is trying to write generated server id in the write protected location. I've been thinking about it a bit and decided to introduce parameter that explicitly enables/disables write of a DUID to the file. Note that during the review of #3874 Tomek suggested that we should handle cases when there is no persistent storage available. In such environments we should be able to disable writing a DUID to a file. In this particular case of a failing distcheck we'll disable writing a server id to a file in some of the tests.

I am moving it to 1.0 because it is a regression after a merge of 1.0 ticket.

Subtickets

Change History (6)

comment:1 Changed 4 years ago by marcin

  • Owner set to UnAssigned
  • Status changed from new to reviewing

This ticket is now ready for review.

Proposed ChangeLog entry:

10XX.	[func]		marcin
	It is possible to disable writing generated DHCPv6 server
	identifier in a persistent storage. This also fixes a
	failing distcheck.
	(trac #4211, git abcd)

comment:2 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:3 Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

dhcp6-srv.xml
s/hard drive/stable storage. Most devices that could possibly take use
of that feature are routers and other embedded devices. They don't
have a hard drive per se, just various types of flash and rom memory.

dhcp6.spec
Thanks a lot for taking care of the .spec update. Can you add a brief
"item_description" for the new parameter you added? I know that
there's a lot of entries that lack this field, but we should try
to add it to new fields that are introduced.

io_utils.{cc|h}
It feels really bizarre that we did not have any utility function
for dealing with those basic operations. I did search around a bit,
but it seems that we indeed didn't have such a code before.


Ok, I take it back what I said on Jabber. Reviewing 25 files is not
bad at all. Yes, it's a lot of files, but each file takes seconds to
review. I like it that way :)


On a more serious note, when you think about the grand picture, the
capability to store or skip storing the DUID on disk is a small
feature. If it requires updating 25 files, I think there's something
wrong with how we organized the code. This should be one clever if
clause in the duid management code, a few lines in the config parser
somewhere and unit-tests update. Together with the doc update, this
should be maybe 5-7 files in total. If such a simple change requires
so many changes, it means that in the long run, we'll be spending
majority of our engineering time simply updating unit-tests. This is a
serious danger in the long term. I'll bring this up some time in
January once the 1.0 excitement is gone.


The code builds and unit-tests pass on Ubuntu 15.10 x64, except
unrelated failure:

[ RUN      ] NameChangeUDPListenerTest.basicReceivetest
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:259: Failure
Value of: result_
  Actual: 3
Expected: NameChangeListener::SUCCESS
Which is: 0
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:262: Failure
Value of: checkSendVsReceived(sent_ncr_, received_ncr_)
  Actual: false
Expected: true
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:259: Failure
Value of: result_
  Actual: 3
Expected: NameChangeListener::SUCCESS
Which is: 0
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:262: Failure
Value of: checkSendVsReceived(sent_ncr_, received_ncr_)
  Actual: false
Expected: true
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:262: Failure
Value of: checkSendVsReceived(sent_ncr_, received_ncr_)
  Actual: false
Expected: true
../../../../../../src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc:271: Failure
Value of: listener_->isIoPending()
  Actual: true
Expected: false
[  FAILED  ] NameChangeUDPListenerTest.basicReceivetest (1 ms)

I did see it before on master, so it seems to be a problem with my
build environment.


The changes requested in this review are minimal, so I don't need to
see this ticket again. Please merge as soon as you deal with them.

Thanks for addressing this issue.

comment:4 Changed 4 years ago by tomek

One more minor comment: I think the changes in #3874 and #4211 address #2645. If you agree with that, please move #2645 to 1.0 milestone and close it.

comment:5 Changed 4 years ago by marcin

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

Merged with commit d8f39b7aff9312237d4b6d6de39a7336a25ead4c

comment:6 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.