-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare for using azure cleanroom in tests #214
base: main
Are you sure you want to change the base?
Conversation
@@ -43,37 +43,32 @@ setup: ## Setup proposals and generate an initial key | |||
|
|||
stop-host: ## 🏃 Stop the host | |||
@echo -e "\e[34m$@\e[0m" || true | |||
sudo lsof -t -i :8000 | xargs -r sudo kill -9 | |||
source ./scripts/ccf/sandbox-local/down.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These scripts force the usage of containers. So running KMS locally will no longer be possible. By the way, this is how I do my dev work. What are the limitations going forward?
In development you start frequently KMS. Containers have this persistent state behavior of you don't completely remove them. Meaning rebuilding of the container for every dev step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using CCF only from the container is indeed a choice we need to make carefully, my reasoning for supporting this choice are:
- It mimics how the real system will behave (i.e. Run CCF network somewhere else, and apply KMS code/policies/etc to it)
- It allows us to have a much simpler dev environment as you don't need to work on top of the CCF dev container or install CCF manually
- It makes running against KMS on different styles of CCF network trivial (sandbox locally, sandbox in aci, mCCF while it lasts, azure-cleanroom etc.)
I believe the considerations you raise aren't bad enough to outweigh the benefits for the following reasons:
- Persistent state: Using
docker compose down
cleans up for you, I've never had trouble with it - Rebuilding:
- Since the container image is just the official CCF dev image (i.e. no KMS code), it doesn't typically change while developing the KMS
- In my experience starting or recreating the container takes ~5 seconds, building the kms code often takes longer
- If you don't mind keeping the current KV/ledger of a running KMS this scheme even lets you update the code without stopping or recreating the CCF network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much work would it be to support Ronny's way of working as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible, we would need:
- A local environment with virtual CCF installed and all it's dependencies
- See the changes to devcontainer.json, requires developing inside the ccf/dev container or complex setup
- We can then add
/scripts/ccf/sandbox-local-non-container/
directory with up and down scripts- We would need a some extra code to handle the lifecycle stuff which docker compose does for us in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I see that as a pain, and I don't see any benefits vs just starting the off the shelf CCF container and treating it like every other use case of CCF
Motivation
When attempting to add azure cleanroom support, I noticed that the KMS code defines a "workspace" which is assumed to have the same structure as the workspace created when running sandbox.sh from
ghcr.io/microsoft/ccf/app/dev/${CCF_PLATFORM}:${TAG}
.Since the network created by azure cleanroom doesn't have this structure we need to update the code that has this assumption.
Changes