Opened 12 months ago

Closed 5 months ago

#5374 closed defect (complete)

ClientClasses should be ordered

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: classification 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: 14
Total Hours: 14 Internal?: no

Description

Today it is defined as a set so without specified order. It should be explicitly ordered so a defined as a list and not a set.
Classified as low because this has no obvious bad impact today.

Subtickets

Change History (30)

comment:1 Changed 12 months ago by fdupont

About performance we have to check what are the operation we do on it. IMHO only the find method is significantly different.

comment:2 Changed 12 months ago by fdupont

Or implementing it as both unordered_set and list. We can do what we want as it is a class!

comment:3 Changed 12 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4

As discussed on 2017-10-05 call, moving to 1.4.

comment:4 Changed 11 months ago by fdupont

IMHO we need a more generic client class improvements design ticket with implementation tickets. The ordering is only one item, I think about adding a "member" expression token to code easily class combinations using boolean operators. BTW this will allow to fix the white / black list mess which is not (and IMHO should be never) implemented. There are IMHO two not trivial points: class definition must be checked to have only backward references (so no loops) and class (list) setting by host reservation is done after classification.

comment:5 Changed 10 months ago by fdupont

Should become a child of #5429.

comment:6 Changed 10 months ago by fdupont

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

comment:7 follow-up: Changed 10 months ago by fdupont

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

Reused the ticket to go a bit further:

  • get rid of white list code (never finished: white lists have zero or one element)
  • use insert order for client class collections and client class definitions. Note this allows next items.
  • add membership token (member('foobar')) which checks if the packet already belongs to a class, e.g. with not allows to create the complement of a class in an efficient and easy way
  • to avoid dependency loops only backward and built-in dependencies are allowed by the parser.

Note trac5374 branch is stacked on trac5425 so please review #5425 (pools

comment:8 in reply to: ↑ 7 Changed 10 months ago by fdupont

Replying to fdupont:

Reused the ticket to go a bit further:

  • get rid of white list code (never finished: white lists have zero or one element)
  • use insert order for client class collections and client class definitions. Note this allows next items.
  • add membership token (member('foobar')) which checks if the packet already belongs to a class, e.g. with not allows to create the complement of a class in an efficient and easy way
  • to avoid dependency loops only backward and built-in dependencies are allowed by the parser.

Note trac5374 branch is stacked on trac5425 so please review #5425 (class guard in pools).

comment:9 Changed 10 months ago by fdupont

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

