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.

Sign up for GitHub

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

Jump to bottom

Output of entity changes in KIC's logs instead of Println in database-reconciler repo #5289

Open
1 of 2 tasks
randmonkey opened this issue Dec 5, 2023 · 2 comments · May be fixed by #6131
Open
1 of 2 tasks

Output of entity changes in KIC's logs instead of Println in database-reconciler repo #5289

randmonkey opened this issue Dec 5, 2023 · 2 comments · May be fixed by #6131
Assignees
@rainest
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low
Milestone
KIC v3.4.x

Comments

@randmonkey
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

Currently we enable output of go-database-reconciler in setup when log level is set to trace or debug:

kubernetes-ingress-controller/internal/manager/setup.go

Lines 48 to 50 in 64a0a61

if c.LogLevel != "trace" && c.LogLevel != "debug" {
// disable deck's per-change diff output
cprint.DisableOutput = true

This uses cprint in https://github.com/Kong/go-database-reconciler to output the entity changes, which calls https://github.com/fatih/color for printing message on the terminal in ANSI colors. This output format is suitable for terminal but not for log files. It brings inconsistency in logs, and may print special characters for colors.

Proposed Solution

  • Fetch changes in return values of diff.Solve
  • Output in KIC's loggers if we enable them by setting log levels

Additional information

We should also consider updating the code in https://github.com/Kong/go-database-reconciler after separated it from decK. This repo is not mainly used as a CLI as decK, so the direct output to terminal may need to be cleaned.

Acceptance Criteria

  • When "debug" or "trace" level enabled, we can see output of entity changes (in DB mode) from KIC's logger
@mflendrich mflendrich mentioned this issue Dec 6, 2023
Closed
2 tasks
@randmonkey randmonkey added this to the KIC v3.2.x milestone Dec 13, 2023
@randmonkey
Copy link
Contributor Author

@rainest Could this be fixed after #5785 merged?

@randmonkey randmonkey added area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low labels Apr 24, 2024
@rainest
Copy link
Contributor

rainest commented Jun 3, 2024 •
edited
Loading

Not quite.

kubernetes-ingress-controller/internal/dataplane/sendconfig/dbmode.go

Line 94 in 5cefa9a

_, errs, _ := syncer.Solve(ctx, s.concurrency, false, false)
is currently just discarding the list of change events from https://github.com/Kong/go-database-reconciler/blob/db51b8d3033b178fd0729c19c06429fb75ca40b3/pkg/diff/diff.go#L568 and only handling the errors.

We need to add a new log loop that does handle that with some sort of diff output, or add a debug endpoint for it. My original intent was to do the latter, since even with our own logger and without the colors, the diffs don't lend themselves well to logging because they require multiple lines to be legible, and break the usual log contract as such.

Having a endpoint that just prints the latest diff elsewhere displays that more cleanly. We could opt to still log basic "entity changed successfully" info alongside that.

Edit: nevermind, I forgot that return was for the legacy format. We do log changes through the new channel system in

kubernetes-ingress-controller/internal/dataplane/sendconfig/dbmode.go

Lines 125 to 134 in 5cefa9a

if event.Error == nil {
s.logger.V(util.DebugLevel).Info("updated gateway entity", "action", event.Action, "kind", event.Entity.Kind, "name", event.Entity.Name)
} else {
s.logger.Error(event.Error, "failed updating gateway entity", "action", event.Action, "kind", event.Entity.Kind, "name", event.Entity.Name)
parsed, err := resourceErrorFromEntityAction(event)
if err != nil {
s.logger.Error(err, "could not parse entity update error")
} else {
s.resourceErrors = append(s.resourceErrors, parsed)
}

Full diff endpoint pending #6101

@rainest rainest linked a pull request Jun 4, 2024 that will close this issue
Open
1 task
@lahabana lahabana modified the milestones: KIC v3.2.x, KIC v3.3.x Jun 5, 2024
@lahabana lahabana modified the milestones: KIC v3.3.x, KIC v3.4.x Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rainest rainest

Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low
Projects
None yet
Milestone
KIC v3.4.x
Development

Successfully merging a pull request may close this issue.

feat(diagnostics) add diff endpoints Kong/kubernetes-ingress-controller
3 participants
@rainest @lahabana @randmonkey