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

talosctl shutdown doesn't error if no talosconfig is passed. #7900

Closed
iMartyn opened this issue Oct 25, 2023 · 5 comments · Fixed by #7906
Closed

talosctl shutdown doesn't error if no talosconfig is passed. #7900

iMartyn opened this issue Oct 25, 2023 · 5 comments · Fixed by #7906
Assignees
Labels

Comments

@iMartyn
Copy link

iMartyn commented Oct 25, 2023

Bug Report

I ended up in a terminal where I hadn't exported TALOSCONFIG and issued a talosctl shutdown command and nothing happened. I assumed it was an async command and waited... and waited.

Description

There should be an error message if you issue a talosctl shutdown command without either an explicit --talosconfig or a TALOSCONFIG env var.

Logs

/home/martyn [martyn@i9worker] [22:16]
> unset TALOSCONFIG

/home/martyn [martyn@i9worker] [22:16]
> talosctl shutdown -n 192.168.1.206 -e 192.168.1.206 --force

/home/martyn [martyn@i9worker] [22:16]

(no output seen on shutdown command)

Environment

  • Talos version: [talosctl version --nodes <problematic nodes>] 1.5.4
  • Kubernetes version: [kubectl version --short] v1.28.2
  • Platform: metal-pine64
@iMartyn iMartyn changed the title Shutdown doesn't error if no talosconfig is passed. talosctl shutdown doesn't error if no talosconfig is passed. Oct 25, 2023
@smira
Copy link
Member

smira commented Oct 26, 2023

you probably have a default talosconfig which has some (unreachable?) nodes, which talosctl is using.

talosctl config info to help, not an issue in the CLI.

@smira smira closed this as completed Oct 26, 2023
@iMartyn
Copy link
Author

iMartyn commented Oct 27, 2023

No, I don't, and this is still an error that the CLI should handle, no matter what the cause!

/home/martyn [martyn@i9worker] [13:38]
> talosctl config info
no context is set

/home/martyn [martyn@i9worker] [13:38]
> talosctl shutdown -n 192.168.1.206 -e 192.168.1.206 --force

/home/martyn [martyn@i9worker] [13:39]
>

This is actually an issue in the CLI - no context should always return an error, not silently fail!

Please reopen.

@smira smira reopened this Oct 27, 2023
@smira
Copy link
Member

smira commented Oct 27, 2023

yep, looks like error reporting is broken there

@smira
Copy link
Member

smira commented Oct 27, 2023

@utkuozdemir I think something is wrong here

if errors.Is(err, context.Canceled) {
err = nil
}

and many commands using this code path are broken same way

@utkuozdemir
Copy link
Member

@smira Yes, seems like it. I'll submit a fix.

utkuozdemir added a commit to utkuozdemir/talos that referenced this issue Oct 27, 2023
Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command.

This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution.

But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors.

This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed.

Closes siderolabs#7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
utkuozdemir added a commit to utkuozdemir/talos that referenced this issue Oct 27, 2023
Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command.

This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution.

But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors.

This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed.

Closes siderolabs#7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
smira pushed a commit to smira/talos that referenced this issue Nov 8, 2023
Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command.

This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution.

But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors.

This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed.

Closes siderolabs#7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
(cherry picked from commit 5dff164)
smira pushed a commit to smira/talos that referenced this issue Nov 8, 2023
Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command.

This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution.

But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors.

This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed.

Closes siderolabs#7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
(cherry picked from commit 5dff164)
smira pushed a commit to smira/talos that referenced this issue Nov 8, 2023
Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command.

This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution.

But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors.

This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed.

Closes siderolabs#7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
(cherry picked from commit 5dff164)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants