Skip to content
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

Refactor talosctl command #10133

Open
rothgar opened this issue Jan 14, 2025 · 8 comments
Open

Refactor talosctl command #10133

rothgar opened this issue Jan 14, 2025 · 8 comments

Comments

@rothgar
Copy link
Member

rothgar commented Jan 14, 2025

Feature Request

Update talosctl to have the following subcommands structured as verbs (apply, edit, gen, get, run), nouns (etcd, k8s, node), and maintenance (completion, config, help, version).

command usage
apply, a used to apply a configuration
completion show shell completion
config manage talosonfig file
edit change in-place configuration
etcd managed etcd
gen generate example configurations
get, g get information about resources
help print help
kubernetes, k8s, k manage kubernetes clusters
node, n node management commands
run, r interactive commands
version print version

This replaces or moves a significant portion of subcommands to categorize them into one of these other groups.

Here is a high level table of how existing commands would be removed, moved, or modified

Original Command New Structure Status
apply-config, apply apply with serviceaccount subcommand 🔴 Removed apply-config
bootstrap Moved under etcd bootstrap 🟡 Moved
cluster create/destroy/show Moved under kubernetes cluster 🟡 Moved
completion bash/zsh completion bash/zsh ⚪ Unchanged
config (all subcommands) config with added validate 🟡 Modified
conformance Moved under kubernetes ca 🟡 Moved
containers, c Moved under get containers 🟡 Moved
copy, cp Moved under run 🟡 Moved
dashboard Moved under run 🟡 Moved
disks Moved under get 🟡 Moved
dmesg Moved under run 🟡 Moved
edit edit with --patch option 🟡 Modified
etcd (all subcommands) etcd with added bootstrap 🟡 Modified
events Removed 🔴 Removed
gen (all subcommands) gen ⚪ Unchanged
get get with expanded subcommands 🟡 Modified
health Moved under get 🟡 Moved
help help ⚪ Unchanged
image, images Moved under get 🟡 Moved
inject Removed 🔴 Removed
inspect Removed 🔴 Removed
kubeconfig Moved under get 🟡 Moved
list, ls Moved under run 🟡 Moved
logs Moved under get service 🟡 Moved
machineconfig Removed 🔴 Removed
memory, m, free Moved under run 🟡 Moved
meta Moved under node 🟡 Moved
mounts, mount Moved under run 🟡 Moved
netstat, ss Moved under run 🟡 Moved
patch Removed 🔴 Removed
pcap, tcpdump Moved under run 🟡 Moved
processes, p, ps Moved under run 🟡 Moved
read, cat Moved under run 🟡 Moved
reboot Moved under run 🟡 Moved
reset Moved under node 🟡 Moved
restart Moved under run 🟡 Moved
rollback Moved under node 🟡 Moved
rotate-ca Moved under kubernetes ca 🟡 Moved
service, services Moved under get 🟡 Moved
shutdown Moved under run 🟡 Moved
stats Moved under get containers 🟡 Moved
support Moved under run 🟡 Moved
time Moved under get 🟡 Moved
upgrade Moved under node 🟡 Moved
upgrade-k8s Moved under kubernetes 🟡 Moved
usage, du Moved under run 🟡 Moved
validate Moved under config 🟡 Moved
version version ⚪ Unchanged
N/A New kubernetes command 🟢 Added
N/A New run shell command 🟢 Added
N/A New run kubectl command 🟢 Added
N/A New node command group 🟢 Added

Description

We would like to update the command structure of talosctl into a more thoughtful (and hopefully consistent) structure that's easier to discover commands and understand how they should be used.

The main goal is not to add a lot more functionality but to clean up how the CLI has evolved over time.

One of the biggest changes is a run subcommand which is intended to be used for debugging nodes. This has many of the common debug commands (eg ls, ss, ps) and will also be able to be used interactively as a "shell". This would take the place of ssh but still give a familiar ssh feeling of debugging a single node without always needing to specify --node and --endpoint.

Getting this structure in place will help us clean up a lot of cruft and extend talosctl to be more flexible in the future.

Removed commands

The commands we would remove will be apply-config, events, inject, machineconfig, inspect, and patch.

apply-config would be removed in favor of apply because it is a more common command for admins to use. It’s already aliased to apply so it should be the easiest removal.

events isn’t a command we’re using inside talos. If it has use cases then we could possibly move it under get events.

inject currently only applies to service accounts and we can move that functionality to an apply flag or via traditional kubectl commands.

machineconfig is duplicated with gen and isn’t needed as a separate command. If there are other use cases please let us know.

inspect is a command that is primarily used for development and not needed for administration. If we want to keep this command (or something like it) we should move it to a dedicated development debugging tool.

patch is duplicated with apply and edit. If we want to have an automatic way to edit a resource with a patch file we can add a  --file option to edit which might be easier for user to find.

Migration process

We will want to keep the existing command structure and new structure for at least 2 releases where possible. There are some command flags that would overlap or be incompatible and we'll need to make sure those commands are safe to run.

We will hide the old structure from the help output so users can get used to the new structure but old subcommands would still be available.

Internally this proposal can be referenced as RFD-13

@rothgar rothgar added this to Product Jan 14, 2025
@rothgar rothgar moved this to Ideas in Product Jan 14, 2025
@oliverl-21
Copy link

Muscle memory need to be retrained but sounds good.

But what about an option -n all

Could be interesting for certain commands like talosctl dashboard or talosctl run dashboard. Because bash completion is not working with the required format -n node1,node2,no

@mrjson79
Copy link

I believe the change makes sense. There will be some re-training of the muscle memory but it looks more coherent.

@miran248
Copy link

miran248 commented Jan 14, 2025

But what about an option -n all

That, plus -n control-planes|c|cp and -n workers|w would be nice, as well.

-n all should be the default behavior, imo. With less detailed ui though (to minimize the clutter).

Edit, that would overload the --nodes and make it ambiguous, so, i don't think it's worth it.
Just make it show data from all nodes, if not specified. :)

@rothgar
Copy link
Member Author

rothgar commented Jan 15, 2025

I think we need to offer more options for where commands are run, but it would be impossible to built in all flexibility without making it config driven.

One of the things that makes it difficult is if you're using clusters with omni then omni becomes your endpoint and -n all would be every node in your omni instance.

I think it's important to address but wouldn't be changed with this issue. This one is strictly focused on the command structure.

@srgvg
Copy link

srgvg commented Jan 15, 2025

some suggestions:

  • be sure to make it more clear which commands support --insecure
  • make targetting nodes aka typing --nodes (and/or --endpoints if possible) easier (e.g. -n workers|controlpanel as suggested above), picking some random node if only one can be targetted

@nberlee
Copy link
Contributor

nberlee commented Jan 15, 2025

In general I hate more typing, but I do understand the need for restructure things

  • gen maybe under config
  • regarding upgrade-k8s after the move under kubernetes I think upgrade should suffice.

I associate get with resource definitions. For example the output of health is more like an active test. Which feels weird to get.

@Btijmen
Copy link

Btijmen commented Jan 15, 2025

I generally like the verb/noun approach, it's more intuitive for new users and more in line with other cli like kubectl. I think the proposal looks pretty neat, although I do agree that it would be nice to have a version of -n all which works with omni.

@rothgar
Copy link
Member Author

rothgar commented Jan 15, 2025

Thanks everyone for the feedback.

For everyone looking for -n grouping please leave your feedback on #10138

@rothgar rothgar moved this from Ideas to Designed in Product Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Designed
Development

No branches or pull requests

7 participants