Make ping endpoints deterministic #335
Conversation
Kuba314
force-pushed
the
ping-endpoints
branch
3 times, most recently
from
ee7c884
to
5589bab
Compare
just a note on this commit... you're correct in the logic of how this CURRENTLY works, but it USED to be that this parameter was in BaseEnrt and so ipsec supported perf_reverse as well... i'm not sure why the refactor changed this but it could be seen as a bug.... HOWEVER.... we don't actually run any reversed ipsec scenarios so i guess this is fine for now and we can add the perfreverse mixin to this once we actually want this functionality back. |
|
@olichtne Passing |
lnst/Recipes/ENRT/helpers.py
Outdated
| def ip_version_string(ip_address: BaseIpAddress) -> str: | ||
| return "ipv4" if isinstance(ip_address, Ip4Address) else "ipv6" | ||
|
|
||
| for ip_version in ip_versions: |
There was a problem hiding this comment.
these loops should imo be inverted - this organization will re-sort the endpoint_pairs based on the collection of ip versions.
imo a function filter(condition, list) should not change the sort order of the list, only skip the items which don't fit the condition.
There was a problem hiding this comment.
This was intentional. I felt that first filtering by IP type, then doing product, was much cleaner than doing a product over all IPs of both endpoints end returning only endpoint pairs where both IPs are of the same type.
We shouldn't rely on the returned endpoints to be sorted in any way. Should I switch to first doing product, then filtering? Is there a good reason for it?
There was a problem hiding this comment.
this function doesn't do the product of endpoints though... it already accepts the list of endpoints and then returns a filtered list of endpoints. so it serves as a generic filter function for a list of objects. and imo that sort of filtering should be STABLE wrt. how the input list was ordered.
There was a problem hiding this comment.
Ah, I thought we're talking about the ping_endpoint_pairs function and not filter_ip_endpoint_pairs. Sure, I agree the filter should be stable.
The reason for this is to display ipv4 and ipv6 endpoints one after the other in results/logs since all are run in parallel, but this ordering is already done in ping_endpoint_pairs. I'll change the filter to be stable.
Kuba314
force-pushed
the
ping-endpoints
branch
from
3c64edb
to
9ba38eb
Compare
|
Up-to-date quick-check job: J:8466627 |
|
New quick-check job: J:8469963 (had an error in my UUID-picking script, which picked tests with no thresholds instead of tests with thresholds, this time it should fail on threshold indexes). |
this interestingly failed for 2 tests: for the GreTunnelRecipe:
for the GeneveTunnelRecipe:
i find it interesting that it's |
|
@olichtne only |
|
Running tests again, this time properly in separate jobs for rhel 8 and 9:
Edit: I cancelled the jobs, because we decided to first refactor our internal threshold-index handling. This PR is blocked until that's done. This might take a while. |
Kuba314
force-pushed
the
ping-endpoints
branch
from
9ba38eb
to
66a2296
Compare
|
postponed until we completely get rid of result index usage for baseline evaluation |
Ipsec*Recipes don't inherit from PerfReversibleMixin and thus don't even accept the perf_reverse argument.
It makes sense for this method to be a generator since it's in the name.
This just makes the class implementation shorter.
This function yields only IP endpoints of IP type requested by the ip_versions argument.
ENRT/helpers included EnrtConfiguration from BaseEnrtRecipe which itself imports ENRT/helpers creating a cyclic import.
(instead of reversing PingConfs)
AFAICT, SimpleMacsecRecipe's generate_ping_configurations differed from BaseEnrtRecipe's implementation only in the fact that it did not actually implement ping_bidirect. Defining ping endpoints is enough.
This parameter was used to ensure that recipes unable to run multiple pings at the same time didn't do so. This will be handled per-recipe's generate_ping_endpoints() configuration where each recipe now decides which endpoint pairs it wants to ping-test in parallel.
This ensures that Ipsec*Recipes don't ever run with `ping_bidirect=True` instead of logging a debug message and throwing out parallel endpoint tests.
Defining generate_ping_endpoints() is enough. But we can't use the ip_endpoint_pairs helper function, because we have to respect the selected IPs from the subconfiguration and not just take all available ipv4 and ipv6 addresses as the helper function does.
We are returning lists, we should typehint the methods as such. Previously we generated collections, which are easier to put together, because they're covariant (`x: Collection[Base] = [Derived()]` is possible). Now this is not needed and can be achieved elsehow.
We have no use-case for testing ping endpoints sequentially.
`functools.permutations` generate even reversed endpoints (both AB and BA). This should be controlled by `BaseEnrtRecipe`'s `ping_bidirect`.
Kuba314
force-pushed
the
ping-endpoints
branch
from
66a2296
to
6297046
Compare
Description
Continued work on endpoints started in #318. Ping endpoint IPs are now taken directly from configured IPs and not from
ips_filter(), + some other changes.Previously sequentially run pings are now run in parallel.
Tests
Early testing job of all distinct production-ready recipe,perf_stream types with thresholds: J:8445495
Reviews
@LNST-project/lnst-developers