-
-
Notifications
You must be signed in to change notification settings - Fork 116
Set machine id in service container ENV #135
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
Conversation
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 looks very good to me, thank you for your contribution! ❤️ Just one little comment on setting a new env var in the docker config.
- I'm not entirely positive that it needs the callback approach to get the id here (like it does with the IP), but it seemed like it was possible the machine ID would not be available yet when it was a new machine still being initialized.
Yes, you're right. We create the Docker gRPC service when we start the daemon so there could be the case when m.state.ID is empty if this is a new machine that hasn't been initialised yet.
gRPC doesn't allow registering new gRPC services on the already running server. That's why we have to start the Docker one that early. One of the way to get the updated machineID later is by passing a closure instead of a value.
We can perhaps restart the gRPC server with new services initialised using the updated data. Maybe I'll try to refactor it that way later.
- Are there other bits of info like machine ID that would be useful, and if so, would the ENV approach work for them to?
- service id?
- public IP (maybe wireguard IP -- not sure if useful?), but not sure if that is too dynamic/could change during lifetime of the instance
I don't know tbh as I don't have any use cases for this yet. Maybe a service name/id is useful as well. It's pretty easy to add later so let's do it once we have a use case.
- Is there a better way to do this than ENV? I currently solve this by writing a file with the machine id to a host file on each machine that is mounted into the service for it to read. Maybe something similar, but built-in to uncloud, would be a better approach. Perhaps using the new compose config mechanism -- an uncloud config file that gets written into /etc/uncloud.json or similar?
Injecting a metadata JSON using the new config feature sounds clever. But overall I really like the idea of using ENV vars which IMO is the most user friendly option as it doesn't require parsing anything and writing any code to get the values.
If we have a use case when having a file is more preferable then we can inject the file as well. For now, let's do the ENV vars only.
BTW, Fly uses exactly this approach as well: https://fly.io/docs/machines/runtime-environment/
internal/machine/docker/server.go
Outdated
| // Inject the machine ID if available | ||
| if s.machineID != nil { | ||
| if machineID := s.machineID(); machineID != "" { | ||
| envVars = append(envVars, "UNCLOUD_MACHINE_ID="+machineID) |
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.
Can you please not depend on the serialisation format (key=value) of the Env map for the Docker config here but instead just assign a new key-value in the map. I also think we don't want to modify the Env map in the user's spec so what we can do is to clone it (maps.Clone), assign a new UNCLOUD_MACHINE_ID value, and then use the .ToSlice() below in the config.
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.
Updated.
|
We should also create a new reference doc page for this and the DNS names. Any advice on how to structure it and which section to put? |
…serialization format
Updated. Let me know if that change looks okay.
The Fly env variables look pretty similar to other cases I'd thought of, but most either aren't relevant, already available, or implicit or easily embedded in the service spec (e.g. The public IP and service ID are still the only ones I can imagine coming up, but I agree waiting to add them until it does is reasonable. I do already need the private IP in a service, but
Definitely. Maybe a new section under "Concepts"? Maybe a "Services" or "Errata"? Start with useful details that don't have an obvious place for now and reorganize as it becomes more clear? I'll try to think about what other bits weren't obvious to me as I got started with it as well. |
|
Thanks for the update! I've fixed one edge case with a
This is a bit tricky as we rely on Docker for IP address management so we don't know the private IP before starting a container. In order to be able to inject it as an env var, we would need to do the IP management for the
The "Services" section sounds good to me 👍
That would be great! Feel free to share them in any way you like. Or you can even submit a PR with a markdown file in docs: https://github.com/psviderski/uncloud/tree/main/website. Any non-perfect info is better than missing docs as it's now 🙏 |
|
Thanks for the fix! I opened a PR with some quick notes on the internal DNS names and container ENV here: https://github.com/psviderski/uncloud/pull/136/files |
Just a draft of the idea, but I was thinking about how to expose info like the current machine ID to the services running on it. This is passing it in via the ENV.
Some thoughts:
I'm not entirely positive that it needs the callback approach to get the id here (like it does with the IP), but it seemed like it was possible the machine ID would not be available yet when it was a new machine still being initialized.
Are there other bits of info like machine ID that would be useful, and if so, would the ENV approach work for them to?
/etc/uncloud.jsonor similar?