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

fix group mapping and encoding order #20846

Merged
merged 1 commit into from
Feb 15, 2016
Merged

fix group mapping and encoding order #20846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions pkg/apimachinery/registered/registered.go
View file
Open in desktop
Expand Up @@ -114,22 +114,24 @@ func IsEnabledVersion(v unversioned.GroupVersion) bool {
return found
}

// EnabledVersions returns all enabled versions.
func EnabledVersions() (ret []unversioned.GroupVersion) {
for v := range enabledVersions {
ret = append(ret, v)
// EnabledVersions returns all enabled versions. Groups are randomly ordered, but versions within groups
// are priority order from best to worst
func EnabledVersions() []unversioned.GroupVersion {
ret := []unversioned.GroupVersion{}
for _, groupMeta := range groupMetaMap {
ret = append(ret, groupMeta.GroupVersions...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Map traversal are not stable, is this expect to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Map traversal are not stable, is this expect to be?

No. Godoc specifies: Groups are randomly ordered, but versions within groups are priority order from best to worst. Random ordering on groups works, but we need correct ordering on versions within those groups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are groups randomly ordered? What value does it provide?

When choosing how to encode with a legacy encoder, the first matching group is used. This is a natural method for getting all the GroupVersions and since the group will be traversed eventually, the group ordering doesn't matter. The version ordering within that group is critical.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on the version ordering. I'm just trying to understand why enabled
versions is random. It's fine for this issue, but I don't know that I
consider randomness correct in this context.

On Wed, Feb 10, 2016 at 8:31 AM, David Eads notifications@github.com
wrote:

In pkg/apimachinery/registered/registered.go
#20846 (comment)
:

@@ -114,22 +114,24 @@ func IsEnabledVersion(v unversioned.GroupVersion) bool {
return found
}

-// EnabledVersions returns all enabled versions.
-func EnabledVersions() (ret []unversioned.GroupVersion) {

  • for v := range enabledVersions {
  •   ret = append(ret, v)
    +// EnabledVersions returns all enabled versions. Groups are randomly ordered, but versions within groups
    +// are priority order from best to worst
    +func EnabledVersions() []unversioned.GroupVersion {
  • ret := []unversioned.GroupVersion{}
  • for _, groupMeta := range groupMetaMap {
  •   ret = append(ret, groupMeta.GroupVersions...)

Why are groups randomly ordered? What value does it provide?

When choosing how to encode with a legacy encoder, the first matching
group is used. This is a natural method for getting all the GroupVersions
and since the group will be traversed eventually, the group ordering
doesn't matter. The version ordering within that group is critical.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/20846/files#r52456327.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on the version ordering. I'm just trying to understand why enabled
versions is random. It's fine for this issue, but I don't know that I
consider randomness correct in this context.

I'm not sure that I know what order I'd like this to have. I know that I don't like alphabetical, because everyone will name theirs aaaa. Given package loading ordering which isn't obvious to me, I'm not sure that I like enablement order.

I think I want group ordering to be suggested by discover endpoints and strictly ordered client-side if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, ok, so this is a behavior change for ordering, I think after discussion we said this might break rest mapper because it relies on extensions/v1beta1.pods to come after v1.pods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm, ok, so this is a behavior change for ordering, I think after discussion we said this might break rest mapper because it relies on extensions/v1beta1.pods to come after v1.pods

That discussion was related to a different pull where I explicitly called out the change in the description: #20829 This has never had an ordering guarantee before.

}
return
return ret
}

// EnabledVersionsForGroup returns all enabled versions for a group.
func EnabledVersionsForGroup(group string) (ret []unversioned.GroupVersion) {
for v := range enabledVersions {
if v.Group == group {
ret = append(ret, v)
}
// EnabledVersionsForGroup returns all enabled versions for a group in order of best to worst
func EnabledVersionsForGroup(group string) []unversioned.GroupVersion {
groupMeta, ok := groupMetaMap[group]
if !ok {
return []unversioned.GroupVersion{}
}
return

return append([]unversioned.GroupVersion{}, groupMeta.GroupVersions...)
}

// Group returns the metadata of a group if the gruop is registered, otherwise
Expand Down
8 changes: 8 additions & 0 deletions pkg/runtime/serializer/versioning/versioning.go
View file
Open in desktop
Expand Up @@ -57,12 +57,20 @@ func NewCodec(
if encodeVersion != nil {
internal.encodeVersion = make(map[string]unversioned.GroupVersion)
for _, v := range encodeVersion {
// first one for a group wins. This is consistent with best to worst order throughout the codebase
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is fine with me

if _, ok := internal.encodeVersion[v.Group]; ok {
continue
}
internal.encodeVersion[v.Group] = v
}
}
if decodeVersion != nil {
internal.decodeVersion = make(map[string]unversioned.GroupVersion)
for _, v := range decodeVersion {
// first one for a group wins. This is consistent with best to worst order throughout the codebase
if _, ok := internal.decodeVersion[v.Group]; ok {
continue
}
internal.decodeVersion[v.Group] = v
}
}
Expand Down