Improve DHCP IP address parsing speed for journal #687
Conversation
|
Hi @Horofic could you take a look at this PR? This fix makes sure |
| @@ -33,15 +33,16 @@ def detect(cls, target: Target) -> Optional[Filesystem]: | |||
| return None | |||
|
|
|||
| @export(property=True) | |||
| def ips(self) -> list[str]: | |||
| @arg("--dhcp-all", action="store_true", help="parse all syslog, messages and journal files for DHCP IP addresses") | |||
| def ips(self, dhcp_all: bool = False) -> list[str]: | |||
There was a problem hiding this comment.
Could you add a test to test this new behaviour?
There was a problem hiding this comment.
Added test in ab47074. It seems that there is a bug in the @arg decorator. The documentation states This decorator adds the ``__args__`` private attribute to a method or property., however it does not seem to add __args__ for this exported property.
There was a problem hiding this comment.
I think this is inherit to properties, so no arguments can be passed to it. This might be incorrectly be reflected in the documentation.
Also, I remembered a similar discussion in #133 where we talked about a similar change where we decided to move it to ips_dhcp in the meantime. It might be a good idea to move towards this again, to avoid the t.ip vs t.ips() inconsistency across OSes.
There was a problem hiding this comment.
Perhaps if the parsed argparse args are available somewhere in the self.target instance we could still check for provided arguments at runtime.
There was a problem hiding this comment.
I'll raise this discussion in the team and get back to you!
There was a problem hiding this comment.
We had a brief discussion and decided that's not an ideal solution. You can execute multiple functions, and each function can have it's own argument parser. There's no "one big" set of parsed arguments available at runtime, each plugin only gets to see its own set of parsed argument that matched its defined arguments.
For something like properties I think it'd be better to use dependency injection, but that's non-trivial to add and we rarely use properties anyway, so I fear it may be a bit of a waste to add support for that.
For this specific case we expect this to be resolved by the network interface rework that's been in the planning for a while, described in #774 ( epic:network interfaces plugin ).
An alternative to ips_dhcp to work around this issue in the meantime is to (ab)use the target configuration system. At the moment, this can be used by placing a file called .targetcfg.py in the same directory (or parents) of a target, and storing a variable like UNIX_IPS_DHCP = True in there. You can access this through target._config.UNIX_IPS_DHCP in the code. Note that this behaves like a module type so you may need to getattr. We're open to improvements to this config system by the way, e.g. adding support for environment variables (#771, #772).
Or revert back to ips_dhcp 😄 but both of these will be temporary workarounds.
There was a problem hiding this comment.
Those are some good options, will look into that. For now we're fine with leaving dhcp_all out of ips, as changed in a862b2e.
|
It also seems like to the |
Co-authored-by: Stefan de Reuver <[email protected]>
Those should now be fixed! |
Co-authored-by: Stefan de Reuver <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
==========================================
+ Coverage 75.21% 75.24% +0.03%
==========================================
Files 296 296
Lines 25422 25434 +12
==========================================
+ Hits 19121 19139 +18
+ Misses 6301 6295 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This PR aims to improve DHCP parsing logic introduced in #550. Unfortunately the previous approach makes interactive commands such as
target-infoslow. We have opted to only parse the first 10000 journal log messages for DHCP IP address leases by default. This can be extended with the flag--dhcp-all.