Send kea-dev mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.isc.org/mailman/listinfo/kea-dev
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of kea-dev digest..."
Today's Topics:
1. Re: Client classification design (Tomek Mrugalski)
2. Re: Client classification design (Tomek Mrugalski)
3. Re: Client classification - discussion summary (?)
(Tomek Mrugalski)
4. Re: thoughts on #3780 (Stephen Morris)
5. Fwd: Re: thoughts on #3780 (Thomas Markwalder)
----------------------------------------------------------------------
Message: 1
Date: Fri, 23 Oct 2015 14:35:22 +0200
From: Tomek Mrugalski <[email protected]>
To: Marcin Siodelski <[email protected]>, [email protected]
Subject: Re: [kea-dev] Client classification design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
Suggested items that were addressed are removed for clarity.
On 22.10.2015 12:30, Marcin Siodelski wrote:
> "Our current (already written) implementation... " Suggest adding a
> timestamp because as the time goes by this will refer to the past
> situation.
Added Sep. 2015. It's late October already, but the classification code
hasn't changed in more than a year. It's somewhat likely that first new
code will be merged before end of Oct, so it would be a bit ambiguous.
Hence past month.
> G.3 - It would be clearer to say "DHCP Options returned to the
> client are the combination of global options, subnet specific options
> and those specific to the classes that the client belongs to"
>
> I also think for G.3 that it should say something about
> class-specific options taking precedence over global and subnet
> specific options.
Updated. There's a separate section that discusses this matter further.
> G.5 - This requirement is unclear as it stands. In particular
> "expression" is ambiguous. If it was explained well enough, the
> second part of this require wouldn't be needed. Maybe add something
> that expression in general consists of several conditions with bool
> logic applied in-between. Or something like that.
Reworded. If you're still unhappy, please provide a text.
> G.7. - I don't understand why the requirement is repeated in parens
> and what "can apply" means.
There was something lost here. There is no "can apply" on neither the
requirments or design pages.
> G.8 - It is going to be hard to measure "minimal dependencies". I
> recommend you simply remove the part about minimal dependencies. I
> guess nobody would add dependencies that are not required by the
> library implementation.
I disagree. People may try to push this thing into lidhcpsrv, because
that's where it will be used first. I'd like to keep that requirement.
> G.10. - I suggest rewriting it to say that "The administrator must
> be able to define arbitrary number of classes". I also suggest
> removing the part about issuing warning etc. I don't think we should
> even consider this warning at this point, because it is hard to
> determine upfront what number of classes is already worth a warning.
> I'd leave it up to the administrator of a system.
The text says "we may consider". That doesn't mean we will do it. It
will be a good sanity check against people who would come up with ideas
like defining an unique class for each subscriber.
> E.1, E.2. - I suggest that you expand on "extracting option values".
> Is it to say: "Extracting full option payload as an input for
> expression"? As opposed to extracting individual option fields?
Added clarification.
> E.3. - I think this requirement could be clearer as to how the
> expression would look like for the option with multiple fields,
> would this be comma separated format or something else. This may be
> considered implementation detail really but I rise this question
> because you used example of "MSFT" string which is normally used in
> the substring operation context. Simply don't know if this
> requirement equally applies to the case of equality operator.
The text is clear enough in my opinion. We should not deliberate any
further what a constant string means.
> E.5. I presume the equality operator is supported in the sense that
> you can compare the payload of the whole option to the string or
> hexstring. Correct?
It is supported in the sense that it compares two evaluated values (two
strings), no matter where those values (strings) came from.
> I am confused with the distinction between the "General" and
> "Functionality" requirements. Although, it is not a big deal because
> the essence is in the requirements themselves but grouping into
> distinct sets may make requirements clearer or may make them actually
> harder to read.
Merged those two lists.
> I don't see anything mentioned explicitly about sub-options here. Do
> we need additional requirements to cover the classification based on
> sub-options?
Not in phase 1.
> I see further on that you're thinking about using the output of
> toText() function. But there are some issues with this output being
> compared to a fixed string, which I mention below. There is also an
> issue that if someone creates a configuration file which would use
> the equality operator, the future change in the toString()/toText()
> implementation would make his configuration incompatible.
The alternative is to implement toString() for all option formats we
currently have in 1.0. Good luck with that. We'll cover the possible
configuration incompatibilities in the release notes.
> A couple of comments about the Design document follow.
>
> "Configuration" There are three proposals but the design doesn't
> include the final decision which of them will be implemented. I also
> think that the examples should reflect the fact that we're planning
> to start with implementation of global classes, rather than classes
> nested within the subnet.
It has one syntax now that has class definitions on a global level
(those can optionally include options that would apply to all subnets).
Also, options defined on subnet level can be extended can be class
specific (i.e. reserved for specific class).
> "Parsing"
>
> I am not sure I understand that: "This list is not complete. We will
> likely need a dedicated class for extracting information from
> Vendor-Identifying Vendor Class (124) and Vendor-Identifying
> Vendor-Specific Information (125) options."
>
> What list does it refer to? Also, why would you need additional
> class for those options? Aren't you planning to introduce a virtual
> toString() function which will be implemented within existing class
> hierarchy?
Clarified.
> "Interaction with Option classes" I am trying to understand the code
> that will be introduced into Option class and derived classes. So,
> you're planning to evaluate using the string value carried within the
> option, e.g. "MSFT". In the requirements you also mentioned that
> evaluation using the hex representation is possible. So, technically
> you're going to implement the toString() function within Option
> class, returning the string OR hex string? Would this be controlled
> using some sort of parameter passed to toString()? I presume it would
> be something like toString(const bool use_hex) ?
You're reading too much into this. The requirement states:
"Constant expression specified as hexstring MAY be supported.". Note
that it's a MAY for phase 1, so we'll likely skip it anyway. This will
only apply to constant strings. Instead of "MSFT" it could be possible
to write 0x4D534654.
> "Expected Tickets"
>
> About task #4. After implementing this ticket all instances of the
> classes derived from Option would return toText()? So this change
> would simply be to implement the following function in the Option
> class?
>
> std::string Option::toString() const { return (toText()); }
>
> Note that some specialized options would return data in different
> formats and they also include indentation and new line characters.
> Comparing such output of toText() for equality wouldn't probably do
> any good. It should work fine for substring, though.
Yes, that's the plan. If you have better idea that could be implemented
in the time constraints we have, please speak up.
> Another related question. How this will satisfy the requirement to
> compare option against hex string?
That's easy. All operations are done on strings (we do not try to invent
strong typed language here. We sort of have that in ISC DHCP and it's
more than enough). Hexstring will be converted to normal string during
Token parsing, e.g. the configuration has 0x4D534654, which will be
parsed as 4 characters long constant string.
> About #5. Are we actually going to use any Yacc or Bison for 1.0?
Yes. With only == operator we could possibly get away without it. With
substring (which takes 3 arguments) there's no sane way to do it without
proper grammar definition. You could try to come up with some sort of
boost-based regexp parser that would not be extensible and would have to
be thrown away in 2 months when the real complexity planned for 1.1
comes in. That doesn't fall into my definition of sane plan, so I
rejected it.
> About #15. I don't have such a strong opinion about the update to
> Developer's Guide. I agree we should aim for this but the world
> wouldn't collapse if we don't address it for 1.0. The world will
> collapse if we don't update the User Guide properly.
Oh no. We won't skip that one. It seems that we're doing everything here
by the book. We surely cannot skip such an important step as
documentation. It should be easy to do anyway. At this stage the design
is detailed enough that it should be mostly a copy-paste type of work.
Tomek
------------------------------
Message: 2
Date: Fri, 23 Oct 2015 14:42:41 +0200
From: Tomek Mrugalski <[email protected]>
To: [email protected]
Subject: Re: [kea-dev] Client classification design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
Points I agree with that were implemented completely were removed for
clarity.
On 16.10.2015 13:39, Thomas Markwalder wrote:
> First, let me commend Tomek for all the effort he has put into this
> and harassment he has endured for it ;).
Thanks. IMHO the amount of harassment is somewhat disproportionate to
the amount of time we have in 1.0 timeframe, but we can continue this
discussion as long as necessary.
> REQUIREMENTS:
>
> You have two paragraphs which begin with "Client classification can
> be a very complex feature. Due to scheduling constraints...". I don't
> think you need em both ;)
Removed.
> G3. I think this should be expanded similar to what Stephen had in
> the discussion document, where he stated the search path:
>
> "The search path for an option value is then:
>
> Subnet class-option-data matching the client class Subnet
> option-data Global class-option-data matching the client class Global
> option-data"
Added a new section about options precedence.
> We also need to state what happens if a client matches more than one
> class and they both specify the same option. Even if we say it is
> arbitrary.
Added clarification for that. Since we'll need to evaluate each class
anyway, we'll likely to go through their list iteratively. That means
that the options will keep the order they were specified in the config.
That also makes intuitive sense. The last one specified "wins".
> G10. Requirements should be stated as positives not negatives.
Why?
> "The system MUST support an arbitrary number of class defintions."
Ok, updated as suggested.
> I think we do want the ability to specify alternate or additional
> class options for a given class on a per subnet basis, which both
> options 1 and 3 provide.
>
> I tried both #1 and #3 in a test configuration and #1 was a bit
> uglier to read and write than #3, and I think #3 will be less effort
> to implement. Here's what the hybrid might look like:
Good proposal. I used it almost as is.
> I believe the tokens should be stateless. But rather than pass in a
> fixed number of tokens into evaluate(), I believe we should pass in a
> "value" stack. Each evaluate() implemenation pops off as many values
> from the stack as it requires to calculate its result. If there are
> not enough on the stack that's an error.
This should be caught at the configuration parsing stage. The tokens are
now stateless, so we actually have 2 stacks. One constant with tokens
and another (variable) stack with token values.
Tomek
------------------------------
Message: 3
Date: Fri, 23 Oct 2015 14:46:39 +0200
From: Tomek Mrugalski <[email protected]>
To: [email protected]
Subject: Re: [kea-dev] Client classification - discussion summary (?)
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
Hopefully this summarizes the discussion about the client classification
design. I responded to hopefully all comments.
Here's a brief summary of those changes:
- clarified option order (global, subnet, global class, subnet class,
host). The last one will not make it into 1.0. The one before it (subnet
class) may or may not make it into 1.0.
- explained what to do when client belongs to multiple classes and each
has the same option defined (only one will be sent, which class wins
will be determined by the implementation, but it will most likely be the
class that defined later in the config file.
- class configuration syntax agreed on (accepted Tom's proposal as is)
- evaluation process clarified. The Token stack is now stateless.
- renumbered the requirements, so the assigned numbers are continuous
- make it clear that the parser will be flex/bison based
- many smaller clarifications and corrections
If you really think that the design is not good enough and we have more
time to dedicate to further refinement, feel free to continue the
discussion. If not, let's move on to the ticket estimates.
Tomek
------------------------------
Message: 4
Date: Fri, 23 Oct 2015 14:15:25 +0100
From: Stephen Morris <[email protected]>
To: [email protected]
Subject: Re: [kea-dev] thoughts on #3780
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
On 22/10/2015 16:49, Thomas Markwalder wrote:
> One of the issues here is that subsequent MySQL api calls made on a
> connection which has been lost can core, so once we detect an issue we
> can't use that connection for anything other than looking at error
> values. Currently the MySqlLeaseMgr has no real defense against that.
>
> At first blush, the second option seems the proper thing to do.
> However, we have done a pretty thorough job of making sure database
> related exceptions, and even unexpected std::exceptions within our
> libraries, do not bring the server down. This means that propagating
> this new exception all the way out to a server's ::run() method, would
> require adding catch-rethrows to quite a few try-catch blocks. These
> blocks are in lots of places from allocation engine to
> TimeMgr::handleReadySocket(). Basically any place that can end up
> accessing the database. This seems like a pretty invasive thing to do.
>
> Unless we think there might be other conditions, unrelated to the
> database, that we wish to treat as "fatal" in the sense that we want the
> server to shut down, but in a more orderly fashion that calling exit().
> If that's true, having explicit catches for a generic class of
> isc::FatalException might be warranted.
I dislike calling exit() in the middle of the code, although this might
be a good short-term solution for Kea 1.0 - providing we revisit it
before Kea 1.1. Ultimately though, I think that creating an
isc::FatalException class is the way to go.
Ideally, all exceptions generated by library code should be derived from
std::exception. If this were the case, we could change all "catch
(...)" calls to "catch (const std::exception&)" to allow
isc::FatalExceptions (if not derived from the base class) to propagate
all the way to the top level. Unfortunately, Boost defines an exception
hierarchy that is not derived from std::exception, so if we were do
that, Boost exceptions could also leak to the top level.
If we were compiling with C++11, we could call the current_exception()
construct in the "catch (...)" clause to determine the thrown exception
type and take action accordingly. But we aren't, so we can't.
A possible alternative to calling exit() is to replace all "catch (...)"
clauses with:
catch (const isc::FatalException&) {
throw;
}
catch (...) {
:
}
I count 61 instances of "catch (...)" in Kea (74 if you include their
appearance in the unit tests), so this is a viable solution, albeit more
work than calling exit().
Ultimately I think that the best solution (and one requiring most
effort, so not suitable to do before the next release) is to revisit all
places where we catch exceptions and check what we really want to do. In
many cases we should probably just catch isc::Exception as these
exceptions will be from "throw" statements we have put inside the code
and so can anticipate.
Frequently though, we catch std::exception (or "..."). Exceptions
derived from this class include bad_alloc (can't allocate memory),
out_of_range (e.g. accessing outside the bounds of a vector) etc. Should
we be catching those? For example:
* bad_alloc means that Kea can't allocate memory. Theoretically we
could free up memory but in practice we don't and memory allocation
failure should be treated as fatal error condition. Attempting to
continue is likely to lead to a failure in an unrelated part of the code.
* out_of_range may be a valid exception to catch if we are using a
user-supplied value to access some data, although I would prefer that
the value was explicitly checked for validity before being used. If
some Kea code generates this through, it probably means that there is a
logic error somewhere. (Although if this is the design of the code, the
code should be specifically checking for std::out_of_range, not
std::exception.) Rather than try to continue with the current packet, we
should probably kill the program or effectively re-initialize - drop the
current packet and process the next one.
In many cases, catching only exceptions derived from isc::Exception will
be the right thing to do and other exceptions should be left to
propagate up through the stack.
Of course, there will be cases where all exceptions should be caught.
For example, the Hooks feature causes user libraries to be loaded. Kea
should protect itself from errors generated in that code, so the current
behaviour of logging user library-generated exceptions and dismissing
them is probably correct. But these cases are likely to be few and far
between.
> I am inclined to implement option #1 for 1.0. It might not be pretty
> but it is far less invasive than trying to thread the exception all the
> way through. In the end, what would we really gain by doing that?
> Granted, we would not all of our destructors but is this really a big
> issue? We could attempt to register some sort atexit() function but
> I'm not sure if it's worth it.
exit() will be fine. The operating system's process cleanup should
leave things in a consistent state, although if a destructor does
something like deleting a marker file, that of course won't happen.
Stephen
------------------------------
Message: 5
Date: Fri, 23 Oct 2015 10:08:30 -0400
From: Thomas Markwalder <[email protected]>
To: Kea Dev List <[email protected]>
Subject: [kea-dev] Fwd: Re: thoughts on #3780
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Sorry, my first reply went only to Stephen:
-------- Forwarded Message --------
Subject: Re: [kea-dev] thoughts on #3780
Date: Fri, 23 Oct 2015 10:07:55 -0400
From: Thomas Markwalder <[email protected]>
To: Stephen Morris <[email protected]>
On 10/23/15 9:15 AM, Stephen Morris wrote:
> On 22/10/2015 16:49, Thomas Markwalder wrote:
>
>> One of the issues here is that subsequent MySQL api calls made on a
>> connection which has been lost can core, so once we detect an issue we
>> can't use that connection for anything other than looking at error
>> values. Currently the MySqlLeaseMgr has no real defense against that.
>>
>> At first blush, the second option seems the proper thing to do.
>> However, we have done a pretty thorough job of making sure database
>> related exceptions, and even unexpected std::exceptions within our
>> libraries, do not bring the server down. This means that propagating
>> this new exception all the way out to a server's ::run() method, would
>> require adding catch-rethrows to quite a few try-catch blocks. These
>> blocks are in lots of places from allocation engine to
>> TimeMgr::handleReadySocket(). Basically any place that can end up
>> accessing the database. This seems like a pretty invasive thing to do.
>>
>> Unless we think there might be other conditions, unrelated to the
>> database, that we wish to treat as "fatal" in the sense that we want the
>> server to shut down, but in a more orderly fashion that calling exit().
>> If that's true, having explicit catches for a generic class of
>> isc::FatalException might be warranted.
> I dislike calling exit() in the middle of the code, although this might
> be a good short-term solution for Kea 1.0 - providing we revisit it
> before Kea 1.1. Ultimately though, I think that creating an
> isc::FatalException class is the way to go
>
> Ideally, all exceptions generated by library code should be derived from
> std::exception. If this were the case, we could change all "catch
> (...)" calls to "catch (const std::exception&)" to allow
> isc::FatalExceptions (if not derived from the base class) to propagate
> all the way to the top level. Unfortunately, Boost defines an exception
> hierarchy that is not derived from std::exception, so if we were do
> that, Boost exceptions could also leak to the top level.
>
> If we were compiling with C++11, we could call the current_exception()
> construct in the "catch (...)" clause to determine the thrown exception
> type and take action accordingly. But we aren't, so we can't.
>
> A possible alternative to calling exit() is to replace all "catch (...)"
> clauses with:
>
> catch (const isc::FatalException&) {
> throw;
> }
> catch (...) {
> :
> }
Actually it isn't just "catch (...)", we have to look at
catch(std::exception)
and/or catch(isc::Exception). The latter depending upon the class from
which
a new FatalException is derived.
> I count 61 instances of "catch (...)" in Kea (74 if you include their
> appearance in the unit tests), so this is a viable solution, albeit more
> work than calling exit().
>
> Ultimately I think that the best solution (and one requiring most
> effort, so not suitable to do before the next release) is to revisit all
> places where we catch exceptions and check what we really want to do. In
> many cases we should probably just catch isc::Exception as these
> exceptions will be from "throw" statements we have put inside the code
> and so can anticipate.
>
> Frequently though, we catch std::exception (or "..."). Exceptions
> derived from this class include bad_alloc (can't allocate memory),
> out_of_range (e.g. accessing outside the bounds of a vector) etc. Should
> we be catching those? For example:
>
> * bad_alloc means that Kea can't allocate memory. Theoretically we
> could free up memory but in practice we don't and memory allocation
> failure should be treated as fatal error condition. Attempting to
> continue is likely to lead to a failure in an unrelated part of the code.
>
> * out_of_range may be a valid exception to catch if we are using a
> user-supplied value to access some data, although I would prefer that
> the value was explicitly checked for validity before being used. If
> some Kea code generates this through, it probably means that there is a
> logic error somewhere. (Although if this is the design of the code, the
> code should be specifically checking for std::out_of_range, not
> std::exception.) Rather than try to continue with the current packet, we
> should probably kill the program or effectively re-initialize - drop the
> current packet and process the next one.
>
> In many cases, catching only exceptions derived from isc::Exception will
> be the right thing to do and other exceptions should be left to
> propagate up through the stack.
>
> Of course, there will be cases where all exceptions should be caught.
> For example, the Hooks feature causes user libraries to be loaded. Kea
> should protect itself from errors generated in that code, so the current
> behaviour of logging user library-generated exceptions and dismissing
> them is probably correct. But these cases are likely to be few and far
> between.
>
>
>> I am inclined to implement option #1 for 1.0. It might not be pretty
>> but it is far less invasive than trying to thread the exception all the
>> way through. In the end, what would we really gain by doing that?
>> Granted, we would not all of our destructors but is this really a big
>> issue? We could attempt to register some sort atexit() function but
>> I'm not sure if it's worth it.
> exit() will be fine. The operating system's process cleanup should
> leave things in a consistent state, although if a destructor does
> something like deleting a marker file, that of course won't happen.
>
> Stephen
> _______________________________________________
> kea-dev mailing list
> [email protected]
> https://lists.isc.org/mailman/listinfo/kea-dev
Ok, barring other opinions I will proceed with the quick-and-dirty, of
calling exit() upon "fatal" DB condition.
I am also adding this same fatal behavior to out Posgres backend.
I agree that we should pursue creating an isc::FatalException class and
altering catch() blocks to propagate it,
post 1.0. I'll create a ticket for this.
Unit testing RDBMs connectivity loss does present challenges.
1. In a user's environment, we have know way of knowing what else they
may be using their RDBMS server
for and should not attempt to start it and stop it at will.
2. The mechanism for starting and stopping the RDBMS is going to very
install site dependent.
As, Wlodek pointed out we could develop specific tests to run under
Forge, as that is an environment in which we have 100% control.
At the very least I will document manual test steps used to verify the
behavior.
Thanks,
Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.isc.org/pipermail/kea-dev/attachments/20151023/0e3f7c42/attachment.html>
------------------------------
_______________________________________________
kea-dev mailing list
[email protected]
https://lists.isc.org/mailman/listinfo/kea-dev
End of kea-dev Digest, Vol 19, Issue 10
***************************************
