Document MultiKueue migration to ClusterProfile accessProviders#12015
Document MultiKueue migration to ClusterProfile accessProviders#12015kahirokunn wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kahirokunn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis pull request updates the KEP-693 README to document a ClusterProfile API schema migration: introducing an ChangesClusterProfile API Schema Migration
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
63c701e to
ad61363
Compare
ad61363 to
1178c49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@keps/693-multikueue/README.md`:
- Around line 296-299: The README claims
`accessProviders`/`status.accessProviders` as the preferred source but the
codebase still validates and uses
`credentialsProviders`/`status.credentialProviders`; either change the prose to
state this is a planned/stack-dependent migration (explicitly note that
validation and controller wiring still rely on `credentialsProviders` and
`status.credentialProviders`) or update the implementation to match the doc by
switching validation and controller logic to prefer
`accessProviders`/`status.accessProviders`; specifically, modify the code paths
referenced (apis/config/v1beta2/configuration_types.go,
pkg/config/validation.go, and
pkg/controller/admissionchecks/multikueue/controllers.go) to read/validate
`accessProviders` and `status.accessProviders` (ensuring name precedence
semantics where `accessProviders` wins) or else adjust the README text to
clearly scope the change as migration-only and mention the existing fields
(`credentialsProviders`/`status.credentialProviders`) remain authoritative until
code is updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d3d46f9-c49b-4ae1-8386-baeacf81e089
📒 Files selected for processing (1)
keps/693-multikueue/README.md
kshalot
left a comment
There was a problem hiding this comment.
/lgtm
Thank you for aligning this!
|
LGTM label has been added. DetailsGit tree hash: da19f51fae24a9cdb6edfaa75ec8616839a36601 |
|
Although let's hold off merging this until #12011 is merged. I doubt the implementation will change much, so the KEP diff will likely stay the same, but just in case. |
| type ClusterProfileCredentialsProvider = ClusterProfileAccessProvider | ||
| ``` | ||
|
|
||
| The `credentialsProviders` field remains accepted for backwards compatibility. If both `accessProviders` and `credentialsProviders` are specified, the controller uses providers from both fields. When both fields specify a provider with the same name, the provider in `accessProviders` takes precedence. |
There was a problem hiding this comment.
I'm just curious why combining providers from both fields instead validate that they should be mutually exclusive.
|
/lgtm Thank you for making this change. Just a quick question re. the behavior when both are specified. |
|
@hdp617: changing LGTM is restricted to collaborators DetailsIn response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind kep
/area multikueue
What this PR does / why we need it:
Updates KEP-693 to document MultiKueue's migration from ClusterProfile
credentialsProviderstoaccessProviders.The Cluster Inventory API renamed the provider concept because the old
credentialProvidersfield actually carries cluster access information rather than credential material. The newaccessProvidersfield is now the preferred API, andcredentialProvidersis deprecated:The Cluster Inventory API still keeps compatibility for the deprecated field today, but that compatibility is expected to be removed in the future:
Because MultiKueue consumes ClusterProfile access information, this KEP update records the intent to migrate before the deprecated Cluster Inventory API field is removed. The KEP now describes
multiKueue.clusterProfile.accessProvidersas the preferred MultiKueue configuration field, keepscredentialsProvidersas a deprecated compatibility field, and records the precedence rule when both fields define the same provider name.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
This is a KEP-only follow-up for the API shape being implemented in #12011.
AI disclosure: This PR was written in part with the assistance of generative AI.
Does this PR introduce a user-facing change?
Summary by CodeRabbit
accessProvidersfield for cluster access provisioningcredentialsProvidersfield in favor ofaccessProviderswhile maintaining backward compatibility