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

feat: add velero collector (#806) #1365

Closed
wants to merge 3 commits into from
Closed

feat: add velero collector (#806) #1365

wants to merge 3 commits into from

Conversation

arcolife
Copy link
Contributor

Description, Motivation and Context

Velero collector and analyzer

Partially fixes: #806
supersedes: #1303

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

This was referenced Oct 11, 2023
Merged
Closed
@arcolife arcolife marked this pull request as ready for review October 11, 2023 05:53
@arcolife arcolife requested a review from a team as a code owner October 11, 2023 05:53
@xavpaice xavpaice added the type::feature New feature or request label Oct 11, 2023
adamancini and others added 3 commits October 13, 2023 00:33
@adamancini @arcolife
feat: add velero collector (replicatedhq#806)
cabede6 
  * test velero spec

  * need to get an older velero library to support restic

Signed-off-by: Archit Sharma <[email protected]>
@arcolife
updated schema
a237f4b 
Signed-off-by: Archit Sharma <[email protected]>
@arcolife
refactor
a7e77f6 
Signed-off-by: Archit Sharma <[email protected]>
CpuID
CpuID suggested changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@CpuID CpuID left a comment

Choose a reason for hiding this comment

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

great start... looking good. one comment added about velero binary detection

can we also get some tests around the code that's been added here?

pkg/collect/velero.go

// collect output from velero binary
if pod != nil {
for _, command := range VeleroCommands {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need some logic to check that the velero binary exists on a system? eg. if the collector is defined but velero isn't installed, will this error out on bundle creation? (not ideal)

we should gracefully skip collection if the velero binary isn't found, and/or provide something informative. bonus points if its visible in the bundle that the binary wasn't found

banjoh
banjoh requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@banjoh banjoh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems all that we collect are logs and CRs/CRDs to troubleshoot velero. As earlier discussed, lets check if the clusterResources and logs collectors can be used to achieve what this velero collector implementation does. From the tests we ran, it seem like all the information is collected but since we eyeballed the support bundle output comparison, please have a better look. Here is the spec we used

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: support-bundle
spec:
  collectors:
    - clusterResources: {} # Usually omitted since its added by default
    - logs:
        namespace: velero

In addition to the above, check whether the spec above covers collecting all that is discussed in #806 issue. If there is any data that is velero specific, then the new collector can address that.

@adamancini
Copy link
Member

great start... looking good. one comment added about velero binary detection

can we also get some tests around the code that's been added here?

need to read the pr fully but if it uses what I wrote, the velero binary should be in the velero pod.

@xavpaice
Copy link
Member

xavpaice commented Nov 8, 2023

no longer needed, the cluster-resources collector already has the data for an analyzer.

@xavpaice xavpaice closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CpuID CpuID CpuID requested changes

@banjoh banjoh banjoh requested changes

@adamancini adamancini Awaiting requested review from adamancini

@DexterYan DexterYan Awaiting requested review from DexterYan

@diamonwiggins diamonwiggins Awaiting requested review from diamonwiggins

Assignees
No one assigned
Labels
type::feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Velero Collector and Analyzer
5 participants
@arcolife @adamancini @xavpaice @banjoh @CpuID