[kubernetes/kubernetes] [WIP] add apiversion and kind to list subitems (#63972)
David Eads
Fixes #3030 (from 2014, would that be a record?)
The current state is ugly, but could be tidied up and tests added. I put a random test-cmd check in there for proof.
Any concerns about where and how I did this? It requires
- that scheme recognizes your list item
- that the list item is registered in the same GV as your list
- that you specify a specific (not generic) list.
I'd probably reconcile the pkg/api/meta cycle by pushing these into the pkg/runtime/scheme package.
@kubernetes/sig-api-machinery-bugs
@smarterclayton @liggitt @lavalamp @sttts
You can view, comment on, or merge this pull request online at:
https://github.com/kubernetes/kubernetes/pull/63972
Commit Summary
- add apiversion and kind to list subitems
File Changes
- M hack/make-rules/test-cmd-util.sh (4)
- M staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go (162)
Patch Links:
- https://github.com/kubernetes/kubernetes/pull/63972.patch
- https://github.com/kubernetes/kubernetes/pull/63972.diff
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
k8s-ci-robot
@deads2k: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: deads2k
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:hack/OWNERS[deads2k]staging/src/k8s.io/apimachinery/pkg/OWNERS[deads2k]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
k8s-ci-robot
@deads2k: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-kops-aws | 931943b | link | /test pull-kubernetes-e2e-kops-aws |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-test | 931943b | link | /test pull-kubernetes-bazel-test |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
Daniel Smith
@lavalamp commented on this pull request.
In staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go:
> if copy {
obj = obj.DeepCopyObject()
}
- setTargetKind(obj, kind)
- return obj, nil
+ err := s.setTargetKind(obj, kind)
+ return obj, err
Please return nil, err if err is not nil, makes it clear that obj can't be used if there was an error.
Daniel Smith
@lavalamp commented on this pull request.
In hack/make-rules/test-cmd-util.sh:
> @@ -4861,6 +4861,10 @@ runTests() {
kube::log::status "Creating namespace ${ns_name}"
kubectl create namespace "${ns_name}"
kubectl config set-context "${CONTEXT}" --namespace="${ns_name}"
+
+ # convenient spot to check for namespace kinds being set.
+ # TODO before merge, I'll make a real test
...and add a TODO that we probably need to do this for embedded types that don't happen to be list items, if we have any :(
Daniel Smith
How badly does this hurt performance? LGTM
Jordan Liggitt
How badly does this hurt performance?
I had the same question, it looks like a really complex inner loop. I'm imagining this running on every item listed across all resources on informer lists.
David Eads
I'll find the benchmark and run it. How much are we willing to tolerate to make life easier for every client? Some percentage of the entire encoding path I'd imagine
David Eads
@deads2k commented on this pull request.
In hack/make-rules/test-cmd-util.sh:
> @@ -4861,6 +4861,10 @@ runTests() {
kube::log::status "Creating namespace ${ns_name}"
kubectl create namespace "${ns_name}"
kubectl config set-context "${CONTEXT}" --namespace="${ns_name}"
+
+ # convenient spot to check for namespace kinds being set.
+ # TODO before merge, I'll make a real test
...and add a TODO that we probably need to do this for embedded types that don't happen to be list items, if we have any :(
Not as bad as you think. My test failures here indicate that we actually check this in fuzz tests, so I update the fuzz tests and we get full coverage.
David Eads
@deads2k commented on this pull request.
In staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go:
> if copy {
obj = obj.DeepCopyObject()
}
- setTargetKind(obj, kind)
- return obj, nil
+ err := s.setTargetKind(obj, kind)
+ return obj, err
Please return nil, err if err is not nil, makes it clear that obj can't be used if there was an error.
ok
Daniel Smith
On Thu, May 17, 2018 at 2:07 PM David Eads <notifi...@github.com> wrote:
> I'll find the benchmark and run it. How much are we willing to tolerate to
> make life easier for every client? Some percentage of the entire encoding
> path I'd imagine
>
> —
> Reply to this email directly, view it on GitHub
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAngloEQVXIeDM5oIWC2OdAUUfUya0SRks5tzeaMgaJpZM4UDNGT>
> .
David Eads
I had the same question, it looks like a really complex inner loop. I'm imagining this running on every item listed across all resources on informer lists.
Remember that this is only down the conversion path. Anything on external types doesn't get converted, just decoded. This won't impact the controllers on the read side. The real hit is server-side, where we already walk the list multiple times.
I think I'm willing to pay 5%, but I'm not sure about the scalability folks.
5% seems fair. It's a big behavior benefit.
@wojtek-t @shyamjvs is there a good existing benchmark in the code I should be looking at for a conversion change. We'll hit this on the api-server when serving a list.
Dr. Stefan Schimanski
@sttts commented on this pull request.
In staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go:
> +func isListType(obj Object) bool {
+ // if we're a runtime.Unstructured, check whether this is a list.
+ // TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object
+ if unstructured, ok := obj.(Unstructured); ok {
+ return unstructured.IsList()
+ }
+
+ _, err := getItemsPtr(obj)
+ return err == nil
+}
+
+// GetItemsPtr returns a pointer to the list object's Items member.
+// If 'list' doesn't have an Items member, it's not really a list type
+// and an error will be returned.
+// This function will either return a pointer to a slice, or an error, but not both.
+func getItemsPtr(list Object) (interface{}, error) {
this duplicated code here concerns me. Something is wrong with the layering.
Why not move meta.GetItemsPtr here?
Dr. Stefan Schimanski
@sttts commented on this pull request.
In staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go:
> +func isListType(obj Object) bool {
+ // if we're a runtime.Unstructured, check whether this is a list.
+ // TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object
+ if unstructured, ok := obj.(Unstructured); ok {
+ return unstructured.IsList()
+ }
+
+ _, err := getItemsPtr(obj)
+ return err == nil
+}
+
+// GetItemsPtr returns a pointer to the list object's Items member.
+// If 'list' doesn't have an Items member, it's not really a list type
+// and an error will be returned.
+// This function will either return a pointer to a slice, or an error, but not both.
+func getItemsPtr(list Object) (interface{}, error) {
Same for EachListItem.
Shyam JVS
is there a good existing benchmark in the code I should be looking at for a conversion change.
@deads2k I'm not sure what you're exactly looking for, but there a whole bunch of benchmarks under pkg/api/testing (especially conversion_test.go) that you might find useful.
I think I'm willing to pay 5%, but I'm not sure about the scalability folks.
IIUC that you mean 5% increase in list latencies, that might be ok. We have some slack b/w our current latencies and SLO for list calls (which is atm 5s for namespaced list calls and 10s for cluster-scoped ones).
cc @wojtek-t
David Eads
@deads2k commented on this pull request.
In staging/src/k8s.io/apimachinery/pkg/runtime/scheme.go:
> +func isListType(obj Object) bool {
+ // if we're a runtime.Unstructured, check whether this is a list.
+ // TODO: refactor GetItemsPtr to use an interface that returns []runtime.Object
+ if unstructured, ok := obj.(Unstructured); ok {
+ return unstructured.IsList()
+ }
+
+ _, err := getItemsPtr(obj)
+ return err == nil
+}
+
+// GetItemsPtr returns a pointer to the list object's Items member.
+// If 'list' doesn't have an Items member, it's not really a list type
+// and an error will be returned.
+// This function will either return a pointer to a slice, or an error, but not both.
+func getItemsPtr(list Object) (interface{}, error) {
this duplicated code here concerns me. Something is wrong with the layering.
Why not move meta.GetItemsPtr here?
From the description of a WIP pull: "I'd probably reconcile the pkg/api/meta cycle by pushing these into the pkg/runtime/scheme package.". I figure I'll expose them and meta will keep it's wrapper to here since that's where they make more sense.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: deads2k
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
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:hack/OWNERS[deads2k]- pkg/api/OWNERS
staging/src/k8s.io/apimachinery/pkg/OWNERS[deads2k]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
David Eads
No real difference for normal types, but lists are a more significant bump:
OLD BenchmarkReplicationControllerListConversion-8 100000 14848 ns/op
NEW BenchmarkReplicationControllerListConversion-8 100000 16719 ns/op
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | 931943b | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-bazel-test | 931943b | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-node-e2e | b7dd2a7 | link | /test pull-kubernetes-node-e2e |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-test | b7dd2a7 | link | /test pull-kubernetes-bazel-test |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | 931943b | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-node-e2e | b7dd2a7 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-bazel-test | b7dd2a7 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-verify | b7dd2a7 | link | /test pull-kubernetes-verify |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
David Eads
@deads2k pushed 1 commit.
- 303a1e2 POC make it fast
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
David Eads
Alright, got a POC commit that shows how to do this fast enough.
with specific methods BenchmarkReplicationControllerListConversion-8 100000 14819 ns/op
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: deads2k
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
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:hack/OWNERS[deads2k]- pkg/api/OWNERS
- pkg/apis/core/OWNERS
- staging/src/k8s.io/api/OWNERS
staging/src/k8s.io/apimachinery/pkg/OWNERS[deads2k]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
David Eads
@deads2k commented on this pull request.
In staging/src/k8s.io/api/core/v1/types.go:
> @@ -5090,3 +5091,20 @@ const (
// and data streams for a single forwarded connection
PortForwardRequestIDHeader = "requestID"
)
+
+func (l *ReplicationControllerList) EachListItem(fn func(runtime.Object) error) error {
+ for i := range l.Items {
+ if err := fn(&l.Items[i]); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+func (l *ReplicationControllerList) AssignTypes() error {
Who's got an appetite to generate these?
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-test | b7dd2a7 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-verify | b7dd2a7 | link | /test pull-kubernetes-verify |
| pull-kubernetes-node-e2e | 303a1e2 | link | /test pull-kubernetes-node-e2e |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | 931943b | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-bazel-test | b7dd2a7 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-verify | 303a1e2 | link | /test pull-kubernetes-verify |
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-test | b7dd2a7 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-node-e2e | 303a1e2 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-verify | 303a1e2 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-kops-aws | 303a1e2 | link | /test pull-kubernetes-e2e-kops-aws |
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 303a1e2 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-verify | 303a1e2 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-kops-aws | 303a1e2 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-bazel-test | 303a1e2 | link | /test pull-kubernetes-bazel-test |
k8s-ci-robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 303a1e2 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-verify | 303a1e2 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-kops-aws | 303a1e2 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-bazel-test | 303a1e2 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-kubemark-e2e-gce | 303a1e2 | link | /test pull-kubernetes-kubemark-e2e-gce |
Dr. Stefan Schimanski
@sttts commented on this pull request.
In staging/src/k8s.io/api/core/v1/types.go:
> @@ -5090,3 +5091,20 @@ const (
// and data streams for a single forwarded connection
PortForwardRequestIDHeader = "requestID"
)
+
+func (l *ReplicationControllerList) EachListItem(fn func(runtime.Object) error) error {
+ for i := range l.Items {
+ if err := fn(&l.Items[i]); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+func (l *ReplicationControllerList) AssignTypes() error {
Would a generic helper SetTypeMeta(gv) using an accessor do the same thing?
Dr. Stefan Schimanski
@sttts commented on this pull request.
In staging/src/k8s.io/api/core/v1/types.go:
> @@ -5090,3 +5091,20 @@ const (
// and data streams for a single forwarded connection
PortForwardRequestIDHeader = "requestID"
)
+
+func (l *ReplicationControllerList) EachListItem(fn func(runtime.Object) error) error {
+ for i := range l.Items {
+ if err := fn(&l.Items[i]); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+func (l *ReplicationControllerList) AssignTypes() error {
Also related: there was a discussion that we might want to generate register.go.
k8s-ci-robot
@deads2k: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
David Xia
Hey, are there any updates on this? I'd love to see this fixed as it's convenient to have the Kind and APIVersion for each resource. Let me know how I can help move this along.
fejta-bot
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
Kubernetes Prow Robot
Closed #63972.
Kubernetes Prow Robot
@fejta-bot: Closed this PR.
In response to this:
Rotten issues close after 30d of inactivity.
Reopen the issue with/reopen.
Mark the issue as fresh with/remove-lifecycle rotten.Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
Nate Fonseka
/remove-lifecycle rotten
Nate Fonseka
/reopen
Kubernetes Prow Robot
@nfons: You can't reopen an issue/PR unless you authored it or you are a collaborator.
In response to this:
/reopen
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
Nikhita Raghunath
Reopening based on ^^
/reopen
Nikhita Raghunath
Kubernetes Prow Robot
Reopened #63972.
Kubernetes Prow Robot
@nikhita: Reopened this PR.
In response to this:
/reopen
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
Kubernetes Prow Robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: deads2k
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.
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
—
Kubernetes Prow Robot
—
Kubernetes Prow Robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 303a1e2 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-verify | 303a1e2 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-kops-aws | 303a1e2 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-kubemark-e2e-gce | 303a1e2 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-gce | 303a1e2 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-bazel-test | 303a1e2 | link | /test pull-kubernetes-bazel-test |
Kubernetes Prow Robot
Kubernetes Prow Robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
Kubernetes Prow Robot
@deads2k: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
fejta-bot
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
Kubernetes Prow Robot
@fejta-bot: Closed this PR.
In response to this:
Rotten issues close after 30d of inactivity.
Reopen the issue with/reopen.
Mark the issue as fresh with/remove-lifecycle rotten.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
Kubernetes Prow Robot
Closed #63972.
halfcrazy
Any updates?
Yu-han Lin
any update?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Steven E. Harris
The lack of this fix bit me again today.
Frank Greco Jr
I just ran into this
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are on a team that was mentioned.