More support export logs #102
Conversation
- Pod is the RepoHost - Pod is currently a replica - Container is pgBackRest added a function to return labels for the following; - Replica instances - RepoHost instance
- PG Conf files from db instances (anything in the DATADIR matching *.conf - pgBackRest log files from the db instances - pgBackRest log files from the repo host
…primary and replicas) and the pgBackRest logs (from the primary, replicas, and repo host). - gatherAllPostgresLogs - gatherDbBackrestLogs - gatherRepoHostLogs Because of these additional logs, we also need to restructure where the logs are stored within the Support Export output. We create a folder for each pod within the /logs directory; and we place the container logs and the file logs there when appropriate.
The gatherPostgresqlLogs function was replaced with gatherAllPostgresLogs. We shoudl remove the old function since it is no longer used.
improved comments to functions. removed unused function
|
I tested this in my local environment (CPK 5.6.0, PG 16) and confirmed:
|
|
I'm curious, what do you get if you run this command while a backup job is running for the cluster? |
internal/cmd/export.go
Outdated
| if err != nil { | ||
| writeInfo(cmd, err.Error()) | ||
| if strings.Contains(stderr, "No such file or directory") { | ||
| writeDebug(cmd, "Cannot find any Backrest log files. This is acceptable in some configurations.\n") |
There was a problem hiding this comment.
Have you (manually) tested out a scenario where there were no pgbackrest log files?
There was a problem hiding this comment.
Yes but I found some places when a continue would help the CLI recover and still gather. I'll push those improvements in a bit.
There was a problem hiding this comment.
Sorry I missed this before, but I think this stderr check could go below in the stderr block (lines 1143-1146 currently)
For this one - the CLI is pulling the Pod logs from the Pod created by the CronJob. Depending upon the backrest config, there can be a lot of detail there. If |
minor refactoring to improve edge cases without backrest logs. renaming functions for consistency.
internal/cmd/export.go
Outdated
| if err != nil { | ||
| if strings.Contains(stderr, "No such file or directory") { | ||
| writeDebug(cmd, "Cannot find any Postgres log files. This is acceptable in some configurations.\n") | ||
| } | ||
| if apierrors.IsForbidden(err) { | ||
| writeInfo(cmd, err.Error()) | ||
| return nil | ||
| } | ||
| continue | ||
| } | ||
| if stderr != "" { | ||
| writeDebug(cmd, stderr) | ||
| continue | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| if strings.Contains(stderr, "No such file or directory") { | |
| writeDebug(cmd, "Cannot find any Postgres log files. This is acceptable in some configurations.\n") | |
| } | |
| if apierrors.IsForbidden(err) { | |
| writeInfo(cmd, err.Error()) | |
| return nil | |
| } | |
| continue | |
| } | |
| if stderr != "" { | |
| writeDebug(cmd, stderr) | |
| continue | |
| } | |
| if err != nil { | |
| if apierrors.IsForbidden(err) { | |
| writeInfo(cmd, err.Error()) | |
| return nil | |
| } | |
| continue | |
| } | |
| if stderr != "" { | |
| if strings.Contains(stderr, "No such file or directory") { | |
| writeDebug(cmd, "Cannot find any Postgres log files. This is acceptable in some configurations.\n") | |
| } else { | |
| writeDebug(cmd, stderr) | |
| } | |
| continue | |
| } |
What do you think about moving the stderr check into its own discrete section?
There was a problem hiding this comment.
I think I can simplify this further if I can assume that any error would populate err and stderr. Is that a safe assumption in this case?
There was a problem hiding this comment.
Basically reduce that section to the following.
My overall goal is just to emit a helpful message when no PG logs are found, but still continue or return nil` when appropriate.
// Get Postgres Log Files
stdout, stderr, err := Executor(exec).listPGLogFiles(numLogs)
if err != nil {
if strings.Contains(stderr, "No such file or directory") {
writeDebug(cmd, "Cannot find any Postgres log files. This is acceptable in some configurations.\n")
}
if apierrors.IsForbidden(err) {
writeInfo(cmd, err.Error())
return nil
}
writeDebug(cmd, stderr)
continue
}
There was a problem hiding this comment.
I think I can simplify this further if I can assume that any error would populate err and stderr. Is that a safe assumption in this case?
I don't know if that's a safe assumption. The way I think about it, "stderr" is when the function returns an error, while "err" is the possibility that the function doesn't even run.
Some code for an executor:
postgres-operator-client/internal/util/executor.go
Lines 56 to 64 in 9782c9b
For instance, if you run the command to find files, and there are no files, we get the stderr: "no files". But if you run the command to find files and Kubernetes has deleted your pod, we get an err telling us there's no pod with that name.
internal/cmd/export.go
Outdated
| for _, pod := range dbPods.Items { | ||
| writeDebug(cmd, fmt.Sprintf("Pod Name is %s\n", pod.Name)) | ||
|
|
||
| podExec, err := util.NewPodExecutor(config) |
There was a problem hiding this comment.
🤔
Could you declare this NewPodExecutor outside of the loop and then reuse it on every loop?
internal/cmd/export.go
Outdated
| if strings.Contains(stderr, "No such file or directory") { | ||
| writeDebug(cmd, "Cannot find any Repo Host log files. This is acceptable in some configurations.\n") | ||
| } |
Refactor error handling for finding PG and backrest logs. I refactored the error handling to use an OR condition to capture the state where no logs are available. This is because err and/or stderr may report 'no results' depending upon the way we call `ls` (i.e. with/without piping the results to `head`),
| writeDebug(cmd, "Error getting pgBackRest logs\n") | ||
|
|
||
| if err != nil { | ||
| writeDebug(cmd, fmt.Sprintf("%s\n", err.Error())) | ||
| } | ||
| if stderr != "" { | ||
| writeDebug(cmd, stderr) | ||
| } | ||
|
|
||
| if strings.Contains(stderr, "No such file or directory") { | ||
| writeDebug(cmd, "Cannot find any pgBackRest log files. This is acceptable in some configurations.\n") |
There was a problem hiding this comment.
⛏️ This function is really to make your life easier, so you know better than me, but these logs in gatherRepoHostLogs look the same as the logs in gatherDbBackrestLogs -- will that be confusing? It doesn't look that confusing to me, but wanted to check.
There was a problem hiding this comment.
they're different logs and coming from different paths - they could be abstracted into a common function (along with the PG logs), but right now they are very different.
The pgo support export feature gathers Postgres logs from replicas in addition to the primary and pgbackrest logs where appropriate (in addition to those gathered in the container logs.