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 support for AZ-separated quotas #614

Merged
merged 42 commits into from
Dec 9, 2024
Merged

Add support for AZ-separated quotas #614

merged 42 commits into from
Dec 9, 2024

Conversation

VoigtS
Copy link
Member

@VoigtS VoigtS commented Nov 29, 2024

This implementation handles the introduction of resource topologies. This allows the handling of synchronizing AZ-aware and AZ-separated quotas with the backend.
The plugins will check if a liquid provides a valid toplogy. Invalid AZ's that are detected during scrapes will be rejected. Scraped AZ-separated quota will be included into the backend_quota column that resides in the project_az_resources table. From limes quota will then be synced with the backend if necessary. The backend_quota values will then be updated to the calculated quota values.

Things to note:
* The AZ key sorting in the uitlity functions might be able to be improved.
* MatchLiquidReportToTopology might collect the errors for all resources instead of failing at the first mismatch.
* SetQuota required a new type to support passing through the AZ-aware qutoas to the backend. We might have to adjust the naming of the structure and its contents.

TODOS:

  • Adjust the algorithm to not provide base quota in any for az-separated resources. (Requires additional agreement on how to approach this topic.) -> The base quota value will now be provided to all AZs if the topology is AZSeparated.
  • Cleanup steps involving ResourceBehavior.CommitmentIsAZAware

@coveralls
Copy link

coveralls commented Nov 29, 2024

Coverage Status

coverage: 79.325% (+0.3%) from 78.993%
when pulling 8e72ed5 on resource_topology
into f0beb49 on master.

@VoigtS VoigtS force-pushed the resource_topology branch from 5cea98f to 8f5f123 Compare December 5, 2024 13:16
@VoigtS VoigtS marked this pull request as ready for review December 5, 2024 13:18
internal/api/commitment.go Outdated Show resolved Hide resolved
internal/api/commitment_test.go Outdated Show resolved Hide resolved
internal/collector/scrape.go Outdated Show resolved Hide resolved
internal/collector/scrape.go Outdated Show resolved Hide resolved
internal/plugins/utils.go Outdated Show resolved Hide resolved
internal/plugins/capacity_liquid.go Outdated Show resolved Hide resolved
internal/plugins/liquid.go Outdated Show resolved Hide resolved
internal/collector/scrape_test.go Outdated Show resolved Hide resolved
internal/collector/sync_quota_to_backend.go Outdated Show resolved Hide resolved
internal/datamodel/apply_computed_project_quota.go Outdated Show resolved Hide resolved
VoigtS and others added 9 commits December 5, 2024 16:46
scrape now makes an explicit choice about how to fill backendQuota.

Co-authored-by: Stefan Majewsky <[email protected]>
ACPQ: remove wrongly added allocated value for AZ separated base quota application. Unit test fixes follow.

Co-authored-by: Stefan Majewsky <[email protected]>
Because internal algorithms depend on a topology being chosen.
…ead of using service id

using the service ID causes an update to all resources in this service disregarding the true topology.
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

We're getting there. A few more nitpicks, and here's a big thing on top: I don't know how to flag this on a specific line, so I'm going to write it here. If the code works the same as in my head, we will have a neverending series of useless quota syncs on resources with AZSeparatedResourceTopology. As per the spec, these resources should have ResourceUsageReport.Quota == nil (i.e. no total quota should be reported), which means that project_resources.backend_quota will be empty. ACPQ currently always fills project_resources.quota for the resources that it operates on, so on the resource level, we always have quota != backend_quota and therefore a quota sync is forced during each ACPQ run.

My gut feeling is that ACPQ should just not fill project_resources.quota for AZSeparateResourceTopology resources. (The absence of the overall quota could then also be a trigger for the UI to hide the region-wide quota/usage bar.) But I have not finished thinking about whether that will induce corner cases elsewhere.

internal/collector/sync_quota_to_backend.go Outdated Show resolved Hide resolved
internal/datamodel/apply_computed_project_quota_test.go Outdated Show resolved Hide resolved
internal/plugins/capacity_liquid.go Outdated Show resolved Hide resolved
internal/plugins/liquid.go Outdated Show resolved Hide resolved
internal/plugins/utils.go Outdated Show resolved Hide resolved
@VoigtS
Copy link
Member Author

VoigtS commented Dec 6, 2024

We're getting there. A few more nitpicks, and here's a big thing on top: I don't know how to flag this on a specific line, so I'm going to write it here. If the code works the same as in my head, we will have a neverending series of useless quota syncs on resources with AZSeparatedResourceTopology. As per the spec, these resources should have ResourceUsageReport.Quota == nil (i.e. no total quota should be reported), which means that project_resources.backend_quota will be empty. ACPQ currently always fills project_resources.quota for the resources that it operates on, so on the resource level, we always have quota != backend_quota and therefore a quota sync is forced during each ACPQ run.

My gut feeling is that ACPQ should just not fill project_resources.quota for AZSeparateResourceTopology resources. (The absence of the overall quota could then also be a trigger for the UI to hide the region-wide quota/usage bar.) But I have not finished thinking about whether that will induce corner cases elsewhere.

I did not know that it is intended for the liquid to return a nil value if it has the AZ separated toplogy. I included a skip for setting the ACPQ quota and provided an explanation for it.

Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

I have pushed a few nitpick fixes. Also, there is one last bigger thing:

internal/datamodel/apply_computed_project_quota.go Outdated Show resolved Hide resolved
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@majewsky majewsky merged commit 072f27e into master Dec 9, 2024
5 checks passed
@majewsky majewsky deleted the resource_topology branch December 9, 2024 14:25
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.

3 participants