feat: add logs subcommand for mgrctl (Fixes #398) by YatesMold · Pull Request #724 · uyuni-project/uyuni-tools

YatesMold · 2026-02-20T18:45:04Z

What does this PR change?

This PR introduces the logs subcommand to mgrctl, allowing users to easily monitor and follow logs for various Uyuni services directly inside the container, without manually typing mgrctl exec "tail -f ...".

It implements the requested feature in Issue #398, adding support for the following flags:

--taskomatic: Shows Taskomatic logs (/var/log/rhn/rhn_tasko*.log)
--web: Shows Web UI logs (/var/log/rhn/rhn_web*.log)
--salt: Shows Salt logs (api, master, minion)
--reposync: Shows Reposync logs
--files: Regex support to filter specific reposync log files dynamically.
-f, --follow: Follow log output.

Codespace

Check if you already have a running container clicking on

Test coverage

No tests: This subcommand acts as a wrapper that constructs file paths and re-uses the extensively tested exec.RunRawCmd logic to execute the tail commands.
DONE

Links

Issue(s): Fixes #398

DONE

Changelogs

No changelog needed
(Note to reviewers: Happy to add a specific changelog entry if required for new subcommands!)

Signed-off-by: charvenetis <charvenetis@gmail.com>

Copilot

Pull request overview

Adds a new mgrctl logs subcommand to make it easier to tail/follow common Uyuni service logs from inside the server container, addressing Issue #398.

Changes:

Introduce mgrctl logs with service-selection flags (--taskomatic, --web, --salt, --reposync) plus --files filtering and -f/--follow.
Wire the new subcommand into the mgrctl root command.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
`mgrctl/cmd/logs/logs.go`	Implements `logs` command: builds log path list and executes `tail` inside the container via podman/kubectl.
`mgrctl/cmd/cmd.go`	Registers the new `logs` subcommand with the root CLI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-25T11:34:39Z

+	logsCmd.Flags().Bool("reposync", false, L("Show Reposync logs"))
+	logsCmd.Flags().String("files", "", L("Regex to filter reposync log files"))
+	logsCmd.Flags().BoolP("follow", "f", false, L("Follow log output"))


--files is silently ignored unless --reposync is also set, which is confusing given the flag help text. It would be better to validate this combination (e.g., return an error if --files is provided without --reposync).

Copilot · 2026-02-25T11:34:39Z

+	// Re-use the execution logic from the exec package
+	return exec.RunRawCmd(command, commandArgs)


Unlike mgrctl exec, this command doesn’t preserve the exit status of the underlying podman/kubectl exec (errors bubble up to main, which exits with code 1). If tail fails (e.g., missing files), it would be useful to propagate the real exit code by handling *exec.ExitError similarly to mgrctl/cmd/exec/exec.go.

Copilot · 2026-02-25T11:34:39Z

+// NewCommand returns a new cobra.Command for logs.
+func NewCommand(globalFlags *types.GlobalFlags) *cobra.Command {
+	var flags flagpole
+
+	logsCmd := &cobra.Command{
+		Use:   "logs",
+		Short: L("Show or follow logs of Uyuni services inside the container"),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return utils.CommandHelper(globalFlags, cmd, args, &flags, nil, run)
+		},
+	}
+
+	logsCmd.Flags().Bool("taskomatic", false, L("Show Taskomatic logs"))
+	logsCmd.Flags().Bool("web", false, L("Show Web UI logs"))
+	logsCmd.Flags().Bool("salt", false, L("Show Salt logs"))
+	logsCmd.Flags().Bool("reposync", false, L("Show Reposync logs"))
+	logsCmd.Flags().String("files", "", L("Regex to filter reposync log files"))
+	logsCmd.Flags().BoolP("follow", "f", false, L("Follow log output"))
+
+	utils.AddBackendFlag(logsCmd)
+	return logsCmd
+}
+
+func run(_ *types.GlobalFlags, flags *flagpole, _ *cobra.Command, _ []string) error {
+	cnx := shared.NewConnection(flags.Backend, podman.ServerContainerName, kubernetes.ServerFilter)
+	podName, err := cnx.GetPodName()
+	if err != nil {
+		return err
+	}
+
+	command, err := cnx.GetCommand()
+	if err != nil {
+		return err
+	}
+
+	var paths []string
+	if flags.Taskomatic {
+		paths = append(paths, "/var/log/rhn/rhn_tasko*.log")
+	}
+	if flags.Web {
+		paths = append(paths, "/var/log/rhn/rhn_web*.log")
+	}
+	if flags.Salt {
+		paths = append(paths, "/var/log/salt/api", "/var/log/salt/master", "/var/log/salt/minion")
+	}
+	if flags.Reposync {
+		if flags.Files != "" {
+			findCmd := fmt.Sprintf("find /var/log/rhn/reposync/ -type f | grep -E '%s'", flags.Files)
+			paths = append(paths, fmt.Sprintf("$(%s)", findCmd))
+		} else {
+			paths = append(paths, "/var/log/rhn/reposync/*.log")
+		}
+	}
+
+	if len(paths) == 0 {
+		return fmt.Errorf(L("please specify at least one service to get logs for (e.g., --web, --salt)"))
+	}


There are unit tests for other mgrctl subcommands (e.g., mgrctl/cmd/term/term_test.go, mgrctl/cmd/api/api_test.go), but this new command has none. Adding tests for flag validation (no service selected, --files without --reposync) and for the assembled exec arguments would help prevent regressions, especially around the reposync filtering behavior.

Indeed, unit tests would be needed. We at least want to be sure that the flags logic is correct.

Copilot · 2026-02-25T11:34:40Z

+			findCmd := fmt.Sprintf("find /var/log/rhn/reposync/ -type f | grep -E '%s'", flags.Files)
+			paths = append(paths, fmt.Sprintf("$(%s)", findCmd))
+		} else {


--files is interpolated directly into a single-quoted grep -E inside a bash -c string. This breaks on patterns containing ' and also enables shell injection (e.g., closing the quote and appending commands) when running inside the container. Avoid building a shell pipeline from user input; pass the pattern as an argument (with proper escaping) or use find/grep options that don't require shell-eval of the pattern.

Copilot · 2026-02-25T11:34:40Z

+		if flags.Files != "" {
+			findCmd := fmt.Sprintf("find /var/log/rhn/reposync/ -type f | grep -E '%s'", flags.Files)
+			paths = append(paths, fmt.Sprintf("$(%s)", findCmd))
+		} else {
+			paths = append(paths, "/var/log/rhn/reposync/*.log")
+		}
+	}
+
+	if len(paths) == 0 {
+		return fmt.Errorf(L("please specify at least one service to get logs for (e.g., --web, --salt)"))
+	}
+
+	tailCmd := "tail"
+	if flags.Follow {
+		tailCmd += " -f"
+	}
+
+	shCmd := fmt.Sprintf("%s %s", tailCmd, strings.Join(paths, " "))
+


