feat(kt-devnet): Option to clean up existing enclave#15063
feat(kt-devnet): Option to clean up existing enclave#15063
Conversation
kurtosis-devnet/README.md
Outdated
| ### Existing enclave already running | ||
|
|
||
| Set the `CLEAN_KT_DEVNET` envvar to `true` to make any `just devnet SPEC` based recipe stop and remove the existing | ||
| enclave prior to attempting startup. | ||
|
|
There was a problem hiding this comment.
Why wouldn't this just be the default given things don't work if there's an existing devnet?
There was a problem hiding this comment.
Things do work in some cases, but curious what your things are that don't. For example you can re-run the NAT suite without re-starting the devnet, and that's usually preferred for the feedback loop to be shorter.
There was a problem hiding this comment.
Seems like there should be a way to just run tests rather than restart devnet and run tests then? I'm just trying to get to a point where just interop-devnet works because right now it fails far more often than it works. If it's supposed to work even if an existing chain is running we should just fix that to be reliably. If it's meant to stop and rebuild the chain then we should make it always explicitly stop. I'm not really sure what it's expected to do right now to be honest so not sure which way to go on that.
But to me having to set a magic env var is just another magic incantation people have to learn to make things work when it should just work in the first place.
There was a problem hiding this comment.
Flipped the logic around. Now just *-devnet's default behavior is to attempt to clean an existing enclave, whereas op-acceptance-tests' usage of it skips the attempt to clean the devnet by default.
Should line up more with what folks are expecting there - faster test iteration with not tearing the whole devnet enclave down every time, and when directly using the devnet launcher, the old enclave is assumed to be trash (since a new devnet is explicitly being started.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #15063 +/- ##
============================================
- Coverage 46.19% 31.29% -14.90%
============================================
Files 1171 3 -1168
Lines 100291 131 -100160
============================================
- Hits 46325 41 -46284
+ Misses 50628 80 -50548
+ Partials 3338 10 -3328
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
clabby
force-pushed
the
cl/kt-clean
branch
2 times, most recently
from
d7093da to
731b202
Compare
sigma
left a comment
sigma
left a comment
There was a problem hiding this comment.
If that helps for the time being let's merge it.
That being said, can we define when this should be removed?
I don't think the expectation should be that we need to recreate devnets that often.
My understanding is that one of the reasons people insisted on using Kurtosis in the first place was to leverage its declarative aspirations and get to a more incremental story. So it'd feel odd to durably expect the opposite :)
| if [[ -z "$SKIP_ENCL_CLEAN" ]]; then | ||
| # clear out any existing enclaves with the same name. | ||
| # this can fail and it doesn't matter. | ||
| kurtosis enclave rm "$ENCL_NAME" --force 2&>/dev/null || true |
There was a problem hiding this comment.
I'm not sure it'll help all that much.
In scenarios where re-deploying the enclave in-place doesn't work, there's a decent chance that kurtosis enclave rm won't work either: typically it's not the enclave itself that's the problem, but the surrounding kurtosis/docker environment
There was a problem hiding this comment.
Interesting. I'm under the impression that this command also nukes the containers. Is that incorrect?
e: I just tested it out locally, and it did.
| fi | ||
| export ENCL_NAME="$DEVNET_NAME"-devnet | ||
|
|
||
| if [[ -z "$SKIP_ENCL_CLEAN" ]]; then |
There was a problem hiding this comment.
For consistency (with KURTOSIS_PACKAGE for example) I'd rather use a just variable (potentially initialized from the environment) than an environment variable
scharissis
left a comment
scharissis
left a comment
There was a problem hiding this comment.
The op-acceptance-tests change looks good to me.
An overall nitpick: I see no advantage in shortening the environment variable name and so I suggest we use the full SKIP_ENCLAVE_CLEAN which is more obvious.
opgitgovernance
added
the
S-stale
|
This pr has been automatically marked as stale and will be closed in 5 days if no updates |
5 participants
Overview
Adds an optional envvar to the
just devnet SPECrecipe base that automatically attempts to clean up an existing enclave with the same name prior to starting if set. Failures are silent in the event that the envvar is set, but the enclave doesn't exist already.