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

Add status check for Harbor's read-only status #6917

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

bksteiny
Copy link
Contributor

What does this PR do?

This adds a status check to monitor whether or not Harbor is in a system-wide read-only mode.

Motivation

I added this check because we've had a few occurrences where Harbor has gone into a read-only mode and we haven't been able to figure out why. This is especially a problem if you're replicating to 1 or many instances, and replication stops because a registry has gone into read-only mode.

Additional Notes

The Harbor API returns a boolean for the read_only status. I originally wanted to submit this data as a gauge (True = 1, False =0), however I wasn't able to get the tests to pass on Harbor version 1.4 (the read_only status isn't available in that version of Harbor).

Here are the reasons why I wanted to collect this as a gauge:

  • Some folks may not care if their registry is in read-only mode, so it may not seem "critical" to them
  • We have more flexibility from an alerting standpoint
  • IMO, it's easier to see patterns / historical occurrences

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@bksteiny bksteiny requested review from a team as code owners June 17, 2020 13:27
Comment on lines 47 to 61
{
"agent_version": "6.13.0",
"integration": "Harbor",
"groups": [
"host",
"registry"
],
"check": "harbor.read_only",
"statuses": [
"ok",
"unknown",
"critical"
],
"name": "Is Harbor in read_only mode",
"description": "Returns `OK` if the registry is not in `read_only` mode, otherwise returns `CRITICAL`. Monitors if the registry is in read_only mode or not."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know if I should modify this. I can remove it if necessary

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #6917 into master will increase coverage by 5.49%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
harbor/datadog_checks/harbor/api.py 94.64% <100.00%> (ø)
harbor/datadog_checks/harbor/harbor.py 85.55% <100.00%> (ø)
harbor/tests/common.py 100.00% <100.00%> (ø)
harbor/tests/test_unit.py 100.00% <100.00%> (ø)
hazelcast/tests/common.py
hdfs_datanode/tests/common.py
mapr/tests/test_unit.py
ignite/tests/common.py
hdfs_datanode/tests/test_integration.py
presto/tests/conftest.py
... and 838 more

sarina-dd
sarina-dd previously approved these changes Jun 17, 2020
Copy link
Contributor

@sarina-dd sarina-dd left a comment

Choose a reason for hiding this comment

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

👍 docs

@FlorianVeaux
Copy link
Member

Hi @bksteiny and thanks for your PR 👍

The use case really makes sense to me but for exactly the arguments you mentioned, I think this information should really be a gauge metric instead of a service check.

Can you update the PR to re-submit it as a gauge and we can then look into the details for the tests to pass?

@bksteiny bksteiny force-pushed the harbor-read-only-status branch from ee084b2 to d303deb Compare June 22, 2020 12:57
@bksteiny
Copy link
Contributor Author

Hi @FlorianVeaux, I've pushed my changes that collect the read-only status as a gauge metric. Thanks for the assist

Copy link
Member

@FlorianVeaux FlorianVeaux left a comment

Choose a reason for hiding this comment

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

Added a few comments 👍
Can you also add the metric to the metadata.csv file?

harbor/tests/test_unit.py Outdated Show resolved Hide resolved
harbor/tests/common.py Outdated Show resolved Hide resolved
@FlorianVeaux
Copy link
Member

FlorianVeaux commented Jun 23, 2020

Thanks @bksteiny
Merging it, this will be released with the next agent version 👍 (6.21 and 7.21)

@FlorianVeaux FlorianVeaux merged commit 8c1c822 into DataDog:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants