Enable major version segmentation of enabled feature gates #2637
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the feature gate system to introduce version-aware gating. The Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
payload-command/render/write_featureset.go (1)
209-245: Consider deterministic iteration order for reproducibility.Map iteration over
consolidatedGroupsis non-deterministic. While this doesn't affect correctness (each file has unique content), it may cause non-reproducible build artifacts when diffing generated outputs.🔎 Proposed fix for deterministic ordering
+ // Sort keys for deterministic output + var sortedKeys []profileFeatureSet + for key := range consolidatedGroups { + sortedKeys = append(sortedKeys, key) + } + sort.Slice(sortedKeys, func(i, j int) bool { + if sortedKeys[i].clusterProfile != sortedKeys[j].clusterProfile { + return sortedKeys[i].clusterProfile < sortedKeys[j].clusterProfile + } + return sortedKeys[i].featureSetName < sortedKeys[j].featureSetName + }) + // Generate files for each consolidated group - for groupKey, versionGroups := range consolidatedGroups { + for _, groupKey := range sortedKeys { + versionGroups := consolidatedGroups[groupKey] for _, group := range versionGroups {features/util.go (1)
89-134: Dead code:statusByClusterProfileByFeatureSetis no longer used.The field
statusByClusterProfileByFeatureSetonfeatureGateBuilder(line 98) and its initialization innewFeatureGate()(lines 124-132) appear to be legacy code that's no longer used after the refactor to the option-basedstatus []featureGateStatusapproach.🔎 Proposed cleanup
type featureGateBuilder struct { name string owningJiraComponent string responsiblePerson string owningProduct OwningProduct enhancementPRURL string status []featureGateStatus - - statusByClusterProfileByFeatureSet map[ClusterProfileName]map[configv1.FeatureSet]bool }func newFeatureGate(name string) *featureGateBuilder { b := &featureGateBuilder{ - name: name, - statusByClusterProfileByFeatureSet: map[ClusterProfileName]map[configv1.FeatureSet]bool{}, + name: name, } - for _, clusterProfile := range AllClusterProfiles { - byFeatureSet := map[configv1.FeatureSet]bool{} - for _, featureSet := range configv1.AllFixedFeatureSets { - byFeatureSet[featureSet] = false - } - b.statusByClusterProfileByFeatureSet[clusterProfile] = byFeatureSet - } return b }features/features.go (1)
81-83: Unused helper functioninCustomNoUpgrade().The
inCustomNoUpgrade()helper is defined but never used in any feature gate definitions. This may be intentional (CustomNoUpgrade is user-controlled), but if so, consider removing the unused function or adding a comment explaining its purpose.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumvendor/github.com/blang/semver/v4/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/range.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/semver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/sql.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (15)
features/features.gofeatures/okd_featureset_parity_test.gofeatures/util.gogo.modhack/update-payload-featuregates.shpayload-command/render/render.gopayload-command/render/write_featureset.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🧰 Additional context used
🧬 Code graph analysis (4)
features/okd_featureset_parity_test.go (2)
config/v1/types_feature.go (3)
Default(41-41)OKD(58-58)FeatureGateAttributes(136-144)insights/v1alpha1/types_insights.go (1)
Enabled(133-133)
features/features.go (2)
features/util.go (5)
ClusterProfileName(34-34)FeatureGateDescription(13-27)AllClusterProfiles(39-39)SelfManaged(38-38)Hypershift(37-37)config/v1/types_feature.go (3)
FeatureSet(37-37)FeatureGateAttributes(136-144)AllFixedFeatureSets(61-61)
features/util.go (1)
config/v1/types_feature.go (6)
FeatureSet(37-37)Default(41-41)TechPreviewNoUpgrade(45-45)DevPreviewNoUpgrade(49-49)CustomNoUpgrade(54-54)OKD(58-58)
payload-command/render/render.go (3)
features/util.go (1)
ClusterProfileName(34-34)features/features.go (1)
FeatureSets(8-37)config/v1/types_feature.go (3)
FeatureSet(37-37)FeatureGate(21-35)Default(41-41)
🔇 Additional comments (22)
hack/update-payload-featuregates.sh (1)
5-5: LGTM - Cleanup step ensures deterministic regeneration.The pre-generation cleanup prevents stale manifests from persisting when feature gates are removed or renamed. This is essential now that version-based gating may produce different file sets.
payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml (1)
6-7: Generated manifest includes new major-version annotation.The addition of
release.openshift.io/major-version: "4,5,6,7,8,9,10"aligns with the PR's goal of enabling version-based feature gating. The broad range (4-10) ensures this manifest applies across all supported major versions.payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml (1)
6-7: Consistent major-version annotation for Hypershift profile.The annotation follows the same pattern as other manifests, maintaining consistency across cluster profiles.
payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for TechPreviewNoUpgrade feature set.payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for DevPreviewNoUpgrade feature set.payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for Hypershift DevPreviewNoUpgrade.go.mod (1)
6-6: New semver dependency for version parsing.The
github.com/blang/semver/v4package is a well-established choice for semantic version parsing in Go. This dependency is actively used inpayload-command/render/render.goto parse payload versions viasemver.ParseTolerant(), enabling the version-aware feature gate computation introduced in this PR.features/okd_featureset_parity_test.go (1)
19-53: LGTM! Test correctly adapted to version-aware data structure.The test has been properly updated to handle the new nested structure returned by
AllFeatureSets(), adding an outer iteration level while preserving the core validation logic that ensures OKD includes all Default feature gates. The error reporting is enhanced with cluster profile context.payload-command/render/render.go (3)
73-76: Version parsing looks correct.Using
semver.ParseTolerantis appropriate for handling various payload version formats. The error handling ensures parsing failures are caught early.
94-94: Major version propagation implemented correctly.The payload major version is consistently extracted from the parsed semver and passed to all
FeatureSetscalls. The function signature update maintains the originalpayloadVersionstring parameter alongside the newpayloadMajorVersionparameter, ensuring both are available where needed.Also applies to: 115-115, 140-140, 169-169
11-11: No action needed — the semver library is at stable version v4.0.0 with no known security vulnerabilities.payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml (1)
6-7: This version range is consistent across all feature gate manifests in the repository and appears to be intentional rather than a placeholder. All 8 feature gate manifests use the identical annotation value "4,5,6,7,8,9,10", indicating this is a deliberate project-wide decision, likely for forward compatibility.payload-command/render/write_featureset.go (4)
24-46: LGTM!The
versionGroupstruct and its methods are well-designed. TheversionRangeString()handles single and range versions cleanly, andversions()correctly iterates the inclusive range.
48-72: LGTM!The equality comparison using sets is a clean approach. Both nil checks and the set-based equality for Enabled/Disabled slices are correctly implemented.
124-133: LGTM!The filename generation logic is clear. The fallback to "Default" for empty feature set names is appropriate.
168-187: LGTM!The legacy feature gate collection correctly traverses the new version-keyed structure and maintains the existing detection logic for gates predating 4.18.
features/util.go (3)
49-87: LGTM!The functional options pattern is cleanly implemented. The convenience functions like
inDefault(),inTechPreviewNoUpgrade(), etc., provide a readable API for feature gate configuration.
100-114: LGTM!The
isEnabledmethod correctly implements permissive matching for version and cluster profile (empty sets match all), while requiring explicit feature set membership. This design enables flexible gate configuration.
156-170: LGTM!The
enable()method correctly accumulates status entries, allowing for flexible OR-based gating logic across multiple calls.features/features.go (3)
8-37: LGTM!The
FeatureSetsfunction correctly evaluates each gate's status using OR semantics (enabled if any status matches) and properly classifies gates as enabled or disabled.
39-64: LGTM!The version collection logic correctly combines hardcoded future versions with explicitly configured ones. The nested map construction is clear. The comment about future-proofing until 2040 is helpful context.
66-70: LGTM!The updated
allFeatureGatesdeclaration and the consistent use of the newenable()API across all feature gate definitions is well-structured.
| // consolidateVersions groups consecutive versions with identical content | ||
| func consolidateVersions( | ||
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | ||
| clusterProfile features.ClusterProfileName, | ||
| featureSetName configv1.FeatureSet, | ||
| ) []versionGroup { | ||
| var versionContents []versionGroup | ||
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | ||
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | ||
| if content, exists := byFeatureSet[featureSetName]; exists { | ||
| versionContents = append(versionContents, versionGroup{ | ||
| startVersion: version, | ||
| content: content, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Sort by version | ||
| sort.Slice(versionContents, func(i, j int) bool { | ||
| return versionContents[i].startVersion < versionContents[j].startVersion | ||
| }) | ||
|
|
||
| var groups []versionGroup | ||
|
|
||
| currentGroup := versionContents[0] | ||
| currentGroup.endVersion = currentGroup.startVersion | ||
|
|
||
| for i := 1; i < len(versionContents); i++ { | ||
| curr := versionContents[i] | ||
|
|
||
| // Check if current version has same content as current group and is consecutive | ||
| if featureGateContentEqual(currentGroup.content, curr.content) && | ||
| curr.startVersion == currentGroup.endVersion+1 { | ||
| // Extend current group | ||
| currentGroup.endVersion = curr.startVersion | ||
| } else { | ||
| // Finalize current group and start new one | ||
| groups = append(groups, currentGroup) | ||
| currentGroup = curr | ||
| currentGroup.endVersion = currentGroup.startVersion | ||
| } | ||
| } | ||
|
|
||
| // Add the final group | ||
| groups = append(groups, currentGroup) | ||
|
|
||
| return groups | ||
| } |
There was a problem hiding this comment.
Panic on empty versionContents slice.
If no matching entries exist for the given clusterProfile and featureSetName, versionContents will be empty, causing a panic at line 99 when accessing versionContents[0].
🔎 Proposed fix
// Sort by version
sort.Slice(versionContents, func(i, j int) bool {
return versionContents[i].startVersion < versionContents[j].startVersion
})
+ if len(versionContents) == 0 {
+ return nil
+ }
+
var groups []versionGroup
currentGroup := versionContents[0]📝 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.
| // consolidateVersions groups consecutive versions with identical content | |
| func consolidateVersions( | |
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | |
| clusterProfile features.ClusterProfileName, | |
| featureSetName configv1.FeatureSet, | |
| ) []versionGroup { | |
| var versionContents []versionGroup | |
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | |
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | |
| if content, exists := byFeatureSet[featureSetName]; exists { | |
| versionContents = append(versionContents, versionGroup{ | |
| startVersion: version, | |
| content: content, | |
| }) | |
| } | |
| } | |
| } | |
| // Sort by version | |
| sort.Slice(versionContents, func(i, j int) bool { | |
| return versionContents[i].startVersion < versionContents[j].startVersion | |
| }) | |
| var groups []versionGroup | |
| currentGroup := versionContents[0] | |
| currentGroup.endVersion = currentGroup.startVersion | |
| for i := 1; i < len(versionContents); i++ { | |
| curr := versionContents[i] | |
| // Check if current version has same content as current group and is consecutive | |
| if featureGateContentEqual(currentGroup.content, curr.content) && | |
| curr.startVersion == currentGroup.endVersion+1 { | |
| // Extend current group | |
| currentGroup.endVersion = curr.startVersion | |
| } else { | |
| // Finalize current group and start new one | |
| groups = append(groups, currentGroup) | |
| currentGroup = curr | |
| currentGroup.endVersion = currentGroup.startVersion | |
| } | |
| } | |
| // Add the final group | |
| groups = append(groups, currentGroup) | |
| return groups | |
| } | |
| // consolidateVersions groups consecutive versions with identical content | |
| func consolidateVersions( | |
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | |
| clusterProfile features.ClusterProfileName, | |
| featureSetName configv1.FeatureSet, | |
| ) []versionGroup { | |
| var versionContents []versionGroup | |
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | |
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | |
| if content, exists := byFeatureSet[featureSetName]; exists { | |
| versionContents = append(versionContents, versionGroup{ | |
| startVersion: version, | |
| content: content, | |
| }) | |
| } | |
| } | |
| } | |
| // Sort by version | |
| sort.Slice(versionContents, func(i, j int) bool { | |
| return versionContents[i].startVersion < versionContents[j].startVersion | |
| }) | |
| if len(versionContents) == 0 { | |
| return nil | |
| } | |
| var groups []versionGroup | |
| currentGroup := versionContents[0] | |
| currentGroup.endVersion = currentGroup.startVersion | |
| for i := 1; i < len(versionContents); i++ { | |
| curr := versionContents[i] | |
| // Check if current version has same content as current group and is consecutive | |
| if featureGateContentEqual(currentGroup.content, curr.content) && | |
| curr.startVersion == currentGroup.endVersion+1 { | |
| // Extend current group | |
| currentGroup.endVersion = curr.startVersion | |
| } else { | |
| // Finalize current group and start new one | |
| groups = append(groups, currentGroup) | |
| currentGroup = curr | |
| currentGroup.endVersion = currentGroup.startVersion | |
| } | |
| } | |
| // Add the final group | |
| groups = append(groups, currentGroup) | |
| return groups | |
| } |
🤖 Prompt for AI Agents
In payload-command/render/write_featureset.go around lines 74 to 122, the
function can panic when versionContents is empty because it unconditionally
accesses versionContents[0]; fix by checking if len(versionContents) == 0 and
return an empty groups slice immediately. Ensure the early-return occurs before
accessing versionContents[0], preserving existing behavior for non-empty slices
(sorting and grouping) unchanged.
|
@JoelSpeed: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
This PR works in tandem with openshift/cluster-version-operator#1282 and an as yet unbuilt config operator PR to allow us to generate different versions of feature gates based on the target major version of the CVO payload.
This will allow us to have manifests that we deploy only in 4, or only in 5, or in a combination of releases. Importantly, it means that we can start setting up multiple major versions in development from the same branches, and gate CVO manifests, and feature gates, based on which major version is built into the build stream.