Change nomad.Config to autoscaler.Nomad.Config when providing configuration for plugins. #945
Conversation
jorgemarey
force-pushed
the
b-unix-api
branch
2 times, most recently
from
b8a3a17
to
ea04ece
Compare
There was a problem hiding this comment.
Hi @jorgemarey! This passes tests but I'm concerned about dropping the use of a.nomadCfg here specifically because the documentation for that field says:
// nomadCfg is the merged Nomad API configuration that should be used when
// setting up all clients. It is the result of the Nomad api.DefaultConfig
// merged with the user-specified Nomad config.Nomad.
That implies we could be missing default field values if we're using a.config.Nomad directly. Where is it that you're seeing the mutation that's breaking things?
|
Hi @tgross Here nomad api mutates de the Address field when it's an unix:// So first when the autoscaler calls api.NewClient here that Address changes, so later when using that configuration to generate a new client for the target plugin, that client has an http://127.0.0.1 as the address for the client. I changed the api.Config to a nomad autoscaler config because I thought made sense that both clients we're creating from the same configuration (both from the nomad autoscaler configuration). |
Oh, I see... this looks like it's actually a bug in |
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
|
I've fixed that here: hashicorp/nomad#23785 I'm going to close this PR, and once #23785 has been merged, we can get the autoscaler's version of the Nomad Thanks so much for opening this PR though, as it definitely helped us debug what's going on! |
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
|
Great. Thanks @tgross! |
Fixes #944
The config.Address is mutated when using unix sockets so when that address is provided for the nomad target plugin, the client created there doesn't use the unix address. This fixes that by using the autoscaler nomad config instead of the nomad api config.