Revise binding/work naming #2969
Conversation
karmada-bot
requested review from
carlory,
mrlihanbo,
RainbowMango and
XiShanYongYe-Chang
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
karmada-bot
added
the
size/XL
karmada-bot
added
the
do-not-merge/work-in-progress
karmada-bot
removed
the
do-not-merge/work-in-progress
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #2969 +/- ##
==========================================
- Coverage 49.27% 48.89% -0.38%
==========================================
Files 203 203
Lines 18222 18366 +144
==========================================
+ Hits 8978 8980 +2
- Misses 8753 8895 +142
Partials 491 491
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dddddai
force-pushed
the
rb-name
branch
2 times, most recently
from
371cd13
to
a4b5afc
Compare
Signed-off-by: dddddai <dddwq@foxmail.com>
|
Thanks @dddddai, I'll get back on this later, maybe after release 1.5. |
|
OK, no rush :) |
|
/assign |
| // GenerateBindingName will generate binding name by kind and name | ||
| func GenerateBindingName(kind, name string) string { | ||
| // GenerateBindingName will generate binding name for the template. | ||
| func GenerateBindingName(ctx context.Context, karmadaClient client.Client, dynamicClient dynamic.Interface, |
There was a problem hiding this comment.
Here(and the GenerateWorkName below) tells the main idea that identifying the conflicts and making a random name, right?
There was a problem hiding this comment.
Yes, we will append a random suffix if there's a conlict, and record the name in resource template annotations
There was a problem hiding this comment.
One of the advantages is that the mechanism can be shared with three controllers:
- build ResourceBinding by detector
- build ResourceBinding by dependenciesdistributor
- build ResourceBinding by hpa controller
But the disadvantage seems to be that GenerateBindingName is coupled with too much logic that beyond a function should do. And after we generated the ResourceBinding, we usually try to create or update it by controllerutil.CreateOrUpdate which will get the ResourceBinding one more time in addition to GenerateBindingName, that seems redundant.
In general, I like the idea that we can introduce an annotation to the resource template to record the conflict condition(just like Deployment CollisionCount) that could be a factor to build ResourceBinding name.
So, I'm thinking if there is a more elegant way to do this. I don't have a clue yet.
I'd like to invite @Garrybest @likakuli for comment.
There was a problem hiding this comment.
100% agree. This function is much more complex than it should be. Introduce an anotation or an API field could be better.
There was a problem hiding this comment.
agree. I think a new field like CollisionCount could be better.
There was a problem hiding this comment.
@likakuli We can't change the API of any resource template, that's the reason why we are considering annotation.
|
I'm trying to figure out which PR/Issue should be included in the coming v1.7 release which is planned at the end of this month. |
What type of PR is this?
/kind bug
/kind design
What this PR does / why we need it:
The current binding/work naming has some defects as mentioned in #2090 and #2099, this pr revises the naming rule to solve them
If name collision happens, we will append a random suffix to the name and retry again
Which issue(s) this PR fixes:
Fixes #2090 and #2099
Special notes for your reviewer:
Does this PR introduce a user-facing change?: