Add generics policy for kubernetes #6693

BenTheElder · 2022-06-07T21:09:00Z

as discussed in the last SIG Architecture meeting

Which issue(s) this PR fixes:

sig-architecture/generics.md

BenTheElder · 2022-06-09T18:16:31Z

sig-architecture/generics.md

+Consider if proposed generics pull requests improve maintainability and readability.
+
+The current generics implementation is known to have some performance issues
+depending on usage: consider requesting benchmarks before / after the changes.


dims · 2022-06-09T18:17:06Z

/assign @johnbelamaric @derekwaynecarr
/approve
/lgtm
/hold for additional reviewers

dims · 2022-06-09T18:18:35Z

/retitle Add generics policy for kubernetes

sig-architecture/generics.md

liggitt

looks like a good starting point, had a couple questions

liggitt · 2022-07-28T15:42:06Z

sig-architecture/generics.md

+
+Generics may be used in Kubernetes starting in v1.25
+
+Avoid generics when writing bug fixes until v1.24 is [out of support][version-support], to ease


+1 to this guidance

liggitt · 2022-07-28T15:50:21Z

sig-architecture/generics.md

+in case we ran into other issues and needed to roll back to unblock the release.
+Now that v1.24.0 is out, use of generics should be unrestricted.
+
+# Generics Policy


should we have guidance for use of generics in our libraries consumed by multiple branches of kubernetes, like k8s.io/utils, k8s.io/kube-openapi, etc? xref discussion in kubernetes/utils#243 (comment)

Great point, we only discussed this from the staging library angle previously.

We should add a note similar to the bug fixes guidance, but as a hard requirement, I think.

I rewrote this a bit to position limitations until v1.24 is out of support and address this point.

looks fine as a starting point. we could maybe relax the guidance to allow use of generics in those repos in ways that would still support go1.17 (go-version conditional files, etc), but I'm not familiar with how possible/fiddly that is... we'd want to be really confident in the implications before doing so, I guess

Even if technically possible (I think so with build tags?) I don’t think we want to allow this versus waiting a few more releases just because it’s very similar to starting to support multiple branches of these libraries from a support perspective and we don’t do that today in any I can think of.

liggitt · 2022-07-28T15:50:59Z

sig-architecture/generics.md

+- Consider if proposed generics pull requests improve maintainability and readability.
+
+- The current generics implementation is known to have some performance issues
+depending on usage: consider requesting benchmarks before / after the changes.


are there any links to the known perf issues to give hints to where generics might not be appropriate yet?

I didn't want to put information into this doc that would become out of date as go improves and measuring > "knowing", but in the initial go1.18 implementation at least generics are only partially monomorphized by GCShape and have an additional indirection at runtime for the method dictionaries.

So they pretty universally cost more, but that may still be justified by type safety and maintainability if you can keep it out of hot paths => avoid developer pain from code generation.

https://github.com/golang/proposal/blob/master/design/generics-implementation-dictionaries-go1.18.md

https://planetscale.com/blog/generics-can-make-your-go-code-slower

I'd like to think a suitable usage for now looks like:
https://github.com/kubernetes-sigs/oci-proxy/blob/f60b1fc751c6a65225aee8f9363d32cb8e0aa54b/pkg/net/cidrs/triemap.go
(not in hotpath, still typesafe, used internally)

I didn't want to put information into this doc that would become out of date as go improves and measuring > "knowing"

fair enough

liggitt · 2022-07-29T18:43:35Z

lgtm

thockin · 2022-07-29T20:09:25Z

I like it!

BenTheElder · 2022-07-29T20:12:01Z

sig-architecture/generics.md

+In Kubernetes v1.24 we shipped Go 1.18 (which adds support for generics)
+rather late in the release cycle, so we temporarily prohibited using generics
+in case we ran into other issues and needed to roll back to unblock the release.
+Now that v1.24.0 is out, use of generics should be unrestricted.


nit: this says "[...] use of generics should be unrestricted" but then below we now say "Generics may be used in Kubernetes starting in v1.25, with the following restrictions applying only until v1.24 is [out of support]"

may simply change this to "[...] use of generics should be allowed."

as discussed in the last SIG Architecture meeting

dims · 2022-08-02T12:22:16Z

/approve
/lgtm

/hold cancel

Let's merge and iterate if needed

k8s-ci-robot · 2022-08-02T12:22:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims

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:

~~sig-architecture/OWNERS~~ [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dims · 2022-08-16T20:36:34Z

saving some urls here for later :)

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 7, 2022

k8s-ci-robot requested review from derekwaynecarr and johnbelamaric June 7, 2022 21:09

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jun 7, 2022

dims reviewed Jun 9, 2022

View reviewed changes

sig-architecture/generics.md Outdated Show resolved Hide resolved

BenTheElder force-pushed the add-generics-draft branch from c74b1ee to 374ad75 Compare June 9, 2022 18:15

BenTheElder commented Jun 9, 2022

View reviewed changes

BenTheElder changed the title ~~[WIP] add generics policy draft~~ add generics policy draft Jun 9, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2022

k8s-ci-robot assigned derekwaynecarr Jun 9, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022

k8s-ci-robot assigned johnbelamaric and dims Jun 9, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 9, 2022

BenTheElder force-pushed the add-generics-draft branch from 374ad75 to 1553956 Compare June 9, 2022 18:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2022

k8s-ci-robot changed the title ~~add generics policy draft~~ Add generics policy for kubernetes Jun 9, 2022

sftim reviewed Jul 11, 2022

View reviewed changes

sig-architecture/generics.md Outdated Show resolved Hide resolved

BenTheElder force-pushed the add-generics-draft branch from 1553956 to d76520c Compare July 28, 2022 02:35

liggitt reviewed Jul 28, 2022

View reviewed changes

BenTheElder force-pushed the add-generics-draft branch from d76520c to 585e120 Compare July 29, 2022 17:27

k8s-ci-robot 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 29, 2022

BenTheElder commented Jul 29, 2022

View reviewed changes

add generics policy draft

66d09b8

as discussed in the last SIG Architecture meeting

BenTheElder force-pushed the add-generics-draft branch from 585e120 to 66d09b8 Compare July 29, 2022 20:13

BenTheElder mentioned this pull request Aug 1, 2022

don't use generics yet kubernetes/test-infra#26989

Closed

palnabarun mentioned this pull request Aug 1, 2022

Update sigs.k8s.io/boskos kubernetes-sigs/kubetest2#207

Merged

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 2, 2022

k8s-ci-robot merged commit 692d830 into kubernetes:master Aug 2, 2022

BenTheElder deleted the add-generics-draft branch August 2, 2022 19:19


		Generics may be used in Kubernetes starting in v1.25

		Avoid generics when writing bug fixes until v1.24 is [out of support][version-support], to ease

Add generics policy for kubernetes #6693

Add generics policy for kubernetes #6693

Uh oh!

Conversation

BenTheElder commented Jun 7, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Jun 9, 2022

Uh oh!

dims commented Jun 9, 2022

Uh oh!

Uh oh!

liggitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jul 29, 2022

Uh oh!

thockin commented Jul 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Aug 2, 2022

Uh oh!

k8s-ci-robot commented Aug 2, 2022

Uh oh!

dims commented Aug 16, 2022

Uh oh!

Uh oh!

liggitt Jul 29, 2022 •

edited

Loading

BenTheElder Jul 29, 2022 •

edited

Loading