Canonically determine local vs remote hosts/ips #2542
Conversation
portante
added
enhancement
Agent
fio
There was a problem hiding this comment.
Looks like a good start.
My principal reservation is that the netifaces package is not a "pure-Python" package (it of necessity contains platform-specific code) and does not currently have a maintainer, not to mention the fact that it's not available (as far as we know) as an RPM. So, while it's a very cool package, we need to ensure that our dependency on it is highly encapsulated, so that we can rip it out if needed. (E.g., if we have to, we could replace it with a small set of heuristics.)
portante
force-pushed
the
is-remote
branch
2 times, most recently
from
1dabe45 to
4ca8579
Compare
portante
force-pushed
the
is-remote
branch
4 times, most recently
from
4f0fb46 to
c29e387
Compare
There was a problem hiding this comment.
- The mocking for
LocalRemoteHostshould be done in some other way. -
pbench-is-remoteshould have a proper docstring. - I think we should reconsider whether/how we work with all of the target addresses (when there is more than one); and, I think we should give due consideration to what the function should return in the "failure case" (i.e., if we get an ambiguous result).
- I think I might have found a missing
else. - Consider renaming things from "remote" to (not) "local".
- Consider DRYing out a command invocation.
- Why use symlinks for the test completion status files? (If one changes, they aren't all going to change...are they?...so they shouldn't be coupled to each other.)
Beyond those, there are a dozen smaller items.
There was a problem hiding this comment.
I've rebased and updated this to handle the subjectively "biggest" issues:
- IPv6 support (non-text comparisons);
- Changing the sense of the test to explicitly support the fact that we can't/don't really test for "remote" but whether a host matches a known local address.
There are a few other issues worth considering here; but I'd also like to get this in because my non-DNS tool meister changes require it. (Which is why I "stole" it from Peter after it's been languishing in the GitHub closet since November...)
|
Argh; bug revealed by Jenkins IPv6 configuration, which is interesting. I think I need to try to mock |
There was a problem hiding this comment.
The new changes appear to have resolved most of my previous concerns. I have a few new comments below. And I still have some conversations from earlier passes:
- I think the mocking should be done in a different way. (@dbutenhof, and I discussed some possibilities -- my current favorite is to have the class to define both the real methods and mocks for them, and to bind the appropriate definition to the name at load-time.)
- There's the question about the missing else in the fio script
- My concern about using symlinks for the test completion status files
Yeah, just needed to be dusted off (rebased); of course now it's out of date again! |
|
Given that we can do a Squash-and-merge after we get everything approved, I'm no longer inclined to see the author do a rebase if it's "just to bring the branch up to date with main". Granted, there are exceptions to that: if the reason for rebasing is another commit to a dependent or related area, we should have the author rebase so that we get the two new changes tested together before they are merged; and, obviously, if there are merge conflicts, those are probably best resolved by the author. But, if the reason for the rebase is otherwise unrelated changes, particularly if the intention of the PR is to merge a single commit, then I think it produces less overhead if we do the Squash-and-merge at the end, rather than having the author handle it and undergoing an additional round of approvals. (Likewise, if the contents of the PR are to be merged as a series of commits, then the author needs to do the squashes, but if the branch is already properly arranged, we can do a Rebase-and-merge without requiring an additional round of approvals.) |
The new class, `LocalRemoteHost` allows one to construct an object which offers an interface, `is_remote(<host name or IP address>)`, returning a boolean `True` or `False` answering that question.
This adapts the new `LocalHostRemote` class for use with the orchestration of Tool Meister's remotely. Because of the nature of the legacy tests, we need to endow the agent utils module with knowledge of the test environment.
1) Handle IPv6 localhost comparisons
2) Reverse the sense of the test from 'is-remote'
to 'is-local' because strictly speaking that's
the condition we know how to test.
We only allow a single `--client` value
corresponding to localhost, so there's
no need for the `do_local_client` logic;
in addition, it used the ${client} loop
local value outside the loop.
You're not wrong; however,
|
4 participants
We need to properly determine whether a given address for a host, IP or name, refers to the local host or to a remote host.
In all the case where it is used (tool registration, Tool Meister sub-system, bench-scripts (
pbench-fioandpbench-uperf, primarily), we need to record at the time a bench-script is run which address is local vs remote.We also need to make sure that we don't register tools for the same local host under different names.
We also need to ensure bench-scripts, i.e.
pbench-uperfandpbench-fio, know the difference between local and remote hosts to avoidsshoperations where possible.