Skip to content

Inject feature gate instance into client-go for kube components.#122738

Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
benluddy:client-go-feature-gate-adapter
Jan 17, 2024
Merged

Inject feature gate instance into client-go for kube components.#122738
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
benluddy:client-go-feature-gate-adapter

Conversation

@benluddy
Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind feature
/sig api-machinery

What this PR does / why we need it:

Registers all client-go features with the DefaultFeatureGate instance and wires client-go to perform feature checks using the same instance used for kube feature checks. For kube components, this means that client features will be exposed in the existing --feature-gates flag.

In order to avoid a dependency cycle between component-base and client-go, client-go maintains parallel definitions of component-base's feature types and constants. Passing kube's default feature gate instance to client-go requires an adapter.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

- [Proposal]: https://docs.google.com/document/d/1g9BGCRw-7ucUxO6OtCWbb3lfzUGA_uU9178wLdXAIfs/edit#heading=h.ookhkzz1jrsp

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 12, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 12, 2024
@benluddy
Copy link
Copy Markdown
Contributor Author

/assign @p0lyn0mial

/hold

Holding for proof PR demonstrating a client feature wired to a kube component.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2024
@benluddy benluddy force-pushed the client-go-feature-gate-adapter branch 2 times, most recently from 12b1839 to 969fa85 Compare January 12, 2024 16:49
"k8s.io/component-base/featuregate"
)

type clientFeatureGateAdapter struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you rename it to clientFeatureGatesAdapter ?
we use the plural form across https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/client-go/features

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed to clientAdapter. It adapts to both clientfeatures.Gates and clientfeatures.Registry and lives in a package named features, so I felt this was more reasonable than having separate adapters per client interface or naming the type based on only one of the implemented interfaces. WDYT?

@p0lyn0mial
Copy link
Copy Markdown
Contributor

Holding for proof PR demonstrating a client feature wired to a kube component.

opened a proof PR #122608 for KCM

@p0lyn0mial
Copy link
Copy Markdown
Contributor

@benluddy since #122571 merged you will need to slightly change TestKubeFeatures.

I already did that in #122608. Feel free to pull feaa22e in here.

@benluddy
Copy link
Copy Markdown
Contributor Author

@benluddy since #122571 merged you will need to slightly change TestKubeFeatures.

I already did that in #122608. Feel free to pull feaa22e in here.

I picked that commit into this PR and added a text description to the test cases to clarify the exact behaviors under test. Please double-check that my text is accurate.

@benluddy benluddy force-pushed the client-go-feature-gate-adapter branch from a52f66e to fe6b37d Compare January 16, 2024 17:20
benluddy and others added 2 commits January 16, 2024 12:51
In order to avoid a dependency cycle between component-base and client-go, client-go maintains
parallel definitions of component-base's feature types and constants. Passing kube's default feature
gate instance to client-go requires an adapter.
Co-authored-by: Ben Luddy <bluddy@redhat.com>
@benluddy benluddy force-pushed the client-go-feature-gate-adapter branch from fe6b37d to 8a32f34 Compare January 16, 2024 17:51
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Jan 16, 2024

/lgtm
/approve

@benluddy you may release hold when ready.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2024
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 4acd4062c4ff546fa210c8c87fd8d3ac7cf42806

@benluddy
Copy link
Copy Markdown
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2024
@benluddy
Copy link
Copy Markdown
Contributor Author

/assign @dims

PTAL on behalf of dep-approvers. This PR adds an import of k8s.io/client-go/features so there's a new corresponding line in vendor/modules.txt.

@dims
Copy link
Copy Markdown
Member

dims commented Jan 16, 2024

/assign @liggitt

@cici37
Copy link
Copy Markdown
Contributor

cici37 commented Jan 16, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 16, 2024
Copy link
Copy Markdown
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/approve
deps

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, deads2k, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2024
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jan 17, 2024

@benluddy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features 8a32f34 link false /test pull-kubernetes-e2e-gce-cos-alpha-features

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.

Details

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.

@p0lyn0mial
Copy link
Copy Markdown
Contributor

/test pull-kubernetes-verify

@benluddy
Copy link
Copy Markdown
Contributor Author

/test pull-kubernetes-verify

Looks like this was an example of #121865

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants