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

Support Quota metrics #6

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Support Quota metrics #6

merged 1 commit into from
Jul 28, 2022

Conversation

P-Cao
Copy link
Contributor

@P-Cao P-Cao commented Jul 25, 2022 •
edited by baoy1
Loading

Description

  • Implement quota metrics
    powerscale_volume_hard_quota_remaining_gigabytes
    powerscale_volume_quota_subscribed_gigabytes
    powerscale_volume_hard_quota_remaining_percentage
    powerscale_volume_quota_subscribed_percentage
    powerscale_directory_total_hard_quota_gigabytes
    powerscale_directory_total_hard_quota_percentage

GitHub Issues

GitHub Issue #
dell/csm#452

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have inspected the Grafana dashboards to verify the data is displayed properly
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility

How Has This Been Tested?

  • Run make check test

image

  • Manual inspection of Prometheus
    Volume metrics:
    image
    image
    image
    image
    Directory metrics:
    image
    image

Manual inspection of the GUI

I have verified that the dashboards show the data properly while generating I/O and storage resources

  • Yes
  • No

@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 25, 2022

run e2e test

@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 25, 2022

e2e pass

taohe1012
taohe1012 reviewed Jul 25, 2022
View reviewed changes
internal/service/service.go

var wg sync.WaitGroup
wg.Add(2)
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

if so, MaxPowerScaleConnections actually is not 10 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if so, MaxPowerScaleConnections actually is not 10 ?

It has nothing to do with the connection. It works like a CountDownLatch for 2 threads: volume level & cluster level

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the discussion, we don't interact with PowerScale array in gatherVolumeQuotaMetrics and gatherClusterQuotaMetrics, and sem := make(chan struct{}, s.MaxPowerScaleConnections) in the two classes can be used to control the concurrency of goroutine. IMO it is okay to leave it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@P-Cao @baoy1
Still considering,
if getting all quota info for all cluster firstly, it will cost most time, and in gatherVolumeQuotaMetrics and gatherClusterQuotaMetrics, the 20 goroutine cost less; So considering maybe getting quota for one cluster as one cycle is better and is more controllable, not getting all clusters quota then dealing them?

Copy link
Contributor Author

@P-Cao P-Cao Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@P-Cao @baoy1 Still considering, if getting all quota info for all cluster firstly, it will cost most time, and in gatherVolumeQuotaMetrics and gatherClusterQuotaMetrics, the 20 goroutine cost less; So considering maybe getting quota for one cluster as one cycle is better and is more controllable, not getting all clusters quota then dealing them?

Maybe improve this later as an independent ticket. This improvement requires the modification of export-push-gather structure.

taohe1012
taohe1012 reviewed Jul 25, 2022
View reviewed changes
internal/service/service.go Outdated Show resolved Hide resolved
internal/service/service.go Outdated Show resolved Hide resolved
internal/service/service.go Show resolved Hide resolved
@P-Cao P-Cao force-pushed the feature-pscale-quota-metrics branch from 5937713 to b7217c7 Compare July 25, 2022 08:47
@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 25, 2022

run e2e test

baoy1
baoy1 reviewed Jul 25, 2022
View reviewed changes
internal/service/service.go Outdated Show resolved Hide resolved
internal/service/service.go Outdated
@@ -28,12 +28,14 @@ const (
DefaultMaxPowerScaleConnections = 10
// ExpectedVolumeHandleProperties is the number of properties that the VolumeHandle contains
ExpectedVolumeHandleProperties = 4
// VolumeQuotaType is the type of Quota corresponding to a volume
VolumeQuotaType = "directory"
Copy link
Contributor

Choose a reason for hiding this comment

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

VolumeQuotaType looks generic. The field name should be specific to "directory".

@P-Cao P-Cao force-pushed the feature-pscale-quota-metrics branch from b7217c7 to 292be03 Compare July 25, 2022 09:50
@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 25, 2022

run e2e test

YianZong
YianZong reviewed Jul 26, 2022
View reviewed changes
internal/service/types.go Outdated
@@ -26,6 +26,11 @@ type VolumeMeta struct {
NameSpace string
}

// ClusterMeta is the details of a volume in an SDC
Copy link

Choose a reason for hiding this comment

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

SDC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@P-Cao P-Cao force-pushed the feature-pscale-quota-metrics branch from 292be03 to db256f0 Compare July 26, 2022 06:36
@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 26, 2022

run e2e test

@P-Cao P-Cao force-pushed the feature-pscale-quota-metrics branch from db256f0 to f3190e2 Compare July 27, 2022 09:13
@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 27, 2022

Volume Metric labels are updated

@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 27, 2022

run e2e test

@P-Cao
Support Quota metrics
ebabfbf 
- Implement quota metrics
  powerscale_volume_hard_quota_remaining_gigabytes
  powerscale_volume_quota_subscribed_gigabytes
  powerscale_volume_hard_quota_remaining_percentage
  powerscale_volume_quota_subscribed_percentage
  powerscale_directory_total_hard_quota_gigabytes
  powerscale_directory_total_hard_quota_percentage
@P-Cao P-Cao force-pushed the feature-pscale-quota-metrics branch from f3190e2 to ebabfbf Compare July 27, 2022 10:05
@P-Cao
Copy link
Contributor Author

P-Cao commented Jul 27, 2022

run e2e test

@baoy1 baoy1 merged commit 1b067ef into main Jul 28, 2022
@baoy1 baoy1 deleted the feature-pscale-quota-metrics branch July 28, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baoy1 baoy1 baoy1 approved these changes

@forrestxia forrestxia forrestxia approved these changes

@taohe1012 taohe1012 taohe1012 approved these changes

@YianZong YianZong YianZong approved these changes

@hoppea2 hoppea2 Awaiting requested review from hoppea2

@alikdell alikdell Awaiting requested review from alikdell

@shaynafinocchiaro shaynafinocchiaro Awaiting requested review from shaynafinocchiaro

@sharmilarama sharmilarama Awaiting requested review from sharmilarama

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@P-Cao @YianZong @taohe1012 @forrestxia @baoy1