When --reposync --files yields no matches, the command substitution expands to an empty list and tail will read from stdin (and with -f it will appear to hang). Consider resolving the file list first and returning a clear error when it’s empty, instead of invoking tail with no path arguments.

cbosdo

This PR is a good start in the uyuni-tools code. See my comments to make it better.

Note that there is a goal to get all the logs files to the podman logs output for easier and more cloud-native access. This would force rewriting the implementation of this command.

cbosdo · 2026-02-25T13:00:23Z

+// NewCommand returns a new cobra.Command for logs.
+func NewCommand(globalFlags *types.GlobalFlags) *cobra.Command {
+	var flags flagpole
+
+	logsCmd := &cobra.Command{
+		Use:   "logs",
+		Short: L("Show or follow logs of Uyuni services inside the container"),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return utils.CommandHelper(globalFlags, cmd, args, &flags, nil, run)
+		},
+	}
+
+	logsCmd.Flags().Bool("taskomatic", false, L("Show Taskomatic logs"))
+	logsCmd.Flags().Bool("web", false, L("Show Web UI logs"))
+	logsCmd.Flags().Bool("salt", false, L("Show Salt logs"))
+	logsCmd.Flags().Bool("reposync", false, L("Show Reposync logs"))
+	logsCmd.Flags().String("files", "", L("Regex to filter reposync log files"))
+	logsCmd.Flags().BoolP("follow", "f", false, L("Follow log output"))
+
+	utils.AddBackendFlag(logsCmd)
+	return logsCmd
+}
+
+func run(_ *types.GlobalFlags, flags *flagpole, _ *cobra.Command, _ []string) error {
+	cnx := shared.NewConnection(flags.Backend, podman.ServerContainerName, kubernetes.ServerFilter)
+	podName, err := cnx.GetPodName()
+	if err != nil {
+		return err
+	}
+
+	command, err := cnx.GetCommand()
+	if err != nil {
+		return err
+	}
+
+	var paths []string
+	if flags.Taskomatic {
+		paths = append(paths, "/var/log/rhn/rhn_tasko*.log")
+	}
+	if flags.Web {
+		paths = append(paths, "/var/log/rhn/rhn_web*.log")
+	}
+	if flags.Salt {
+		paths = append(paths, "/var/log/salt/api", "/var/log/salt/master", "/var/log/salt/minion")
+	}
+	if flags.Reposync {
+		if flags.Files != "" {
+			findCmd := fmt.Sprintf("find /var/log/rhn/reposync/ -type f | grep -E '%s'", flags.Files)
+			paths = append(paths, fmt.Sprintf("$(%s)", findCmd))
+		} else {
+			paths = append(paths, "/var/log/rhn/reposync/*.log")
+		}
+	}
+
+	if len(paths) == 0 {
+		return fmt.Errorf(L("please specify at least one service to get logs for (e.g., --web, --salt)"))
+	}


Indeed, unit tests would be needed. We at least want to be sure that the flags logic is correct.

cbosdo · 2026-02-25T13:02:25Z

+	logsCmd.Flags().Bool("web", false, L("Show Web UI logs"))
+	logsCmd.Flags().Bool("salt", false, L("Show Salt logs"))
+	logsCmd.Flags().Bool("reposync", false, L("Show Reposync logs"))
+	logsCmd.Flags().String("files", "", L("Regex to filter reposync log files"))


Explaining what the regexp is applied to would help the user to know what regex to write: does it need to match the whole path or only a segment? Those would be what a user could wonder.

cbosdo · 2026-02-25T13:03:29Z

+		Use:   "logs",
+		Short: L("Show or follow logs of Uyuni services inside the container"),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return utils.CommandHelper(globalFlags, cmd, args, &flags, nil, run)


directly calling run makes it hard to unit test: use a variable with a function pointer as done in other commands (mostly in mgradm and mgrpxy)

cbosdo · 2026-02-25T13:05:07Z

+		if flags.Files != "" {
+			script := "files=$(find /var/log/rhn/reposync/ -type f | grep -E %q); " +
+				"if [ -z \"$files\" ]; then echo 'No matching reposync logs found.' >&2; exit 1; fi; " +
+				"echo $files"
+			findCmd := fmt.Sprintf(script, flags.Files)
+			paths = append(paths, fmt.Sprintf("$(%s)", findCmd))


We probably need to handle the case where the file doesn't exist yet but would show up later (relevant for the -f option if reposync hasn't been run on that repo yet)

cbosdo · 2026-02-25T13:06:12Z

+	commandArgs = append(commandArgs, "bash", "-c", shCmd)
+
+	// Re-use the execution logic from the exec package
+	err = exec.RunRawCmd(command, commandArgs)


We have helpers to run commands in the shared module. Grep for NewRunner to find them.

agraul · 2026-02-25T14:20:26Z

+		paths = append(paths, "/var/log/rhn/rhn_web*.log")
+	}
+	if flags.Salt {
+		paths = append(paths, "/var/log/salt/api", "/var/log/salt/master", "/var/log/salt/minion")


We don't run a Salt Minion inside the container, /var/log/salt/minion won't exist

…ion runner

sonarqubecloud · 2026-03-04T15:09:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

YatesMold · 2026-03-10T11:32:18Z

Rebased on latest main, CI is green. Ready for review when you get a chance.

cbosdo

This issue needs more thinking before working on a PR: this command is clearly a band aid for logs that are not handled properly in the container.
This may be useful for users running the server on podman, but shouldn't be the way to push forward when running on Kubernetes. If we stick with a similar behavior, this needs to go to mgradm rather than mgrctl

YatesMold · 2026-03-30T15:45:10Z

Thanks for the clarification.
I'll leave it in your hands then. Happy to help if you need anything from my side

feat: add logs subcommand for mgrctl (Fixes uyuni-project#398)

360d0cf

Signed-off-by: charvenetis <charvenetis@gmail.com>

srbarrios requested a review from Copilot February 25, 2026 11:27

Copilot started reviewing on behalf of srbarrios February 25, 2026 11:29 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

fix: resolve linter error and address AI review feedback

14290f4

YatesMold force-pushed the feat-mgrctl-logs branch from f69e093 to 14290f4 Compare February 25, 2026 12:29

cbosdo requested changes Feb 25, 2026

View reviewed changes

agraul reviewed Feb 25, 2026

View reviewed changes

YatesMold added 3 commits February 25, 2026 19:00

refactor: reduce cognitive complexity to satisfy SonarQube

584095f

fix: address maintainer feedback, add unit tests, and refactor execut…

a976487

…ion runner

Merge branch 'main' into feat-mgrctl-logs

082c28c

YatesMold requested a review from cbosdo February 28, 2026 14:06

YatesMold force-pushed the feat-mgrctl-logs branch 2 times, most recently from 7608628 to 082c28c Compare March 4, 2026 15:09

Merge branch 'main' into feat-mgrctl-logs

176ad35

cbosdo requested changes Mar 30, 2026

View reviewed changes

YatesMold requested a review from cbosdo March 30, 2026 15:42

		// Re-use the execution logic from the exec package
		return exec.RunRawCmd(command, commandArgs)

Conversation

YatesMold commented Feb 20, 2026

What does this PR change?

Codespace

Test coverage

Links

Changelogs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo left a comment

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

cbosdo Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

agraul Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Mar 4, 2026

Quality Gate passed

Uh oh!

YatesMold commented Mar 10, 2026

Uh oh!

cbosdo left a comment

Choose a reason for hiding this comment

Uh oh!

YatesMold commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants