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(metrics): Add recurring job label to backup metrics or empty value #3034

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

YvanGuidoin
Copy link
Contributor

Which issue(s) this PR fixes:

Issue N/A

What this PR does / why we need it:

Monitoring on usage of Backup per recurring job so we can trace the type of backup applied by PVC

Special notes for your reviewer:

Tried to understand documentation here and on longhorn-tests, but I am thoroughly confused on how to test a simple code change without creating a new version.

make/make test and CI seems to work though.

Additional documentation or context

Our recurring jobs, to trace usage/granularity per PVC, so we can raise alarms on certain pods not being backed up enough etc...

---
apiVersion: longhorn.io/v1beta1
kind: RecurringJob
metadata:
  name: backup-weekly
  namespace: longhorn-system
spec:
  cron: "@weekly"
  task: "backup"
  groups:
    - backup-weekly
    - backup-daily
    - backup-hourly
  retain: 12
---
apiVersion: longhorn.io/v1beta1
kind: RecurringJob
metadata:
  name: backup-daily
  namespace: longhorn-system
spec:
  cron: "@daily"
  task: "backup"
  groups:
    - backup-daily
    - backup-hourly
  retain: 7
---
apiVersion: longhorn.io/v1beta1
kind: RecurringJob
metadata:
  name: backup-hourly
  namespace: longhorn-system
spec:
  cron: "@hourly"
  task: "backup"
  groups:
    - backup-hourly
  retain: 24

@YvanGuidoin YvanGuidoin force-pushed the backup_metrics_label_job branch 5 times, most recently from 97b9a4a to 43e9650 Compare July 29, 2024 14:29
@YvanGuidoin YvanGuidoin changed the title Add recurring job label to backup metrics or empty value feat(metrics): Add recurring job label to backup metrics or empty value Jul 29, 2024
@derekbit
Copy link
Member

cc @ChanYiLin @c3y1huang for helping review it.

Copy link
Contributor

@c3y1huang c3y1huang left a comment

Choose a reason for hiding this comment

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

LGTM

@c3y1huang
Copy link
Contributor

c3y1huang commented Aug 27, 2024

We should also update the document.
https://github.com/longhorn/website/blob/master/content/docs/1.8.0/monitoring/metrics.md?plain=1

@YvanGuidoin could you help? Thank you.

@YvanGuidoin
Copy link
Contributor Author

@c3y1huang I can help end of next week as I am currently travelling

Signed-off-by: Yvan <y.guidoin@meteocontrol.com>
@c3y1huang
Copy link
Contributor

c3y1huang commented Sep 10, 2024

@YvanGuidoin I've created a feature request for this. In the future, I encourage you to create it yourself next time to have it included in the development cycle. Thank you.

longhorn/longhorn#9429

cc @derekbit

@c3y1huang
Copy link
Contributor

cc @ChanYiLin

@derekbit derekbit merged commit f7f0728 into longhorn:master Sep 10, 2024
8 checks passed
@ChanYiLin
Copy link
Contributor

ChanYiLin commented Sep 10, 2024

Hi @YvanGuidoin
For the testing of this feature, we can rely on @longhorn/qa to verify after the PR is merged
longhorn/longhorn#9429 (comment)
You can elaborate the testing steps and the expected results in this comment so the QA team members can test it on their own environment.

Some enhancement features or function changing features may need to design an e2e test in longhorn-test repo, but there is no need for this one.
Thanks for your contribution.

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