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

feat(logs): Refactor and expose log tailing logic #984

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

LucaSeri
Copy link
Contributor

@LucaSeri LucaSeri commented Nov 7, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

This is currently based on #927.
This PR merges the two implementations of logs tailing from the logs and run modules and exposes the functionality through the logs module.
A fix was introduced such that kraft run doesn't attempt to shutdown the entirety of kraft as this results in very unpredictable and confusing behavior when using kraftkit as an API. Instead, just report the errors and let the caller handle them.

@LucaSeri LucaSeri force-pushed the run-logs branch 3 times, most recently from dc47385 to 7e9719f Compare November 20, 2023 08:50
LucaSeri added a commit to LucaSeri/kraftkit that referenced this pull request Nov 22, 2023
Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
LucaSeri added a commit to LucaSeri/kraftkit that referenced this pull request Nov 22, 2023
Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
LucaSeri added a commit to LucaSeri/kraftkit that referenced this pull request Nov 22, 2023
Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
LucaSeri added a commit to LucaSeri/kraftkit that referenced this pull request Nov 22, 2023
Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LucaSeri, thanks a lot for this! This PR just needs a rebase, also a couple of suggestions:

diff --git a/internal/cli/kraft/logs/logs.go b/internal/cli/kraft/logs/logs.go
index 5496614..4ea1d04 100644
--- a/internal/cli/kraft/logs/logs.go
+++ b/internal/cli/kraft/logs/logs.go
@@ -11,7 +11,6 @@ import (
        "io"
        "os"
 
-       zip "api.zip"
        "github.com/spf13/cobra"
 
        machineapi "kraftkit.sh/api/machine/v1alpha1"
@@ -130,7 +129,7 @@ func (opts *LogOptions) Run(ctx context.Context, args []string) error {
 }
 
 // FollowLogs tracks the logs generated by a machine and prints them to the context out stream.
-func FollowLogs(ctx context.Context, machine *zip.Object[machineapi.MachineSpec, machineapi.MachineStatus], controller machineapi.MachineService) error {
+func FollowLogs(ctx context.Context, machine *machineapi.Machine, controller machineapi.MachineService) error {
        ctx, cancel := context.WithCancel(ctx)
 
        var exitErr error
@@ -139,8 +138,7 @@ func FollowLogs(ctx context.Context, machine *zip.Object[machineapi.MachineSpec,
                events, errs, err := controller.Watch(ctx, machine)
                if err != nil {
                        cancel()
-                       log.G(ctx).Errorf("could not listen for machine updates: %v", err)
-                       exitErr = err
+                       exitErr = fmt.Errorf("listening to machine events: %w", err)
                        return
                }
 
@@ -192,7 +190,6 @@ loop:
                        }
 
                case <-ctx.Done():
-                       log.G(ctx).Debugf("log following context cancelled")
                        break loop
                }
        }

Tailing logs is implemeneted (slightly differently)
in multiple places. Expose this function in order to
use a single implementation.

Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
This commit makes sure that an EOF is interpreted
correctly depending on whether the machine is still
running and might generate future logs or it has exited
and we should stop tailing the logs.

Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
This commit uses the logging logic from the logs module
instead of reimplementing the same logic.

Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
In case the machine fails to start, requesting a shutdown would
cause the application of anyone trying to use this API to
(silently and unexpectedly) crash. Instead, just return the error
and let the caller handle it appropiately.

Signed-off-by: Luca Seritan <luca.seritan@gmail.com>
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

Reviewed-by: Alexander Jung alex@unikraft.io
Approved-by: Alexander Jung alex@unikraft.io

@nderjung nderjung merged commit 15247a1 into unikraft:staging Jan 8, 2024
4 checks passed
@LucaSeri LucaSeri deleted the run-logs branch January 31, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

2 participants