Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#3374 closed defect (fixed)

Component removal broken, Revert 3339 and 3373

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea0.9
Component: configuration Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0.5
Total Hours: 5 Internal?: no

Description

Changes to configuration libraries in an attempt to treat maps differently, undermined the layer's behavior in regards to adding and remvoing components, and likely other unforeseen impacts.

This ticket covers reverting the two tickets responsible.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by tmark

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

comment:2 follow-up: Changed 6 years ago by tmark

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

Reversions are complete.

All unit tests and forge tests pass.

Recommended ChangeLog entry:

7xx.    [bug]       tmark
    Tickets #3339 and #3373 (see entries 760 and 768 respectively) 
    were reverted. They introduced a bug where components added through 
    bindctl, could not be removed. 
    (Trac #3362, git da3b0d4f364d069ffdb47723545798ac589fae42)

comment:3 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:4 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0 to 0.5
  • Owner changed from tomek to tmark
  • Total Hours changed from 0 to 0.5

This ticket is reverting wrong commits, I'm afraid.

There are 4 commits on branch trac3374:

  1. 6bf8a19728573e86e66b1ba9da1d7a606828e75f "trac3373" (oldest)
  2. d339f1979f995f611ce7aa130d2fe88f5450d9ab "trac3373"
  3. 5b30c7090341d4f4dc060c267af241d98ad07821 "trac3339"
  4. 44d6330d9283ca1e02e8e1f75fea90b18d7c1ba8 "trac3339" (newest)

Comment for commit 1 claims:

    This reverts commit da3b0d4f364d069ffdb47723545798ac589fae42, reversing
    changes made to ff46195513aff7b46efdd3c4d06bd777bc86c352.

But if you inspect it with gitk, it shows changes being done to unit-tests in src/lib/dhcpsrv. In fact, it looks like it removes all changes done in 3359!

Commit 2 looks ok (reverts 3373 as advertised).

Commit 3 looks fine as well (seems to revert 3339 as advertised).

Commit 4 is wrong. I was not able to precisely identify which changes it reverts, but I'm sure that it reverts 3322 (relay info override), among others.

To easily see what the commits revert, I used gitk tool, which is nice, easy to use, graphical tool.

To eliminate any doubts, I also used normal git diff and compared differences between commits.

I'm not sure what to do with this. One possible strategy would be to get rid of the trac3374 branch altogether and start from scratch. I have never used git revert, so I'm afraid I can't offer any guidance here.

comment:5 Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

This was my first attempt to use revert. I wiped out the branch and started fresh, and did used a rote process to undo the changes. You should once again be able to add and remove components, such as b10-dhcp6, repeatedly in bindctl without issue:

Adding:

config add Init/components b10-dhcp6
config set Init/components/b10-dhcp6/kind dispensable
config commit

Removing:

config set Dhcp6/subnet6 []
config set Dhcp6/option-def []
config set Dhcp6/option-data []
config set Logging/loggers []
config set Dhcp6/hooks-libraries []
config remove Init/components b10-dhcp6
config commit
Dhcp6 shutdown

comment:6 Changed 6 years ago by tomek

  • Owner changed from tomek to tmark

I did review the changes. They look ok this time.

The only thing that seems to be missing is a changelog entry, but since you mentioned it in this ticket, I assume it will be added.

I have not run any tests though. It may be best to ask Wlodek to run Forge tests on this branch and then merge it.

comment:7 Changed 6 years ago by tmark

  • Estimated Difficulty changed from 0 to 4
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.5 to 5

Changes merged with git c641e2d0569df3ca3e5a93beaf0ecf39db07e402
Add ChangeLog entry 772

comment:8 in reply to: ↑ 2 Changed 6 years ago by jreed

Replying to tmark:

7xx.    [bug]       tmark
    Tickets #3339 and #3373 (see entries 760 and 768 respectively) 
    were reverted. They introduced a bug where components added through 
    bindctl, could not be removed. 
    (Trac #3362, git da3b0d4f364d069ffdb47723545798ac589fae42)

Maybe the 768 entry is not needed. It was never released so we don't need to have changelog entries for un-released changes. Maybe we can reuse the 768 for this one?

comment:9 Changed 5 years ago by hschempf

  • Milestone changed from Kea1.0 to Kea0.9
  • Version set to git
Note: See TracTickets for help on using tickets.