dist: Add distro verify tool #17380
Conversation
repokitteh-read-only
bot
added
the
deps
|
coverage is here https://storage.googleapis.com/envoy-pr/17380/tooling/index.html |
phlax
force-pushed
the
dist-add-verify-util
branch
from
1ec3bd4 to
555a6f9
Compare
phlax
force-pushed
the
dist-add-verify-util
branch
24 times, most recently
from
d5c98c2 to
60763c7
Compare
phlax
force-pushed
the
dist-add-verify-util
branch
10 times, most recently
from
ce4491f to
39fc9db
Compare
|
@htuch this is the runner for the distrotest lib that we landed previously |
|
momentarily reWIPping to rebase and update with newer main code... |
Signed-off-by: Ryan Northey <ryan@synca.io>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good, just a few comments/questions.
/wait
|
|
||
| async def run_test(self, name: str, image: str, package: pathlib.Path, rebuild: bool) -> None: | ||
| """Runs a test for each of the packages against a particular distro""" | ||
| if self.exiting: |
There was a problem hiding this comment.
The need to manual check for exiting here feels fragile.
There was a problem hiding this comment.
in the scheme of things this is defensive, and not strictly required
self.exiting is True in response to KeyboardInterrupt - which triggers a cleanup, and this opens a very brief window where other coroutines can still be running - this basically prevents it starting a new test during that window, which would send a notification to the docker api to build or start a container
i think we can ~safely remove, but in the rare occasion that this is hit, it would just make keyboard exiting slower
i think i would rather keep, but ambivalent enough to change it
There was a problem hiding this comment.
i have left for now, lmk if that is a blocker
There was a problem hiding this comment.
Is there no way to add a co-routine start hook to do this more generically? I'm just a little concerned that we have to do this by hand. The reason why it is done makes sense.
There was a problem hiding this comment.
if you mean some kind of mechanism to prevent any new coroutines being fired, that would be closing the loop
we dont want to close the loop here, we want to let any existing coroutines run or get caught and we want any cleanup to occur (in fact we sometimes even need to start a new loop to do so)
There was a problem hiding this comment.
I'm just a little concerned that we have to do this by hand. The reason why it is done makes sense.
yep, to summarize - we are running asychronously and communicating with a rest API that uses a lot of system resources
There was a problem hiding this comment.
as a general point working with asyncio its better to have a routine/loop that knows how to exit gracefully when it should, than to have routine that forces cancellation/closure on exit
a good example of this is the submit_task in the parallel pr
in my initial implementation in its cancel method i had
self.submit_task.cancel()
try:
await self.submit_task
except asyncio.CancellationError:
# not always hit - deps on timing of tasks created by submit_task
passNothing i could do seemingly would prevent it from sometimes raising the cancellation error
Then i added a closing lock and told the submit_task to bail gracefully when locked and replaced above code with simply
await self.submit_taskand everything was happy
There was a problem hiding this comment.
one thing im wondering from this discussion - whether in the case of async checkers the exiting prop should use an asyncio.Lock
the asyncio locks are not thread-safe (and dont need to be) so its not immediately clear what the advantages of using the asyncio sync primitives are
from my testing it seems like they offer a better guarantee of synchronization in terms of ordering of coroutine execution - so my ~guess is that they are run with higher priority than normal tasks
they can also be used synchronously so we could theoretially just use it for all checkers albeit slightly differently altho im a little uncomfortable with using anything asyncio in non-async code
There was a problem hiding this comment.
There was a problem hiding this comment.
actually Lock cant be used synchronously, just queue
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo the exiting discussion.
| @@ -0,0 +1,227 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
| # | |||
There was a problem hiding this comment.
Nit: Python purist might ask for docstring syntax here. I don't feel that strongly (I think I've been using docstring elsewhere FWIW).
There was a problem hiding this comment.
ive done it this way in all of the other modules - i forget the reason why now - but this is at least consistent with them (copypasta)
as an aside - ive only included -h and deps instructions up here - i thought about this quite a bit - and came to the conclusion that to do anything else for all these runners/checkers would create a maintenance/review headache
3 participants
Commit Message: dist: Add distro verify tool
Additional Description:
provided with following arguments:
it will test each of the debs/rpms against the relevant distros
the format for the yaml is:
the tarball should have a layout like:
which will be created with bazel packaging rules once we land the relevant code
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]