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

Append/Update flow Rule #213

Closed
wants to merge 15 commits into from

Conversation

binbin0325
Copy link
Collaborator

@binbin0325 binbin0325 commented Aug 25, 2020

Describe what this PR does / why we need it

Append and update the rule

Does this pull request fix one issue?

#174
Support appending rule for flow control/circuit breaker/system adaptive #174

Describe how you did it

append Rule 设计思路:
append flow rule 和 loadRule 基本相同,loadRule 加载Rule集合,append 只考虑追加一个Rule的情况即可。
loadRule func 每次生成新的临时TrafficControllerMap(存储resource对应的TrafficShapingController),用来替换原来全局的 tcMap
appendRule 不需要生成新的临时TrafficControllerMap,直接操作tcMap追加rule即可
实现步骤:
1:验证rule合规性
2:根据append rule resouce 查看全局tcMap中是否存在对应TrafficShapingController
3:根据当前追加Rule构建TrafficShapingController

update Rule 设计思路:
更新Rule ,需要传递要更新的Rule id 以及Rule. 更新的rule 对应的resouce 在tcMap中必须存在。
根据resource获取tcMap中的TrafficShapingController 集合
遍历集合 找到要更新的ruleId 对应的TrafficShapingController,进行更新.
实现步骤:
1:验证rule合规性
2:根据append rule resouce 查看全局tcMap中是否存在对应TrafficShapingController
3:遍历集合 找到要更新的ruleId 对应的TrafficShapingController,进行更新.

Describe how to verify it

Special notes for reviews

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@louyuting louyuting self-requested a review August 25, 2020 10:25
}
return m
}

func isAppend(rulesOfRes []*TrafficShapingController, ruleId uint64) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use []*Rule as first argument is better?

@sczyh30
Copy link
Member

sczyh30 commented Sep 1, 2020

@sanxun0325 Could you please illustrate your design in PR description?

@louyuting louyuting added this to the 0.7.0 milestone Sep 2, 2020
@louyuting louyuting assigned binbin0325 and louyuting and unassigned binbin0325 Sep 3, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2020

Codecov Report

Merging #213 into master will increase coverage by 0.28%.
The diff coverage is 58.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   42.93%   43.22%   +0.28%     
==========================================
  Files          74       74              
  Lines        4316     4396      +80     
==========================================
+ Hits         1853     1900      +47     
- Misses       2230     2250      +20     
- Partials      233      246      +13     
Impacted Files Coverage Δ
core/flow/rule_manager.go 60.00% <58.75%> (-0.59%) ⬇️

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 12ffa49...b4bfd76. Read the comment docs.

@binbin0325 binbin0325 marked this pull request as ready for review September 7, 2020 14:19
@binbin0325
Copy link
Collaborator Author

binbin0325 commented Sep 7, 2020

@sanxun0325 Could you please illustrate your design in PR description?

@sczyh30 It has been updated, please check

@binbin0325 binbin0325 changed the title [#174]Append update rule [#174]append update flow rule Sep 7, 2020
@louyuting louyuting added area/flow-control Issues or PRs related to flow control kind/enhancement Category issues or PRs related to enhancement labels Sep 7, 2020
@louyuting louyuting linked an issue Sep 7, 2020 that may be closed by this pull request
5 tasks
@louyuting louyuting changed the title [#174]append update flow rule Appen/Update flow Rule Sep 7, 2020
@sczyh30 sczyh30 self-requested a review September 8, 2020 12:02
@louyuting
Copy link
Collaborator

Could you please resolve conflict?

return nil
}

//Append Rule
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exported function need more detailed comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll add more description information

return errors.New("Ignoring the rule due to bad generated traffic controller")
}
resTcs[idx] = tsc
tcMap[rule.Resource] = resTcs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't have to do this step . tcMap[rule.Resource] = resTcs

…olang into append_update_rule

# Conflicts:
#	core/flow/rule_manager.go
@louyuting louyuting removed this from the 0.7.0 milestone Sep 20, 2020
@louyuting louyuting added the area/rule-manager Issues or PRs related to rules manager label Dec 7, 2020
@louyuting louyuting added this to the 1.1.0 milestone Dec 7, 2020
@luckyxiaoqiang luckyxiaoqiang changed the title Appen/Update flow Rule Append/Update flow Rule Dec 12, 2020
@binbin0325
Copy link
Collaborator Author

Using resource level updates and append rules is sufficient

@binbin0325 binbin0325 closed this Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flow-control Issues or PRs related to flow control area/rule-manager Issues or PRs related to rules manager kind/enhancement Category issues or PRs related to enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Manager: Replace rules in resource level.
4 participants