-
Notifications
You must be signed in to change notification settings - Fork 181
OCPBUGS-62366: remove beta enablement for resource api #1923
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.
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
base: main
Are you sure you want to change the base?
Conversation
/hold |
Note: it's late for 4.20.0, but we should backport it to the next 4.20.z stream /cherry-pick release-4.20 |
@tkashem: once the present PR merges, I will cherry-pick it on top of In 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. |
/lgtm |
@tkashem is this still WIP? /skip |
f7241b7
to
dc3a99f
Compare
WalkthroughRemoved the DynamicResourceAllocation entry from defaultGroupVersionsByFeatureGate in observe_runtime_config.go; VolumeAttributesClass remained unchanged aside from formatting. No exported/public API signatures were altered. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base
setting
📒 Files selected for processing (1)
pkg/operator/configobservation/apienablement/observe_runtime_config.go
(1 hunks)
var defaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]groupVersionByOpenshiftVersion{ | ||
"ValidatingAdmissionPolicy": {{GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}}, | ||
"DynamicResourceAllocation": { | ||
{KubeVersionRange: semver.MustParseRange("< 1.31.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1alpha2"}}, | ||
{KubeVersionRange: semver.MustParseRange(">= 1.31.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1alpha3"}}, | ||
{KubeVersionRange: semver.MustParseRange(">= 1.32.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1beta1"}}, | ||
}, | ||
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}}, | ||
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Re-add DynamicResourceAllocation mapping (enable resource.k8s.io/v1beta2) — current code can’t fulfill the PR objective
Without an entry for DynamicResourceAllocation here, RuntimeConfigFromFeatureGates will never emit resource.k8s.io/v1beta2=true, so the API won’t be enabled when the FG is set.
Apply this diff to gate v1beta2 on Kubernetes 1.33+:
var defaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]groupVersionByOpenshiftVersion{
"ValidatingAdmissionPolicy": {{GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}},
+ "DynamicResourceAllocation": {
+ {GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1beta2"}, KubeVersionRange: semver.MustParseRange(">=1.33.0")},
+ },
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}},
}
If we still support earlier Kubernetes minors in this branch, consider adding a non-overlapping fallback (e.g., v1beta1 with <1.33.0
) to avoid emitting multiple versions for the same group.
Run to verify usage and whitelist coverage:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for DynamicResourceAllocation references (whitelist/blacklist, observers, tests)..."
rg -n -C3 '\bDynamicResourceAllocation\b'
echo
echo "Where is GetDefaultGroupVersionByFeatureGate used?"
rg -n -C3 '\bGetDefaultGroupVersionByFeatureGate\b'
echo
echo "Any other resource.k8s.io version toggles to keep consistent?"
rg -n -C3 'resource\.k8s\.io'
Length of output: 14615
Re-add DynamicResourceAllocation mapping (enable resource.k8s.io/v1beta2)
defaultGroupVersionsByFeatureGate in pkg/operator/configobservation/apienablement/observe_runtime_config.go omits DynamicResourceAllocation, so RuntimeConfigFromFeatureGates will never emit resource.k8s.io/v1beta2=true when the feature gate is set — add:
var defaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]groupVersionByOpenshiftVersion{
"ValidatingAdmissionPolicy": {{GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}},
+ "DynamicResourceAllocation": {
+ {GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1beta2"}, KubeVersionRange: semver.MustParseRange(">=1.33.0")},
+ },
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}},
}
If older Kubernetes minors are supported in this branch, add a non‑overlapping fallback (e.g., older version with <1.33.0
) to avoid emitting multiple versions for the same group.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
var defaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]groupVersionByOpenshiftVersion{ | |
"ValidatingAdmissionPolicy": {{GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}}, | |
"DynamicResourceAllocation": { | |
{KubeVersionRange: semver.MustParseRange("< 1.31.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1alpha2"}}, | |
{KubeVersionRange: semver.MustParseRange(">= 1.31.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1alpha3"}}, | |
{KubeVersionRange: semver.MustParseRange(">= 1.32.0"), GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1beta1"}}, | |
}, | |
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}}, | |
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}}, | |
} | |
var defaultGroupVersionsByFeatureGate = map[configv1.FeatureGateName][]groupVersionByOpenshiftVersion{ | |
"ValidatingAdmissionPolicy": {{GroupVersion: schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}}}, | |
"DynamicResourceAllocation": { | |
{GroupVersion: schema.GroupVersion{Group: "resource.k8s.io", Version: "v1beta2"}, KubeVersionRange: semver.MustParseRange(">=1.33.0")}, | |
}, | |
"VolumeAttributesClass": {{GroupVersion: schema.GroupVersion{Group: "storage.k8s.io", Version: "v1beta1"}}}, | |
} |
dc3a99f
to
84fb83c
Compare
@haircommander i have made changes, let me know your thoughts:
|
SGTM! /lgtm (BTW, the title needs to be updated) |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, tkashem 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 |
/assign |
@tkashem: This pull request references OCPNODE-3758 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
@tkashem: This pull request references Jira Issue OCPBUGS-62366, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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 openshift-eng/jira-lifecycle-plugin repository. |
New changes are detected. LGTM label has been removed. |
enable resource v1beta2 api if the DRA feature gate DynamicResourceAllocation is enabled. v1beta2 has been added in 1.33
DynamicResourceAllocation is in GA with resource.k8s.io/v1 apis in 1.34. we enabled beta apis for DynamicResourceAllocation in tech or dev preview in 4.20 and previous versions. enablement of alpha/beta apis is defunct with 4.21 and onward
@tkashem: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Merging #1932 now, which should have the effect of disabling these GVs atomically with the merge of the 1.34 openshift/kubernetes rebase. Later, we can resume this PR to clean up the then-inert GV entries. |
@tkashem: This pull request references Jira Issue OCPBUGS-62366, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 openshift-eng/jira-lifecycle-plugin repository. |
/hold (rebase may be necessary after #1932 merges) |
This PR cleans up the alpha/beta enablement key for DRA. #1932 sets the upper bound for beta to
< 13.4.0