Got a new idea (from the ISC DHCP if statement): add an eval-on-demand flag (false by default) to implement late evaluation of class expressions. BTW this should allow to have an effect on options n the reserve order (current order is host reservation, subnet, network, classes and global (perhaps only "is" cf #5438).

comment:10 Changed 10 months ago by fdupont

Pushed a fix to missing stuff in #5288 (option data in DHCPv4 pools) at f50a2b92df..1db3a9656f on trac5374.

comment:11 Changed 10 months ago by fdupont

  • Add Hours to Ticket changed from 0 to 12
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 12

Ready for review.

comment:13 Changed 10 months ago by fdupont

Oops, wrong window.

comment:14 Changed 8 months ago by tomek

  • Component changed from Unclassified to classification
  • Priority changed from low to medium

This ticket is part of the goal to improve client classification. It's also essential part of the plan laid out in ticket:5425#comment:19. Hence bumping up priority.

comment:15 Changed 6 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:16 Changed 6 months ago by marcin

I just started looking into this ticket. It appears to have ~6700 lines of code changes (excluding regenerated parsers). The review will take time.

comment:17 Changed 6 months ago by marcin

Also, this ticket introduces changes in 82 files.

comment:18 Changed 6 months ago by fdupont

If you follows the commits this ticket can be split into a small set of different dependent changes. (but after some discussions with Tomek it is not clear that a chain of tickets is really easier to review)

comment:19 follow-up: Changed 6 months ago by marcin

  • Owner changed from marcin to UnAssigned

I reviewed the code changes until commit f848c18c0a58674cf61019b752b10a5c48caffb2.

I have corrected small issue in the doxygen, so please pull.

First of all, I believe that such an extensive change requires more thorough documentation. For example, I'd very much like to see descriptions of some typical use cases when "on-demand" classification is required/useful. From the existing documentation I can't really figure out whether it is only a performance optimzation (to not evaluate classes which aren't to be used anyway) or there are some "functional" requirements which they address.

In addition, I don't see any new example files (or updated existing files) which demonstrate these new features.

I am not sure the "eval-on-demand" is the best name for this parameter. It sounds like it is evaluated when some external entity requests such evaluation. In fact, it is a subnet, pool or network which is configured to use this class. I think a better name would be "lazy-evaluation"?

doc/guide/classify.xml
There is a descrepnacy between the following sentence:

"The process of doing classification is conducted in five steps. The first step is to assess an incoming ..."

in this file and similar sentences in dhcp4-srv.xml and dhcp6-srv.xml:

"The process of doing classification is conducted in five steps. The first step..."

The following sentence: "Perform a second pass by evaluating match expressions of on-demand classes." doesn't seem to be sufficient. It doesn't explain why the second pass is required and what it means "on-demand" class. It should point to the relevant section of the guide where the on-demand classes are well explained.

OLD:

Beginning with 1.4 client classes follow now the insertion order (vs. alphabetical order in previous versions).

NEW:

Beginning with Kea 1.4.0 release, client classes follow the order in which they are specified in the configuration (vs. alphabetical order in previous versions).

The following sentence:

When the eval-on-demand flag is set to true in a class definition, the match expression is skipped during the first evaluation pass.

doesn't really fit into the whole text surrounding it. If you want to keep it out there, it might be good to provide some context:

Expressions for defined classes are evaluated upon reception of each incoming packet, except for the classes for which "eval-on-demand" parameter is set to true. Those expressions are evaluated when a network, subnet or pool selected for this packet is said to use those classes.

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

OLD:

In versions before 1.4 the alphabetical order was used.

NEW:

In Kea versions prior to 1.4.0 the alphabetical order of classes was used. Starting from Kea 1.4.0 the classes are ordered as specified in the configuration.

As I already mentioned at the beginning of the review, the "On-demand classification" section should describe use cases when on-demand classes are useful, rather than merely saying that it is useful.

src/bin/dhcp4/tests/classify_unittest.cc
src/bin/dhcp6/tests/classify_unittests.cc
I would like to see some tests which verify that the pool/subnet/network can be picked when the client belongs to multiple classes, i.e. member('foo') and member('bar'). Note that there are use cases for it in the HA.

src/bin/dhcp6/tests/shared_network_unittest.cc
src/bin/dhcp4/tests/shared_network_unittest.cc
The "precedence tests" are so similar to each other that they could be enclosed in a function rather than copy pasted. This function could use the config as a parameter.

src/lib/dhcpsrv/network.h
The deferClientClass and getOnDemandClasses names do not correspond to each other, but they should. It seems to be better to rename deferClientClass to addOnDemandClass or "addLazyEvaluationClass" assuming that you follow my suggestion to not use "on demand" term.

The description of the "client_class_" says that "default value for this is an empty list". In fact it is now a single class, not a list.

"on_demand_classes_" description says that "these classes will be added to the incoming packet and evaluated earlier". I suggest to say "these classes will be added to the incoming packet and evaluated if this network (or its derivation) is selected for DHCP client asssignments.

src/lib/dhcpsrv/pool.cc
clientSupported: Instead of doing:

if (client_class_.empty()) {
    match = true;
} else if (client_classes.contains(client_class_)) {
    match = true;
}

you could simply do:

bool match = (client_class_.empty() || classes.contains(client_class_));

src/lib/dhcpsrv/pool.h
I don't like the names of deferClientClass and getOnDemandClasses as pointed out earlier for network.h

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
dependentForwardError: the description of this test says that it checks that forward dependencies will not parse, but in fact it merely tests that the class with a reference to a non existing class will cause parsing error. So, this is not really a forward reference case error. Either description or a test should be fixed.

src/lib/eval/eval.dox
Regarding the new paragraph:

Parameters to the isc::eval::EvalContext class constructor are the universe to choose between DHCPv4 and DHCPv6for dependent expressions, and a closure which checks if a client class is already known used by the parser to accept only already known or built-in client class names in client class membership expressions. This closure defaults to accept all client class names.

Extraneous "known" or "used" in here: "client class is already known used"

The first sentence is unclear to me. Specifically I don't understand the connection between the universe: DHCPv4 and DHCPv6 and "dependent expressions". Wouldn't it be enough to say

Parameters to the isc::eval::EvalContext constructor are the universe (DHCPv4 or DHCPv6), and the closure ...

src/lib/eval/eval_context.h
I suggest that the typedef is used for the "check_known closure", instead of repeating the std::function template in multiple places.

comment:20 Changed 6 months ago by marcin

  • Owner changed from UnAssigned to fdupont

comment:21 in reply to: ↑ 19 ; follow-up: Changed 6 months ago by fdupont

Replying to marcin:

I reviewed the code changes until commit f848c18c0a58674cf61019b752b10a5c48caffb2.

I have corrected small issue in the doxygen, so please pull.

=> thanks, I don't remember if I fixed doxygen (I am afraid I do it only half time).

First of all, I believe that such an extensive change requires more thorough documentation. For example, I'd very much like to see descriptions of some typical use cases when "on-demand" classification is required/useful. From the existing documentation I can't really figure out whether it is only a performance optimzation (to not evaluate classes which aren't to be used anyway) or there are some "functional" requirements which they address.

=> it is functional requirement to have different phases in classification because of dependencies of some classes on things which are only available at some points of the service processing. Now we can argue about the name "on-demand" which is perhaps not the best (only the best I found).

In addition, I don't see any new example files (or updated existing files) which demonstrate these new features.

=> noted.

I am not sure the "eval-on-demand" is the best name for this parameter. It sounds like it is evaluated when some external entity requests such evaluation. In fact, it is a subnet, pool or network which is configured to use this class. I think a better name would be "lazy-evaluation"?

=> lazy-evaluation is the very technical name of call-by-need so I am afraid it goes on the bad direction. Remember at an all hands' we got someone who did not know what is a closure. So I explicitly want to stay far from any concept which are very familiar (my PhD was about programming languages) but only to me and people who worked with me 20 years ago...
So I agree the "eval-on-demand" is not very good but lazy-evaluation is worse.

doc/guide/classify.xml
There is a descrepnacy between the following sentence:

"The process of doing classification is conducted in five steps. The first step is to assess an incoming ..."

in this file and similar sentences in dhcp4-srv.xml and dhcp6-srv.xml:

"The process of doing classification is conducted in five steps. The first step..."

The following sentence: "Perform a second pass by evaluating match expressions of on-demand classes." doesn't seem to be sufficient. It doesn't explain why the second pass is required and what it means "on-demand" class. It should point to the relevant section of the guide where the on-demand classes are well explained.

=> hum, I'll try to make this clearer as obviously you didn't fully understand it.

OLD:

Beginning with 1.4 client classes follow now the insertion order (vs. alphabetical order in previous versions).

NEW:

Beginning with Kea 1.4.0 release, client classes follow the order in which they are specified in the configuration (vs. alphabetical order in previous versions).

The following sentence:

When the eval-on-demand flag is set to true in a class definition, the match expression is skipped during the first evaluation pass.

doesn't really fit into the whole text surrounding it. If you want to keep it out there, it might be good to provide some context:

Expressions for defined classes are evaluated upon reception of each incoming packet, except for the classes for which "eval-on-demand" parameter is set to true. Those expressions are evaluated when a network, subnet or pool selected for this packet is said to use those classes.

=> the last sentence is correct so I have to rewrite explanations: in fact you have both late evaluation and late assignment as you have today evaluation and assignment. And I am afraid with the known/unknown we'll go to 3 phases...

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

OLD:

In versions before 1.4 the alphabetical order was used.

NEW:

In Kea versions prior to 1.4.0 the alphabetical order of classes was used. Starting from Kea 1.4.0 the classes are ordered as specified in the configuration.

As I already mentioned at the beginning of the review, the "On-demand classification" section should describe use cases when on-demand classes are useful, rather than merely saying that it is useful.

src/bin/dhcp4/tests/classify_unittest.cc
src/bin/dhcp6/tests/classify_unittests.cc
I would like to see some tests which verify that the pool/subnet/network can be picked when the client belongs to multiple classes, i.e. member('foo') and member('bar'). Note that there are use cases for it in the HA.

=> no problem. If you have an example which makes sense it will be even better.

src/bin/dhcp6/tests/shared_network_unittest.cc
src/bin/dhcp4/tests/shared_network_unittest.cc
The "precedence tests" are so similar to each other that they could be enclosed in a function rather than copy pasted. This function could use the config as a parameter.

=> I (too) often (ab)use of cut & paste.

src/lib/dhcpsrv/network.h
The deferClientClass and getOnDemandClasses names do not correspond to each other, but they should. It seems to be better to rename deferClientClass to addOnDemandClass or "addLazyEvaluationClass" assuming that you follow my suggestion to not use "on demand" term.

=> defer is related to late so IMHO the opposite could be better.

The description of the "client_class_" says that "default value for this is an empty list". In fact it is now a single class, not a list.

=> I'll look at it and fix it.

"on_demand_classes_" description says that "these classes will be added to the incoming packet and evaluated earlier". I suggest to say "these classes will be added to the incoming packet and evaluated if this network (or its derivation) is selected for DHCP client asssignments.

=> as I said before I didn't make late evaluation and late assignment enough distinct.

src/lib/dhcpsrv/pool.cc
clientSupported: Instead of doing:

if (client_class_.empty()) {
    match = true;
} else if (client_classes.contains(client_class_)) {
    match = true;
}

you could simply do:

bool match = (client_class_.empty() || classes.contains(client_class_));

=> yes but do you believe it is clearer? I know some people who do not like direct boolean expressions
(even it this case both are 100% equivalent).

src/lib/dhcpsrv/pool.h
I don't like the names of deferClientClass and getOnDemandClasses as pointed out earlier for network.h

=> noted in .cc.

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
dependentForwardError: the description of this test says that it checks that forward dependencies will not parse, but in fact it merely tests that the class with a reference to a non existing class will cause parsing error. So, this is not really a forward reference case error. Either description or a test should be fixed.

=> not existing -> not yet existing? In any case the test must be clarified.

src/lib/eval/eval.dox
Regarding the new paragraph:

Parameters to the isc::eval::EvalContext class constructor are the universe to choose between DHCPv4 and DHCPv6for dependent expressions, and a closure which checks if a client class is already known used by the parser to accept only already known or built-in client class names in client class membership expressions. This closure defaults to accept all client class names.

Extraneous "known" or "used" in here: "client class is already known used"

The first sentence is unclear to me. Specifically I don't understand the connection between the universe: DHCPv4 and DHCPv6 and "dependent expressions". Wouldn't it be enough to say

Parameters to the isc::eval::EvalContext constructor are the universe (DHCPv4 or DHCPv6), and the closure ...

src/lib/eval/eval_context.h
I suggest that the typedef is used for the "check_known closure", instead of repeating the std::function template in multiple places.

=> I'll look at it and try to improve it.

Thanks for the review. I stopped before going too far from the initial target but I am afraid some explanations were lost or not completed. I'll try to make this easier to follow so #5549 will fit inside this naturally.

comment:22 Changed 6 months ago by fdupont

I propose a plan in 3 steps:

  • update (at the limit) rewrite the guide about client classification
  • decide the right terminology not forgetting #5549
  • apply new terms to names used in the code.

Working on the first step: as soon as we agree about the client classification does we can try to find better terms.

comment:23 Changed 6 months ago by marcin

I am thinking that the better names could be:

  • eval-when-required instead of eval-on-demand
  • require-client-classes instead of eval-client-classes

comment:24 in reply to: ↑ 21 Changed 6 months ago by marcin

Replying to fdupont:

Replying to marcin:

I reviewed the code changes until commit f848c18c0a58674cf61019b752b10a5c48caffb2.

I have corrected small issue in the doxygen, so please pull.

=> thanks, I don't remember if I fixed doxygen (I am afraid I do it only half time).

First of all, I believe that such an extensive change requires more thorough documentation. For example, I'd very much like to see descriptions of some typical use cases when "on-demand" classification is required/useful. From the existing documentation I can't really figure out whether it is only a performance optimzation (to not evaluate classes which aren't to be used anyway) or there are some "functional" requirements which they address.

=> it is functional requirement to have different phases in classification because of dependencies of some classes on things which are only available at some points of the service processing. Now we can argue about the name "on-demand" which is perhaps not the best (only the best I found).

This really comes down to providing a good example in the user's guide to explain that "phased classification" is the only way to solve THAT problem, where THAT is to be described.

In addition, I don't see any new example files (or updated existing files) which demonstrate these new features.

=> noted.

I am not sure the "eval-on-demand" is the best name for this parameter. It sounds like it is evaluated when some external entity requests such evaluation. In fact, it is a subnet, pool or network which is configured to use this class. I think a better name would be "lazy-evaluation"?

=> lazy-evaluation is the very technical name of call-by-need so I am afraid it goes on the bad direction. Remember at an all hands' we got someone who did not know what is a closure. So I explicitly want to stay far from any concept which are very familiar (my PhD was about programming languages) but only to me and people who worked with me 20 years ago...
So I agree the "eval-on-demand" is not very good but lazy-evaluation is worse.

I proposed some other names in another comment.

doc/guide/classify.xml
There is a descrepnacy between the following sentence:

"The process of doing classification is conducted in five steps. The first step is to assess an incoming ..."

in this file and similar sentences in dhcp4-srv.xml and dhcp6-srv.xml:

"The process of doing classification is conducted in five steps. The first step..."

The following sentence: "Perform a second pass by evaluating match expressions of on-demand classes." doesn't seem to be sufficient. It doesn't explain why the second pass is required and what it means "on-demand" class. It should point to the relevant section of the guide where the on-demand classes are well explained.

=> hum, I'll try to make this clearer as obviously you didn't fully understand it.

OLD:

Beginning with 1.4 client classes follow now the insertion order (vs. alphabetical order in previous versions).

NEW:

Beginning with Kea 1.4.0 release, client classes follow the order in which they are specified in the configuration (vs. alphabetical order in previous versions).

The following sentence:

When the eval-on-demand flag is set to true in a class definition, the match expression is skipped during the first evaluation pass.

doesn't really fit into the whole text surrounding it. If you want to keep it out there, it might be good to provide some context:

Expressions for defined classes are evaluated upon reception of each incoming packet, except for the classes for which "eval-on-demand" parameter is set to true. Those expressions are evaluated when a network, subnet or pool selected for this packet is said to use those classes.

=> the last sentence is correct so I have to rewrite explanations: in fact you have both late evaluation and late assignment as you have today evaluation and assignment. And I am afraid with the known/unknown we'll go to 3 phases...

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

OLD:

In versions before 1.4 the alphabetical order was used.

NEW:

In Kea versions prior to 1.4.0 the alphabetical order of classes was used. Starting from Kea 1.4.0 the classes are ordered as specified in the configuration.

As I already mentioned at the beginning of the review, the "On-demand classification" section should describe use cases when on-demand classes are useful, rather than merely saying that it is useful.

src/bin/dhcp4/tests/classify_unittest.cc
src/bin/dhcp6/tests/classify_unittests.cc
I would like to see some tests which verify that the pool/subnet/network can be picked when the client belongs to multiple classes, i.e. member('foo') and member('bar'). Note that there are use cases for it in the HA.

=> no problem. If you have an example which makes sense it will be even better.

I have two servers which do load balancing. The pkt4_receive callout assigns each packet to one of the classes, i.e. "server1" or "server2". The "server1" class is associated with its own pools in a subnet, and the "server2" class is associated with other pools within that subnet. The "server1" must only use its own pools and the "server2" must use its pools. There is additional pool partitioning applied: "telephones" and "computers", each of them picking addresses from distinct pools. So, you want to be able to partition your subnet into 4 pools like this:

  • pool1: "server1" and "telephones"
  • pool2: "server1" and "computers"
  • pool3: "server2" and "telephones"
  • pool4: "server2" and computers.

So, if a telephone requests an address and its request is picked by server1 (of HA) it will use an address from pool1 (and only pool1). If it is a computer and its request is picked by the server2 it will use pool4 etc.

src/bin/dhcp6/tests/shared_network_unittest.cc
src/bin/dhcp4/tests/shared_network_unittest.cc
The "precedence tests" are so similar to each other that they could be enclosed in a function rather than copy pasted. This function could use the config as a parameter.

=> I (too) often (ab)use of cut & paste.

src/lib/dhcpsrv/network.h
The deferClientClass and getOnDemandClasses names do not correspond to each other, but they should. It seems to be better to rename deferClientClass to addOnDemandClass or "addLazyEvaluationClass" assuming that you follow my suggestion to not use "on demand" term.

=> defer is related to late so IMHO the opposite could be better.

We need to conclude on config parameters naming before we can proceed with this, I guess...

The description of the "client_class_" says that "default value for this is an empty list". In fact it is now a single class, not a list.

=> I'll look at it and fix it.

"on_demand_classes_" description says that "these classes will be added to the incoming packet and evaluated earlier". I suggest to say "these classes will be added to the incoming packet and evaluated if this network (or its derivation) is selected for DHCP client asssignments.

=> as I said before I didn't make late evaluation and late assignment enough distinct.

src/lib/dhcpsrv/pool.cc
clientSupported: Instead of doing:

if (client_class_.empty()) {
    match = true;
} else if (client_classes.contains(client_class_)) {
    match = true;
}

you could simply do:

bool match = (client_class_.empty() || classes.contains(client_class_));

=> yes but do you believe it is clearer? I know some people who do not like direct boolean expressions
(even it this case both are 100% equivalent).

I believe it is much clearer. In fact, I never saw similar construct in Kea code. Maybe I saw:

 if (client_class_.empty() || client_classes.contains(client_class_)) {
    match = true;
 }

but enumerating conditions and doing the same thing for each of them doesn't make any sense to me.

src/lib/dhcpsrv/pool.h
I don't like the names of deferClientClass and getOnDemandClasses as pointed out earlier for network.h

=> noted in .cc.

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
dependentForwardError: the description of this test says that it checks that forward dependencies will not parse, but in fact it merely tests that the class with a reference to a non existing class will cause parsing error. So, this is not really a forward reference case error. Either description or a test should be fixed.

=> not existing -> not yet existing? In any case the test must be clarified.

src/lib/eval/eval.dox
Regarding the new paragraph:

Parameters to the isc::eval::EvalContext class constructor are the universe to choose between DHCPv4 and DHCPv6for dependent expressions, and a closure which checks if a client class is already known used by the parser to accept only already known or built-in client class names in client class membership expressions. This closure defaults to accept all client class names.

Extraneous "known" or "used" in here: "client class is already known used"

The first sentence is unclear to me. Specifically I don't understand the connection between the universe: DHCPv4 and DHCPv6 and "dependent expressions". Wouldn't it be enough to say

Parameters to the isc::eval::EvalContext constructor are the universe (DHCPv4 or DHCPv6), and the closure ...

src/lib/eval/eval_context.h
I suggest that the typedef is used for the "check_known closure", instead of repeating the std::function template in multiple places.

=> I'll look at it and try to improve it.

Thanks for the review. I stopped before going too far from the initial target but I am afraid some explanations were lost or not completed. I'll try to make this easier to follow so #5549 will fit inside this naturally.

comment:25 Changed 6 months ago by fdupont

I fixed most obvious defects in the doc (but not all) and changed the names (e.g. eval-on-demand became eval-on-demand). BTW thanks for the "required" idea. I regenerate parsers and checked that tests pass. So with be35405829 you have something usable but of course not ready for another review...

comment:26 Changed 6 months ago by fdupont

  • Add Hours to Ticket changed from 12 to 14
  • Owner changed from fdupont to marcin
  • Total Hours changed from 12 to 14

I believe I addressed all comments. Ready for another review. BTW the doc includes not yet implemented known/unknown support.

comment:27 follow-up: Changed 5 months ago by marcin

  • Owner changed from marcin to fdupont

I have reviewed your latest changes. Thanks for renaming parameters. I think these names are much better and intuitive.

I applied some fixes, mostly in the documentation, so please pull first.

doc/examples/kea4/classify2.json
The class "second_subnet" is evaluated only if required, but none of the subnets in the provided examples require this class.

doc/examples/kea6/classify2.json
The class "second_subnet" is evaluated only if required, but none of the subnets in the provided examples require this class.

Copy paste error: "DHCPv4 server..."

doc/guide/classify.xml
I applied some minor corrections in this file, so please pull.

I can't process the following statement:

      Classes with matching expressions using directly or indirectly on
      the KNOWN builtin class and not marked for only when required
      evaluation are processed in the order they are defined in the
      configuration: the boolean expression is evaluated and when it
      returns true ("match") the incoming packet is associated to the
      class.

Would be clearer to mention what "resources" mean here:

      If needed, resources from pools are assigned, possibly based on the
      class information when some pools are reserved to class members.

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml
I applied some minor corrections in these files, so please pull.

src/bin/dhcp4/tests/classify_unittest.cc
I am not insisting on it but new tests server1Telephone, server1Computer etc. are sufficiently similar to be enclosed within a test function.

Same with "precedence" tests, same as you did in "shared_network_unittest.cc"

src/bin/dhcp6/tests/classify_unittests.cc
Would it be possible to use dhcp6_client test class for the new tests (e.g. precedence) like in the DHCPv4 case?

comment:28 in reply to: ↑ 27 Changed 5 months ago by fdupont

Replying to marcin:

I have reviewed your latest changes. Thanks for renaming parameters. I think these names are much better and intuitive.

=> yes and this greatly improved the code quality (needed as I have the known/unknown stuff to add).

I applied some fixes, mostly in the documentation, so please pull first.

=> pull and reviewed.

doc/examples/kea4/classify2.json
The class "second_subnet" is evaluated only if required, but none of the subnets in the provided examples require this class.

doc/examples/kea6/classify2.json
The class "second_subnet" is evaluated only if required, but none of the subnets in the provided examples require this class.

=> fixed.

Copy paste error: "DHCPv4 server..."

=> fixed. In fact I copied doc/examples/kea6/classify.json so I fixed it too.

doc/guide/classify.xml
I can't process the following statement:

      Classes with matching expressions using directly or indirectly on
      the KNOWN builtin class and not marked for only when required
      evaluation are processed in the order they are defined in the
      configuration: the boolean expression is evaluated and when it
      returns true ("match") the incoming packet is associated to the
      class.

=> I fixed it too:

      Classes with matching expressions using directly or indirectly
      the KNOWN builtin class and not marked for later ("on request")
      evaluation are processed in the order they are defined in the
      configuration: the boolean expression is evaluated and when it
      returns true ("match") the incoming packet is associated to the
      class.

what is almost the same text than for the first evaluation pass.
BTW if it is not clear enough I propose to address it in known/unknown ticket
as of course it is not yet implemented.

Would be clearer to mention what "resources" mean here:

      If needed, resources from pools are assigned, possibly based on the
      class information when some pools are reserved to class members.

=> changed into "addresses and prefixes"

src/bin/dhcp4/tests/classify_unittest.cc
I am not insisting on it but new tests server1Telephone, server1Computer etc. are sufficiently similar to be enclosed within a test function.

Same with "precedence" tests, same as you did in "shared_network_unittest.cc"

=> I am an addict of emacs cut & paste followed by a query replace... too old to change.

src/bin/dhcp6/tests/classify_unittests.cc
Would it be possible to use dhcp6_client test class for the new tests (e.g. precedence) like in the DHCPv4 case?

=> I don't understand as it already uses the client class. IMHO it is another case of please factor the code and this time the cut & paste abuse was not mine...

comment:29 Changed 5 months ago by fdupont

Time to work on the ChangeLog. Things to cite:

  • order changed from alphabetical to configured
  • new member('<class>') expression
  • only-if-required flag
  • require-client-class

comment:30 Changed 5 months ago by marcin

Your changes look good, but I note that it might be now hard to merge it and resolve the conflicts. The change log could be something like:

"Significant improvements to client classification introduced. The order of classes evaluation has changed from alphabetical to the order of appearance. New 'member' expression and the new 'only-if-required' and 'require-client-class' parameters controlling the scope of the class have been introduced".

comment:31 Changed 5 months ago by fdupont

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

Merged. Closing...

Note: See TracTickets for help on using tickets.