OCPBUGS-91974: Stop setting DevPreviewNoUpgrade for TNF clusters#10507
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@fonta-rh: This pull request references Jira Issue OCPBUGS-91974, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTNF support handling now distinguishes 4.22 from earlier fencing versions, and install-config FeatureSet selection follows that version boundary. The TNF tests were updated to cover the revised support-level and FeatureSet expectations. ChangesTNF version gating
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/S
|
@fonta-rh: This pull request references Jira Issue OCPBUGS-91974, which is invalid:
Comment 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 openshift-eng/jira-lifecycle-plugin repository. |
TNF clusters deployed via Agent-Based Install were unconditionally marked with DevPreviewNoUpgrade FeatureSet, blocking upgrades even on 4.22 where TNF is GA. Mirror the TNA pattern: set DevPreviewNoUpgrade only for 4.20 (minimum version), omit it for 4.22+ so the Default FeatureSet applies. Update getSupportLevel() to return DevPreview for 4.20 and Supported for later versions. Assisted-by: Claude Code <noreply@anthropic.com>
fonta-rh
force-pushed
the
fix/ocpbugs-91974-upgrade-gate
branch
from
d0d33ed to
d7570fb
Compare
openshift-ci
Bot
added
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/featuresupport/features_misc.go`:
- Around line 173-177: The version checks in the fencing support branch are
ignoring parse errors from common.BaseVersionEqual and
common.BaseVersionGreaterOrEqual, which can hide malformed
filters.OpenshiftVersion values. Update the logic in features_misc.go to handle
the returned errors explicitly in the same block around the fencing support
decision, and only return SupportLevelDevPreview or continue the support check
when the version parse succeeds; otherwise propagate or surface the error
instead of treating it as a normal unsupported-version case.
In `@internal/installcfg/builder/builder.go`:
- Around line 400-402: The TNF version check in handleFencing currently ignores
the error from common.BaseVersionEqual, which can let a malformed
cluster.OpenshiftVersion fall through to the default path. Update the
handleFencing logic in builder.go to capture and return that parse/comparison
error instead of ignoring it, so cfg.FeatureSet only gets set when the version
comparison succeeds. Use the existing handleFencing error return to fail closed
and avoid generating an install-config with the wrong feature set.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a524020-2af9-4794-bfe0-b1cbaf6756c2
📒 Files selected for processing (4)
internal/featuresupport/feature_support_test.gointernal/featuresupport/features_misc.gointernal/installcfg/builder/builder.gointernal/installcfg/builder/builder_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10507 +/- ##
==========================================
- Coverage 44.34% 44.33% -0.01%
==========================================
Files 423 423
Lines 73512 73517 +5
==========================================
+ Hits 32596 32597 +1
- Misses 37985 37987 +2
- Partials 2931 2933 +2
🚀 New features to boost your workflow:
|
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@fonta-rh: This pull request references Jira Issue OCPBUGS-91974, which is valid. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold finishing testing |
openshift-ci
Bot
added
the
do-not-merge/hold
TNF is TechPreview in 4.20-4.21 and GA from 4.22, not DevPreview as
previously set. Use BaseVersionGreaterOrEqual("4.22") to cover both
TechPreview versions instead of BaseVersionEqual(minVersion).
Assisted-by: Claude Code <noreply@anthropic.com>
|
/lgtm |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/installcfg/builder/builder_test.go (1)
250-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit 4.21 TNF FeatureSet assertion
Line 250 adds the GA (4.22) case, but there’s still no builder-layer assertion for 4.21. Adding it would lock in the intended 4.20–4.21 TechPreview window and catch boundary regressions quickly.
Suggested test addition
+ It("create_configuration_with_all_hosts - TNF cluster 4.21", func() { + certificateVerificationEnabled := installcfg.CertificateVerificationEnabled + fencingCredentials1 := models.FencingCredentialsParams{ + Address: swag.String("https://address1.example.com"), + CertificateVerification: swag.String(string(certificateVerificationEnabled)), + Password: swag.String("password"), + Username: swag.String("username"), + } + fencingCredentials2 := models.FencingCredentialsParams{ + Address: swag.String("https://address2.example.com"), + Password: swag.String("password"), + Username: swag.String("username"), + } + fencingCredentialsHost1String, err := json.Marshal(fencingCredentials1) + Expect(err).ShouldNot(HaveOccurred()) + fencingCredentialsHost2String, err := json.Marshal(fencingCredentials2) + Expect(err).ShouldNot(HaveOccurred()) + host1.FencingCredentials = string(fencingCredentialsHost1String) + host1.Role = models.HostRoleMaster + host2.FencingCredentials = string(fencingCredentialsHost2String) + host2.Role = models.HostRoleMaster + cluster.Hosts = []*models.Host{&host1, &host2} + cluster.ControlPlaneCount = 2 + cluster.OpenshiftVersion = "4.21" + + var result installcfg.InstallerConfigBaremetal + mockMirrorRegistriesConfigBuilder.EXPECT().IsMirrorRegistriesConfigured().Return(false).Times(2) + data, err := installConfig.GetInstallConfig(&cluster, clusterInfraenvs, "") + Expect(err).ShouldNot(HaveOccurred()) + err = json.Unmarshal(data, &result) + Expect(err).ShouldNot(HaveOccurred()) + Expect(result.FeatureSet).To(Equal(configv1.TechPreviewNoUpgrade)) + })🤖 Prompt for 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. In `@internal/installcfg/builder/builder_test.go` around lines 250 - 287, The TNF builder tests in the create_configuration_with_all_hosts coverage only assert the GA path for OpenshiftVersion 4.22, but they miss the 4.21 TechPreview boundary. Add a sibling test near the existing create_configuration_with_all_hosts - TNF cluster GA case that sets cluster.OpenshiftVersion to 4.21 and asserts the expected FeatureSet for the 4.20–4.21 window, using installConfig.GetInstallConfig and the resulting InstallerConfigBaremetal fields to verify the behavior.
🤖 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.
Nitpick comments:
In `@internal/installcfg/builder/builder_test.go`:
- Around line 250-287: The TNF builder tests in the
create_configuration_with_all_hosts coverage only assert the GA path for
OpenshiftVersion 4.22, but they miss the 4.21 TechPreview boundary. Add a
sibling test near the existing create_configuration_with_all_hosts - TNF cluster
GA case that sets cluster.OpenshiftVersion to 4.21 and asserts the expected
FeatureSet for the 4.20–4.21 window, using installConfig.GetInstallConfig and
the resulting InstallerConfigBaremetal fields to verify the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2359e869-6869-4f39-ae88-44152d7697e9
📒 Files selected for processing (4)
internal/featuresupport/feature_support_test.gointernal/featuresupport/features_misc.gointernal/installcfg/builder/builder.gointernal/installcfg/builder/builder_test.go
Assisted-by: Claude Code <noreply@anthropic.com>
| // TNF is GA from 4.22 | ||
| if isGA, _ := common.BaseVersionGreaterOrEqual("4.22", filters.OpenshiftVersion); isGA { |
There was a problem hiding this comment.
nit: should probably be a constant (TNAMinversion or something) but since we're rushing to get this in, can be a follow up PR
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh, rccrdpccl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/hold remove |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/cherrypick 4.22.3 |
|
@fonta-rh: once the present PR merges, I will cherry-pick it on top of 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. |
|
@fonta-rh: all tests passed! 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. |
|
@fonta-rh: Jira Issue OCPBUGS-91974: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-91974 has been moved to the MODIFIED state. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@fonta-rh: cannot checkout 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. |
|
/cherrypick release-4.23 |
|
@fonta-rh: new pull request created: #10515 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. |
|
/cherrypick release-4.22 |
|
@fonta-rh: new pull request created: #10516 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. |
|
/verified by fonta-rh |
|
@fonta-rh: Jira Issue OCPBUGS-91974 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state. 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 openshift-eng/jira-lifecycle-plugin repository. |
6 participants
Summary
DevPreviewNoUpgradeFeatureSet override fromhandleFencing()in the install-config builderTechPreviewtoSupported— TNF (DualReplica) is GA in 4.22Root Cause
handleFencing()ininternal/installcfg/builder/builder.gounconditionally setcfg.FeatureSet = configv1.DevPreviewNoUpgradefor every TNF cluster. This blocked upgrades with:FeatureGatesUpgradeable: "DevPreviewNoUpgrade" does not allow updates.The
DualReplicafeature gate is in theDefaultfeature set since 4.22, so no FeatureSet override is needed. The Arbiter (TNA) feature in the same file already handles this correctly — it only setsTechPreviewNoUpgradefor its minimum version (4.19).Test plan
go test ./internal/installcfg/builder/passesgo test ./internal/featuresupport/passesDevPreviewNoUpgradein cluster FeatureGatesAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit