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: refactor metering ,support Third-party resource access and idempotent #2258

Merged
merged 15 commits into from
Dec 29, 2022

Conversation

xiao-jay
Copy link
Contributor

@xiao-jay xiao-jay commented Dec 13, 2022

Signed-off-by: 晓杰 [email protected]

  • podResource,Resource,Metering,ExtensionResourcePrice api type (one day)
  • metering watch extensionResourcePrice(half day)
  • metering watch ns and create (half day)
  • pod-controller create resurce CR logic(half day)
  • metering watch Resource create by resource-Controller and update Metering (one day)
  • metering create AccountBalance and account will watch it and deduction balancde(half day)
  • Modify inappropriate places in the code by github Comment(one day)
  • e2e test (two day)

one module one commit

计量计费流程图

image

用户创建NS时操作和第三方资源控制器接入流程图
image

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Base: 66.72% // Head: 66.72% // No change to project coverage 👍

Coverage data is based on head (9197106) compared to base (75c1fe1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2258   +/-   ##
=======================================
  Coverage   66.72%   66.72%           
=======================================
  Files           6        6           
  Lines         535      535           
=======================================
  Hits          357      357           
  Misses        148      148           
  Partials       30       30           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xiao-jay xiao-jay force-pushed the metering-flowway branch 5 times, most recently from 35a8705 to c2151b2 Compare December 19, 2022 13:09
@xiao-jay xiao-jay marked this pull request as ready for review December 19, 2022 13:10
@xiao-jay xiao-jay force-pushed the metering-flowway branch 14 times, most recently from 93febac to 576b8b2 Compare December 23, 2022 12:03
Comment on lines 48 to 53
const (
FinalizerName = "metering.sealos.io/finalizer"
UserAnnotationOwnerKey = "user.sealos.io/creator"
MeteringPrefix = "metering-"
ResourceQuotaPrefix = "quota-"
METERINGNAMESPACEENV = "METERING_SYSTEM_NAMESPACE"
DEFAULTMETERINGNAMESPACE = "metering-system"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

place these consts in to api/common_types.go is batter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and notice spile format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if METERINGNAMESPACEENV will not change, switch cr metering to cluster scpoe is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

place these consts in to api/common_types.go is batter?

i agree

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 METERINGNAMESPACEENV will not change, switch cr metering to cluster scpoe is ok?

What is the purpose switch cr metering to cluster scpoe

Comment on lines +60 to +61
MeteringSystemNameSpace string
MeteringInterval int
Copy link
Collaborator

Choose a reason for hiding this comment

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

use globe var is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personal preference,both can realize function.

return nil
}

func (r *MeteringReconcile) DelMetering(ctx context.Context, name, namespace string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to export this func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a sample encapsulation,else need write more code in reconcile

Comment on lines 416 to +420
return ctrl.NewControllerManagedBy(mgr).
For(&meteringv1.Metering{}).
Watches(&source.Kind{Type: &corev1.Namespace{}}, &handler.EnqueueRequestForObject{}).
Watches(&source.Kind{Type: &meteringv1.ExtensionResourcePrice{}}, &handler.EnqueueRequestForObject{}).
Watches(&source.Kind{Type: &meteringv1.Resource{}}, &handler.EnqueueRequestForObject{}).
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, why not split these watchs to defirent controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does IMO mean

Copy link
Contributor Author

@xiao-jay xiao-jay Dec 27, 2022

Choose a reason for hiding this comment

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

because one CR should changed by one controller,if split controller will lead to 2 controller change same CR,lead to some confilct。

Comment on lines 212 to 220
if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, metering, func() error {
for _, v := range metering.Spec.Resources {
used := resource.MustParse(v.Used.String())
v.Used.Sub(used)
}
return nil
}); err != nil {
return fmt.Errorf("sync resource quota failed: %v", err)
return 0, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

calculate func just do calculate is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

var totalAmount float64
for resourceName, resourceValue := range metering.Spec.Resources {
if _, ok := metering.Spec.Resources[resourceName]; ok {
amount := float64(resourceValue.Used.MilliValue()) * float64(resourceValue.Price) / float64(resourceValue.Unit.MilliValue())
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe MilliValue is use less?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like cpu,if use value it can not expressed as 800mi

TotalAmount: 0,
LatestUpdateTime: time.Now().Unix(),
},
if time.Now().Unix()-metering.Status.LatestUpdateTime >= int64(time.Minute.Seconds())*int64(metering.Spec.TimeInterval) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment for this, split to a check func. IMO use second as TimeInterval is batter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to used second,usually will 1minute or 60minute ,not used often like 90s .

Comment on lines +47 to +48
const ACCOUNTNAMESPACEENV = "ACCOUNT_NAMESPACE"
const DEFAULTACCOUNTNAMESPACE = "sealos-system"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we switch account to cluster scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure,user only have one account,i agree you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but not in a hurry,what good if account change to cluster scope?


### How it works
This project aims to follow the Kubernetes [Operator pattern](https://kubernetes.io/docs/concepts/extend-kubernetes/operator/)
sealos cloud是一个多租户的,以k8s为内核的云操作系统,传统的资源隔离级别是虚拟机,而sealos cloud的资源隔离级别是namespace,每个用户都至少有一个自己的namespace用来使用,这样就给怎么计费带来挑战。怎么样计费k8s中 用户使用的cpu、memory等资源?怎么样计费流量等 metering 不可见的资源。
Copy link
Member

Choose a reason for hiding this comment

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

资源隔离级别是namespace, 这是不是不太贴切。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那该如何形容,隔离粒度怎么样

@xiao-jay
Copy link
Contributor Author

xiao-jay commented Dec 28, 2022

i will update readme until I complete the docs

@@ -1,6 +1,6 @@

# Image URL to use all building/pushing image targets
IMG ?= ghcr.io/labring/sealos-metering-controller:dev
IMG ?= ghcr.io/xiao-jay/sealos-metering-controller:dev
Copy link
Member

Choose a reason for hiding this comment

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

better not use personal account

//corev1.ResourceRequestsCPU: resource.MustParse("100"),
corev1.ResourceLimitsCPU: resource.MustParse("10000"),
//corev1.ResourceRequestsMemory: resource.MustParse("100"),
corev1.ResourceLimitsMemory: resource.MustParse("10000Gi"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too large for test purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@@ -90,6 +91,14 @@ func main() {
setupLog.Error(err, "unable to create controller", "controller", "Metering")
os.Exit(1)
}
if err = (&controllers.PodResourceReconciler{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe pod resource controller can split into self-hosting controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next iteration will change to facilitate decoupling

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