-
Notifications
You must be signed in to change notification settings - Fork 516
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
Add host container persistent storage & persist admin ctr ssh host keys #450
Conversation
I've created and uploaded a new |
3808ab6
to
22bc60c
Compare
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.
Nice work!
💴
22bc60c
to
4bf81d9
Compare
Addresses subset of @tjkirch 's comments. |
@@ -32,7 +32,7 @@ pub struct Settings { | |||
pub updates: Option<UpdatesSettings>, | |||
|
|||
#[serde(skip_serializing_if = "Option::is_none")] | |||
pub host_containers: Option<HashMap<SingleLineString, ContainerImage>>, | |||
pub host_containers: Option<HashMap<Identifier, ContainerImage>>, |
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.
Does this require a migration?
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 don't believe so? Both SingleLineString
and Identifier
will accept what we're passing currently for container names, e.g. admin
and control
.
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.
They accept what we're currently passing, but Identifier
is more restrictive than SingleLineString
, and we don't know what our customers are passing to it.
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 think you're right, a sufficiently interesting value here could fail to parse.
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.
While technically right, I'm not sure it's worth writing a migration here; we don't expect this has been used yet, and we'd have to mangle the user's naming tremendously to have a backwards-capable migration.
Creates a new modeled type named Identifier for string used to identify container names which might be used to create files/directories
Creates a persistent storage location for containers under /local/host-containers/CONTAINER-NAME and mapped into the container under /.thar/host-containers/CONTAINER-NAME
Generates host ssh keys in admin container persistent storage under `/.thar/host-containers/admin/etc/ssh`
4bf81d9
to
b13e2cb
Compare
Addresses @iliana 's comments. Creates host container storage directory with mode |
Issue #, if available: Fixes #324
Description of changes:
Creates modelled type
ValidIdentifier
for things like container names.Adds persistent storage for host containers
Generates host SSH keys in admin container's persistent storage location.
Updated
ssh_config
and ssh script accordingly.Testing:
Unit tests for modelled-type
ValidIdentifier
passes.Launch Thar instance, admin container and control container comes up successfully
Host keys are being generated once by the admin container in persistent storage and reused:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.