-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Allow defining meaningful hostnames on multi-tesseract network #206
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
base: main
Are you sure you want to change the base?
Conversation
04fb9a2
to
11a2173
Compare
Nice idea 👍 should we also validate whether service names are distinct? |
I think we might also want to restrict service names such that the corresponding urls are safe. |
Yes, good point. Will add this. |
11a2173
to
3f61ec2
Compare
CLA signatures confirmedAll contributors have signed the Contributor License Agreement. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
===========================================
- Coverage 76.21% 25.68% -50.54%
===========================================
Files 28 24 -4
Lines 3078 3002 -76
Branches 480 487 +7
===========================================
- Hits 2346 771 -1575
- Misses 519 2143 +1624
+ Partials 213 88 -125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is a surprisingly frustrating validation to perform. :(
Unfortunately the docker compose implementation has changed completely since then, and I couldn't find the corresponding validation logic in the golang codebase. What do people think about enforcing the regex
which I believe would be more restrictive than the docker-compose service name, and include only characters that can appear in valid host names. |
I think that would probably work just fine. If we run into cases that require less restrictive names, we can always relax the constraints later without breaking anything. |
@PasteurBot I have read the CLA Document and I hereby sign the CLA |
Thanks @johnbcoughlin. That looks like a useful feature. Is there a precedent for this feature in other applications / libraries (such as the Docker CLI, or |
Yes, I believe "service" is a very common name for this concept. Docker CLI or docker-py isn't the right level of abstraction though, we want to look at container orchestrators.
In e.g. Kubernetes or docker-compose you would be providing these strings as part of a yaml config, so there is no direct analogue for the CLI argument name "service-names". |
typer.Option( | ||
"--service-names", | ||
help=( | ||
"Comma-separated list of service names by which each tesseract should be exposed " |
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.
"Comma-separated list of service names by which each tesseract should be exposed " | |
"Comma-separated list of service names by which each Tesseract should be exposed " |
help=( | ||
"Comma-separated list of service names by which each tesseract should be exposed " | ||
"in the shared network. " | ||
"Tesseracts are reachable from one another at http://{service_name}:8000" |
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.
"Tesseracts are reachable from one another at http://{service_name}:8000" | |
"Tesseracts are reachable from one another at http://{service_name}:8000. " | |
"Not supported when using --no-compose." |
@@ -491,6 +502,11 @@ def serve( | |||
else: | |||
ports = None | |||
|
|||
if service_names is not None: |
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.
Let's check here if --no-compose
is given and error out with a helpful message.
if no_compose: | ||
if len(images) > 1: | ||
raise ValueError( | ||
"Docker Compose is required to serve multiple Tesseracts. " | ||
f"Currently attempting to serve `{len(images)}` Tesseracts." | ||
) | ||
if service_names is not None: | ||
raise ValueError( | ||
"Tesseract service names are only meaningful with Docker Compose." |
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.
"Tesseract service names are only meaningful with Docker Compose." | |
"Tesseract service names can only be set when using Docker Compose." |
if len(set(service_names)) != len(service_names): | ||
raise ValueError("Service names must be unique") | ||
|
||
print(service_names) |
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.
print(service_names) |
print(service_names) | ||
invalid_names = [] | ||
for name in service_names: | ||
print(name) |
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.
print(name) |
if len(invalid_names) != 0: | ||
raise ValueError( | ||
"Service names must contain only alphanumeric characters and hyphens, and must " | ||
f"not begin with a hyphen. Found invalid names: f{invalid_names}." |
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.
f"not begin with a hyphen. Found invalid names: f{invalid_names}." | |
f"not begin with a hyphen. Found invalid names: {invalid_names}." |
print(name) | ||
if not re.match(r'^[A-Za-z0-9][A-Za-z0-9-]*$', name): | ||
invalid_names.append(name) | ||
if len(invalid_names) != 0: |
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.
if len(invalid_names) != 0: | |
if invalid_names: |
invalid_names = [] | ||
for name in service_names: | ||
print(name) | ||
if not re.match(r'^[A-Za-z0-9][A-Za-z0-9-]*$', name): |
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.
Not even underscores?
# I get a "tesseract_core has no attribute Tesseract" error from this?? | ||
#"import tesseract_core; tesseract_core.Tesseract.from_url(\"http://t-9000:8000\").health()" |
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.
This is expected, see comment thread
# I get a "tesseract_core has no attribute Tesseract" error from this?? | |
#"import tesseract_core; tesseract_core.Tesseract.from_url(\"http://t-9000:8000\").health()" |
project_id = project_meta["project_id"] | ||
project_containers = [project_meta["containers"][i]["name"] for i in range(2)] | ||
|
||
T1 = docker_client.containers.get(project_containers[0]) |
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.
Use lower case for non-class variable names
T1 = docker_client.containers.get(project_containers[0]) | |
tess_1 = docker_client.containers.get(project_containers[0]) |
except Exception as e: | ||
assert False, e | ||
finally: | ||
return |
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.
This prevents cleanup from running!
except Exception as e: | ||
assert False, e |
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.
No need to do anything, just let the exception bubble up.
except Exception as e: | |
assert False, e |
if project_id: | ||
run_res = cli_runner.invoke( | ||
app, | ||
[ | ||
"teardown", | ||
project_id, | ||
], | ||
catch_exceptions=False, | ||
) | ||
assert run_res.exit_code == 0, run_res.stderr |
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.
I suggest you use the docker_cleanup
fixture instead
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.
Some minor comments, but overall this looks like a solid feature. Thanks @johnbcoughlin!
Relevant issue or PR
Description of changes
Tesseracts are able to communicate with each other over the multi-tesseract network, but the host names by which they can be addressed are assigned by the SDK, are not human-readable, and are hard to discover. This change allows the user to pass a list of meaningful tesseract "service names" to
tesseract serve
orengine.serve
. These service names function as meaningful identifiers by which tesseracts can refer to each other.As an example, suppose we have started two tesseracts using the following
tesseract serve
command:From within Tesseract A, we can run
Testing done