-
Notifications
You must be signed in to change notification settings - Fork 2k
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
cli: nomad agent does not show configured host_network
#11432
cli: nomad agent does not show configured host_network
#11432
Conversation
This commit should address hashicorp#11223. Specifically it enhances the CLI in order to return two flavours (default,verbose) of the `node status` command. Signed-off-by: Alessandro De Blasis <[email protected]>
Signed-off-by: Alessandro De Blasis <[email protected]>
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.
Hi @deblasis! I left one comment about the API struct but other than that this looks great.
Tested end-to-end with a host network:
$ nomad node status -self
ID = 059d16c0-b748-8134-e9b2-0cfbb537eb97
Name = linux
Class = <none>
DC = dc1
Drain = false
Eligibility = eligible
Status = ready
CSI Controllers = <none>
CSI Drivers = <none>
Uptime = 15m15s
Host Volumes = shared_data
Host Networks = public
CSI Volumes = <none>
Driver Status = docker,exec,java,mock_driver,qemu
...
I've also verified this displays properly as <none>
if there's no host network. And the verbose output looks good too:
$ nomad node status -self -verbose
ID = 059d16c0-b748-8134-e9b2-0cfbb537eb97
Name = linux
Class = <none>
DC = dc1
Drain = false
Eligibility = eligible
Status = ready
CSI Controllers = <none>
CSI Drivers = <none>
Uptime = 15m24s
Host Volumes
Name ReadOnly Source
shared_data false /srv/data
Host Networks
Name CIDR Interface
public 10.199.0.200/24 <none>
...
api/nodes.go
Outdated
type HostNetworkInfo struct { | ||
Name string | ||
CIDR string | ||
Interface string |
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.
Usually we try to have the api struct match the internal struct pretty closely; is there a reason to exclude the reserved ports in this struct?
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.
Oh, and can we add this new object to the Read Node API docs in https://github.com/hashicorp/nomad/blob/main/website/content/api-docs/nodes.mdx 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.
Usually we try to have the api struct match the internal struct pretty closely; is there a reason to exclude the reserved ports in this struct?
Hi @tgross! No, you are right, it was a genuine mistake.
I am gonna add ReservedPorts
, update the tests accordingly and also add your test cases for the <none>
values following Hashicorp principles... 😉 codify, codify, codify!
Oh, and can we add this new object to the Read Node API docs in https://github.com/hashicorp/nomad/blob/main/website/content/api-docs/nodes.mdx as well?
Sure, I'll use your example as a template
Also: -cli: fixed typo in struct comment -cli: updated tests to cover the <none> case -website: updated API docs Signed-off-by: Alessandro De Blasis <[email protected]>
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.
LGTM! Thanks @deblasis!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This commit should address #11223.
Specifically it enhances the CLI in order to return two flavours
(default,verbose) of the
node status
command that includehost_network
informationas defined in the
client
stanza of a node.Signed-off-by: Alessandro De Blasis [email protected]