Skip to content

PT-2421 - Adding further data collection to pt-k8s-debug-collector#1141

Open
eslavyansky wants to merge 4 commits into
percona:3.xfrom
eslavyansky:PT-2421-pgbackrest-info-collection
Open

PT-2421 - Adding further data collection to pt-k8s-debug-collector#1141
eslavyansky wants to merge 4 commits into
percona:3.xfrom
eslavyansky:PT-2421-pgbackrest-info-collection

Conversation

@eslavyansky

Copy link
Copy Markdown
Contributor

This PR introduces additional files in to the final dump archive for pgv2.
Tool logs:

  • patronictl list
  • pgbackrest info

Log files from folders:

  • $PGDATA/log

  • pgdata/pgbackrest/log

  • The contributed code is licensed under GPL v2.0

  • Contributor Licence Agreement (CLA) is signed

  • util/update-modules has been ran

  • Documentation updated

  • Test suite update

This PR introduces additional files in to the final dump archive for pgv2.
Tool logs:
- patronictl list
- pgbackrest info
Log files from folders:
- $PGDATA/log
- pgdata/pgbackrest/log

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR extends pt-k8s-debug-collector’s PG v2 (“pgv2”) collection to include additional pgBackRest logs and command outputs in the generated dump archive, and updates the integration test suite to validate those artifacts are present.

Changes:

  • Add collection of pgdata/pgbackrest/log into the dump archive for pgv2.
  • Add “tool log” collection for pgv2 by executing patronictl list and pgbackrest info in the database container and archiving the outputs as files.
  • Extend TestIndividualFiles coverage to assert the new pgv2 log files are present in cluster-dump.tar.gz.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/go/pt-k8s-debug-collector/main_test.go Adds pgv2 assertions for pgBackRest log files and tool-output log files in the archive.
src/go/pt-k8s-debug-collector/dumper/resources.go Registers new pgv2 directory dump (pgbackrest_log) and tool commands to run.
src/go/pt-k8s-debug-collector/dumper/individual_files.go Implements execution + archiving of tool outputs via processToolOutput.
src/go/pt-k8s-debug-collector/dumper/dumper.go Introduces the toolLog type and adds toolCmds to individualFile.

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

Comment thread src/go/pt-k8s-debug-collector/dumper/individual_files.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@svetasmirnova

Copy link
Copy Markdown
Collaborator

Two issues with this PR:

  1. Test for resource none fails:
=== RUN   TestCollectorRunner/Operator_pgv2/TestIndividualFiles/Resource_none
    main_test.go:507: 
        	Error Trace:	/home/sveta/src/percona/percona-toolkit/src/go/pt-k8s-debug-collector/main_test.go:507
        	            				/home/sveta/go/pkg/mod/github.com/stretchr/testify@v1.11.1/suite/suite.go:115
        	Error:      	Preprocessor Check
        	Test:       	TestCollectorRunner/Operator_pgv2/TestIndividualFiles/Resource_none
        	Messages:   	test pgv2_tools_log_list
        	            	resource:none
        	            	namespace: pgv2
        	            	output is not as expected
        	            	Output: 
        	            	Wanted: [patronictl-list.log pgbackrest-info.log]
...        --- FAIL: TestCollectorRunner/Operator_pgv2/TestIndividualFiles (105.93s)
            --- PASS: TestCollectorRunner/Operator_pgv2/TestIndividualFiles/Resource_pgv2 (42.56s)
            --- PASS: TestCollectorRunner/Operator_pgv2/TestIndividualFiles/Resource_auto (42.58s)
            --- FAIL: TestCollectorRunner/Operator_pgv2/TestIndividualFiles/Resource_none (20.79s)

Actually, tool exhibits correct behavior, because if resource none is specified, only K8 resourses should be collected, not database-specific.
2. README.rst should be updated and contain list of resources now collected for PostgreSQL.

I noticed you implemented it for pgv2 only. This maybe OK, because we need to deprecate pgo, but user manual should also mention it.

@svetasmirnova svetasmirnova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See my previous comment.

With --resource none only general Kubernetes data is collected, not
database-specific files, so TestIndividualFiles must not expect them.
Add a skipForNone flag and skip those checks when the resource is none,
while keeping the pod container log check (collected for every resource).
Drop the now-redundant tar-error guard so genuine failures are not masked.
List the individual files and tool command outputs that are collected
for PostgreSQL Operator v2 (pgv2).
@eslavyansky

Copy link
Copy Markdown
Contributor Author

@svetasmirnova check please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants