Add hooks and other improvements #1257
Conversation
|
@etene beside of |
BrianBaboch
force-pushed
the
add-hooks-and-other-improvements
branch
from
d19d44c to
abfc20a
Compare
BrianBaboch
force-pushed
the
add-hooks-and-other-improvements
branch
from
abfc20a to
1485932
Compare
|
@BrianBaboch if you could replace |
as defined in the RFC
BrianBaboch
force-pushed
the
add-hooks-and-other-improvements
branch
from
1485932 to
2c658c6
Compare
| # "dev", | ||
| # lease.interface, | ||
| # ) | ||
| LOG.info('Adding %s/%s to %s', lease.ip, |
There was a problem hiding this comment.
Not for this PR, just an idea for some future refactoring — go through the code and make all the logging in one single parsable format, be it golang key=value or json or anything else, but that could have single handler in the log collection stacks like loki/grafana or victorialogs. (looking is there any "make a ticket from this comment" :)
There was a problem hiding this comment.
We do that in production, but it does not necessarily entail going through all logging calls. We use the "logfmt" format with a custom logging formatter and that does the trick, we can export them to loki.
Since pyroute2 is a library, i believe logging format should be up to the callers anyway.
| LOG.error('Lease does not set the router option') | ||
| return | ||
| LOG.info('Removing %s as default route.', lease.default_gateway) | ||
| async with AsyncIPRoute() as ipr: |
There was a problem hiding this comment.
Again, not a request to change the code.
Would we benefit if I add an option like "idempotent" (default False) to AsyncIPRoute — that would ignore EEXIST for add and ENOENT for del calls?
Nope, ignore that. I went through possible situations and it looks like such behaviour would contribute more to problems than solutions.
There was a problem hiding this comment.
I think the most useful thing we could do in that regard would be to raise specific exceptions for errno codes, and make them inherit NetlinkError. That way, we wouldn't break any existing code while still allowing callers to catch specific exceptions instead of comparing the error code manually.
|
Are we ready to merge it? |
No description provided.