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

Refactor billing #246

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Refactor billing #246

merged 2 commits into from
Oct 25, 2024

Conversation

zugao
Copy link
Collaborator

@zugao zugao commented Oct 21, 2024

Summary

Currently we are getting data for billing from the workloads (deployments and statefulsets) special label appcat.io/billing=true. This approach proved to be ineffective for some services such as VSHNPostgreSQL which is managed by Stackgres. Unfortunately the operator sometimes is managing the pods directly, by passing the workload. In this particular case we end up without the necessary label in the pod definition.

This new approach manages differently the billing architecture. If previously we were having one prometheus rule which would gather unnecessarily difficult specific data, now we have one rule per each instance namespace and it is managed directly by the composition function where we get reliable data from the source.

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

@zugao zugao added the enhancement New feature or request label Oct 21, 2024
@zugao zugao requested review from a team, Kidswiss, TheBigLee and wejdross and removed request for a team October 21, 2024 06:16
@zugao zugao added the bug Something isn't working label Oct 21, 2024
@zugao zugao added the minor label Oct 21, 2024
@zugao zugao force-pushed the change-billing branch 2 times, most recently from 776e6f2 to 66e2322 Compare October 21, 2024 07:28
@zugao zugao requested review from a team, Kidswiss, TheBigLee and wejdross and removed request for a team October 21, 2024 07:31
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

Have you tested this with an appuio managed scenario as well?

pkg/comp-functions/functions/common/billing.go Outdated Show resolved Hide resolved
pkg/comp-functions/functions/common/billing.go Outdated Show resolved Hide resolved
@zugao
Copy link
Collaborator Author

zugao commented Oct 21, 2024

Have you tested this with an appuio managed scenario as well?

I am deploying now in a test managed cluster and will confirm that everything is fine.

@Kidswiss
Copy link
Contributor

Have you tested this with an appuio managed scenario as well?

I am deploying now in a test managed cluster and will confirm that everything is fine.

The query to generate the data is slightly different, we add another label: https://github.com/vshn/component-appcat/blob/master/component/promql/metering_managed.promql#L14

The value is directly set via the component. You need to add some logic to cover that case here as well.

@zugao
Copy link
Collaborator Author

zugao commented Oct 21, 2024

The value is directly set via the component. You need to add some logic to cover that case here as well.

Indeed I take the sales order from the composition extra config map. This should do the job

@zugao zugao requested a review from Kidswiss October 21, 2024 13:05
@zugao
Copy link
Collaborator Author

zugao commented Oct 21, 2024

Tested in kind with APPUiO Managed (+ sales_order label) and without (- sales_order label).

Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

I just found something concerning the AddServiceBillingLabel step, which can break nextcloud or keycloak provision:

it restarts the instance 1 minute after the initial deployment, so we should also remove that function as soon as possible.

@zugao zugao force-pushed the change-billing branch 3 times, most recently from c45ec4a to 43cccd2 Compare October 24, 2024 13:59
@zugao
Copy link
Collaborator Author

zugao commented Oct 24, 2024

I just found something concerning the AddServiceBillingLabel step, which can break nextcloud or keycloak provision:

it restarts the instance 1 minute after the initial deployment, so we should also remove that function as soon as possible.

Tested if it would restart the pod, it doesn't so I removed it!

@zugao zugao requested a review from Kidswiss October 24, 2024 14:12
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

Just one more thing to simplify, the rest LGTM

pkg/comp-functions/functions/common/billing.go Outdated Show resolved Hide resolved
@zugao zugao merged commit e9f2087 into master Oct 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants