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 group identity plugin #166

Merged
merged 1 commit into from
May 30, 2022
Merged

add group identity plugin #166

merged 1 commit into from
May 30, 2022

Conversation

zwzhang0107
Copy link
Contributor

@zwzhang0107 zwzhang0107 commented May 23, 2022

Signed-off-by: zwzhang0107 zuoweizhang@outlook.com

Ⅰ. Describe what this PR does

#154

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@jasonliu747
Copy link
Member

/assign

@jasonliu747 jasonliu747 linked an issue May 24, 2022 that may be closed by this pull request
@zwzhang0107
Copy link
Contributor Author

/hold for ut

@zwzhang0107
Copy link
Contributor Author

/hold cancel

for hookFeature, hookPlugin := range runtimeHookPlugins {
if DefaultRuntimeHooksFG.Enabled(hookFeature) {
enbaled := DefaultRuntimeHooksFG.Enabled(hookFeature)
Copy link
Member

Choose a reason for hiding this comment

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

typo: enabled

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

}

func (b *bvtPlugin) parseRule(mergedNodeSLO *slov1alpha1.NodeSLOSpec) (bool, error) {
lsrValue := int64(0)
Copy link
Member

@saintube saintube May 26, 2022

Choose a reason for hiding this comment

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

how about lsrValue = int64(bvtDefault)
could the default derive from util.DefaultCPUQOS, or util.DefaultXXXQOS is going to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, using mergedNodeSLO directly is better since it is already merged

podBvt := r.getPodBvtValue(podQoS, podKubeQoS)
podCgroupPath := util.GetPodCgroupDirWithKube(podMeta.CgroupDir)
if err := sysutil.CgroupFileWrite(podCgroupPath, sysutil.CPUBVTWarpNs, strconv.FormatInt(podBvt, 10)); err != nil {
klog.Infof("update pod %v/%v cpu bvt failed, dir %v, error %v",
Copy link
Member

Choose a reason for hiding this comment

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

how about "pod %s", util.GetPodKey(pod)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


var singleton *bvtPlugin

func Object() *bvtPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

Will there be a scenario of concurrent calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not gonna happened, Object is only called during config init stage

Signed-off-by: zwzhang0107 <zuoweizhang@outlook.com>
@codecov-commenter
Copy link

Codecov Report

Merging #166 (84c4c89) into main (ea8496c) will increase coverage by 0.36%.
The diff coverage is 73.39%.

❗ Current head 84c4c89 differs from pull request most recent head deb746f. Consider uploading reports for the commit deb746f to get more accurate results

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   58.30%   58.67%   +0.36%     
==========================================
  Files          92       95       +3     
  Lines        8370     8558     +188     
==========================================
+ Hits         4880     5021     +141     
- Misses       3077     3115      +38     
- Partials      413      422       +9     
Flag Coverage Δ
unittests 58.67% <73.39%> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/koordlet/runtimehooks/config.go 14.28% <ø> (ø)
pkg/util/pod.go 9.52% <0.00%> (-0.48%) ⬇️
pkg/koordlet/statesinformer/states_informer.go 36.19% <25.00%> (+0.47%) ⬆️
pkg/koordlet/statesinformer/states_nodeslo.go 83.80% <33.33%> (ø)
...g/koordlet/runtimehooks/hooks/groupidentity/bvt.go 40.00% <40.00%> (ø)
...et/runtimehooks/hooks/groupidentity/interceptor.go 56.25% <56.25%> (ø)
pkg/koordlet/statesinformer/register.go 70.17% <80.00%> (-2.91%) ⬇️
pkg/koordlet/runtimehooks/runtimehooks.go 62.85% <85.71%> (+11.24%) ⬆️
.../koordlet/runtimehooks/hooks/groupidentity/rule.go 91.26% <91.26%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8496c...deb746f. Read the comment docs.

@hormes
Copy link
Member

hormes commented May 30, 2022

/approve

@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@saintube
Copy link
Member

/lgtm

@koordinator-bot koordinator-bot bot added the lgtm label May 30, 2022
@koordinator-bot koordinator-bot bot merged commit 0072d6b into koordinator-sh:main May 30, 2022
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.

[proposal] support group identity by runtime hooks
6 participants