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

WIP - set apiVersion/kind in list items #80618

Closed
wants to merge 8 commits into from
Closed

WIP - set apiVersion/kind in list items #80618

wants to merge 8 commits into from

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jul 26, 2019

What type of PR is this?
/kind api-change
/kind bug

What this PR does / why we need it:

Explores fixing #3030 by:

  • generating code in conversion to populate apiVersion/kind in external types
  • avoiding clearing apiVersion/kind when decoding into external types

This is very WIP/experimental. Still need to:

  • discuss this approach to populating apiVersion/kind
  • measure allocation impact to list conversions (cc @kubernetes/sig-scalability-pr-reviews)

If we move forward with this approach, still need to:

  • clean up the GVK-clearing decoder
  • figure out a better way to avoid short-circuit memory-equivalent unsafe copy of list items
  • add tests around proto decoding preserving GVK in lists and individual items

Which issue(s) this PR fixes:
Fixes #3030
Fixes #80609

Does this PR introduce a user-facing change?:

`apiVersion` and `kind` fields are now correctly populated in individual list items

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/apiserver area/code-generation area/kubeadm area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 26, 2019
neolit123
neolit123 reviewed Jul 28, 2019
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@kubernetes/sig-cluster-lifecycle-pr-reviews
@rosti @vincepri

@neolit123
Copy link
Member

cc @fabriziopandini

@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 29, 2019
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 28, 2019
@matteoolivi matteoolivi mentioned this pull request Mar 21, 2021
Open
@k8s-ci-robot k8s-ci-robot added area/cloudprovider sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 23, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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

@k8s-ci-robot
Copy link
Contributor

@liggitt: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test 8d917ea link /test pull-kubernetes-bazel-test
pull-kubernetes-kubemark-e2e-gce-big 8d917ea link /test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-unit 80d5262 link /test pull-kubernetes-unit
pull-kubernetes-integration 80d5262 link /test pull-kubernetes-integration
pull-kubernetes-verify 80d5262 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.

@liggitt liggitt marked this pull request as draft April 2, 2021 14:41
@ritazh ritazh added this to In Review (v1.22) in SIG Auth Old Apr 9, 2021
@enj enj moved this from In Review (v1.22) to Needs Triage PRs in SIG Auth Old Apr 9, 2021
@liggitt liggitt removed this from Needs Triage PRs in SIG Auth Old Apr 16, 2021
@k8s-ci-robot
Copy link
Contributor

@liggitt: 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2021
@enj enj added this to Needs Triage PRs in SIG Auth Old Apr 28, 2021
@liggitt liggitt removed this from Needs Triage PRs in SIG Auth Old Apr 30, 2021
@liggitt liggitt removed this from Waiting on Author in SIG Node PR Triage Apr 30, 2021
@ehashman ehashman added this to Waiting on Author in SIG Node PR Triage Apr 30, 2021
@enj enj added this to Needs Triage PRs in SIG Auth Old May 3, 2021
@liggitt liggitt removed this from Needs Triage PRs in SIG Auth Old May 7, 2021
@liggitt liggitt closed this May 7, 2021
SIG Node PR Triage automation moved this from Waiting on Author to Done May 7, 2021
@Jefftree Jefftree mentioned this pull request Aug 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

@bsalamat bsalamat Awaiting requested review from bsalamat

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/kubeadm area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
SIG Node PR Triage
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decoding should not clear apiVersion/kind REST api - 'kind' property not always exists on entities
6 participants
@liggitt @neolit123 @fejta-bot @bai @k8s-ci-robot @justaugustus