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.

Sign up for GitHub

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

Jump to bottom

Generate work name by hash code #509

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Generate work name by hash code #509

merged 2 commits into from
Jul 29, 2021

Conversation

Garrybest
Copy link
Member

Signed-off-by: Garrybest [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Generate work name by MD5 hash of its namespace, kind and name in order to limit the name less than 63 characters. Consider that the mcs-controller has used work name and namespace as a label selector to selecte the associated endpoint, so it is not appropriate to move this label into annotations.

karmada/pkg/controllers/mcs/endpointslice_controller.go

Lines 42 to 45 in 514b5e6

return helper.DeleteEndpointSlice(c.Client, labels.Set{
util.WorkNamespaceLabel: req.Namespace,
util.WorkNameLabel: req.Name,
})

Which issue(s) this PR fixes:
Fixes #463

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2021
@pigletfly
Copy link
Contributor

is this backwards compatible ?

@Garrybest
Copy link
Member Author

is this backwards compatible ?

I don't think so.

@pigletfly
Copy link
Contributor

xref https://github.com/kubernetes/kubernetes/blob/f8ef5a9411595173f2ac3a510713dcabef9be297/pkg/controller/controller_utils.go#L1165-L1177

The ComputeHash func is used by kubernetes default controller-manager.

@RainbowMango
Copy link
Member

Sorry for let this sit so long. I'll get back to it soon.

@RainbowMango RainbowMango added this to the v0.8 milestone Jul 23, 2021
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 23, 2021
@Garrybest Garrybest changed the title generate work name by md5sum Generate work name by hash code Jul 23, 2021
RainbowMango
RainbowMango reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@RainbowMango RainbowMango left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for doing this.
I'm thinking if we can use a pattern like: "name-[hash-value]".
If the whole work name is a hash value, it may be hard to check the work for debugging.

[edit]

I'm thinking if we can use a pattern like: "name-".

This will still have a limitation about the name length. (not really fixes #463, but we can iterate later)

vendor/k8s.io/kubernetes/pkg/util/hash/hash.go Outdated
import (
"hash"

"github.com/davecgh/go-spew/spew"
Copy link
Member

Choose a reason for hiding this comment

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

My concern is the dependency because it seems has been abandoned.

I'm wondering if we can implement a DeepHashObject by ourselves with its alternatives, such as https://github.com/kr/pretty.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense. Let me take a look.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, the k/k is going to deprecate the go-spew, but we can't wait for that. Let's try to implement it by ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

kubernetes/kubernetes#103942

pkg/util/names/names.go Outdated
Comment on lines 59 to 67
var workName string
if len(namespace) == 0 {
return strings.ToLower(name + "-" + kind)
workName = strings.ToLower(name + "-" + kind)
} else {
workName = strings.ToLower(namespace + "-" + name + "-" + kind)
}
return strings.ToLower(namespace + "-" + name + "-" + kind)
hash := fnv.New32a()
hashutil.DeepHashObject(hash, workName)
return rand.SafeEncodeString(fmt.Sprint(hash.Sum32()))
Copy link
Member

Choose a reason for hiding this comment

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

What's the work name look like with this patch? Can you give an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, the work name default-b0d72d2a-modelpool's generated name is 774fd95bcf. It is a uint32 hash code and safely encoded. The origin uint32 hash code is 3309851679.

pkg/util/names/names.go Show resolved Hide resolved
@Garrybest
Copy link
Member Author

The name could be like <name>-<hash>, e.g., mydeployment-64d9d64476. So we have some useful information to figure out which workload it belongs to.

@RainbowMango
Copy link
Member

The name could be like <name>-<hash>, e.g., mydeployment-64d9d64476. So we have some useful information to figure out which workload it belongs to.

Yes, exactly.

@karmada-bot karmada-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2021
@Garrybest
Copy link
Member Author

Now the work name is just like mydeployment-598dcbd97b while the origin work name is default-mydeployment-deployment.

@Garrybest Garrybest requested a review from RainbowMango July 29, 2021 06:13
@RainbowMango
Copy link
Member

LGTM. Are you going to tidy to commits?

Garrybest added 2 commits July 29, 2021 14:55
@Garrybest
add vendor of pretty
58717e4 
Signed-off-by: Garrybest <[email protected]>
@Garrybest
generate work name by hash
4924021 
Signed-off-by: Garrybest <[email protected]>
@Garrybest
Copy link
Member Author

Done, thanks for reminding. @RainbowMango

@RainbowMango RainbowMango added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2021
@RainbowMango
Copy link
Member

/lgtm
/approve
Thanks!!!

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot merged commit 056b1cb into karmada-io:master Jul 29, 2021
@RainbowMango RainbowMango mentioned this pull request Jul 30, 2021
Closed
@Garrybest Garrybest deleted the work-fix branch July 30, 2021 08:53
@lonelyCZ lonelyCZ mentioned this pull request Dec 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RainbowMango RainbowMango Awaiting requested review from RainbowMango

Assignees

@RainbowMango RainbowMango

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.8
Development

Successfully merging this pull request may close these issues.

Cannot create workload that has a long name
4 participants
@Garrybest @pigletfly @RainbowMango @karmada-bot