Skip to content

Conversation

@ryotarai
Copy link
Collaborator

@ryotarai ryotarai commented Jun 23, 2025

Previously, there was an issue where the names of scoped RQs created from scoped HRQs could conflict. This PR fixes that problem.

Problem

In the previous implementation, the name of a scoped RQ was just a prefixed version of the scoped HRQ name. As a result, if a parent namespace and a child namespace had scoped HRQs with the same name, it would try to create RQs with the same name in the child namespace and fail.

Solution

This PR changes the RQ naming to include the namespace name and an MD5 hash of hrqNamespace/hrqName. However, since this can make the name exceed the length limit, the name is truncated appropriately when it's too long.

@ryotarai ryotarai marked this pull request as ready for review July 24, 2025 01:31
)

const (
maxRQNameLength = 253
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 using validation.DNS1123SubdomainMaxLength in k8s.io/apimachinery/pkg/util/validation instead of the magic number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e3589c1

metadata.SetLabel(rq, hrqNameLabel, hrqName)
}

func truncate(s string, n int) string {
Copy link
Member

Choose a reason for hiding this comment

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

n should be positive numbers to avoid panic. Using uint or adding validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 78a2633

@ryotarai ryotarai requested a review from utam0k August 19, 2025 01:10
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks!

@utam0k utam0k merged commit 2c8194a into pfnet:master Aug 21, 2025
2 checks passed
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.

2 participants