Enable platform external s390x#10588
Open
Neeraj8418 wants to merge 2 commits into
Open
Conversation
- Add s390x to supported architectures for external platform - Update agent image generation to support s390x external platform - Modify kernel arguments handling for s390x - Update install config validation for s390x external platform
Contributor
|
[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 |
16 tasks
WalkthroughThis PR adds IBM Z (s390x) external platform support to the OpenShift agent components. It introduces a new platform constant, expands validation to permit external platforms on s390x architectures, implements platform-specific kernel console arguments, and updates logging to reference platforms generically rather than OCI-specifically. ChangesIBM Z External Platform Support
🎯 2 (Simple) | ⏱️ ~12 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)
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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/agent/installconfig.go (1)
137-154: ⚡ Quick winConsider refactoring duplicated validation logic.
The OCI and IBM Z platform validation blocks (lines 139-145 and 147-153) are nearly identical, differing only in the constant name. This duplication could be reduced by extracting the validation into a helper function or iterating over a list of platforms that require external cloud controller manager.
♻️ Proposed refactor to eliminate duplication
func (a *OptionalInstallConfig) validatePlatformsByName(installConfig *types.InstallConfig) field.ErrorList { var allErrs field.ErrorList if installConfig.Platform.Name() == external.Name { - // Validate OCI platform requirements - if installConfig.Platform.External.PlatformName == ExternalPlatformNameOci && - installConfig.Platform.External.CloudControllerManager != external.CloudControllerManagerTypeExternal { - fieldPath := field.NewPath("platform", "external", "cloudControllerManager") - allErrs = append(allErrs, field.Invalid(fieldPath, installConfig.Platform.External.CloudControllerManager, - fmt.Sprintf("When using external %s platform, %s must be set to %s", - ExternalPlatformNameOci, fieldPath, external.CloudControllerManagerTypeExternal))) - } - // Validate IBM Z platform requirements - if installConfig.Platform.External.PlatformName == ExternalPlatformNameIBMZ && - installConfig.Platform.External.CloudControllerManager != external.CloudControllerManagerTypeExternal { - fieldPath := field.NewPath("platform", "external", "cloudControllerManager") - allErrs = append(allErrs, field.Invalid(fieldPath, installConfig.Platform.External.CloudControllerManager, - fmt.Sprintf("When using external %s platform, %s must be set to %s", - ExternalPlatformNameIBMZ, fieldPath, external.CloudControllerManagerTypeExternal))) - } + // Validate external platform requirements for platforms requiring external CCM + platformsRequiringExternalCCM := []string{ExternalPlatformNameOci, ExternalPlatformNameIBMZ} + for _, platformName := range platformsRequiringExternalCCM { + if installConfig.Platform.External.PlatformName == platformName && + installConfig.Platform.External.CloudControllerManager != external.CloudControllerManagerTypeExternal { + fieldPath := field.NewPath("platform", "external", "cloudControllerManager") + allErrs = append(allErrs, field.Invalid(fieldPath, installConfig.Platform.External.CloudControllerManager, + fmt.Sprintf("When using external %s platform, %s must be set to %s", + platformName, fieldPath, external.CloudControllerManagerTypeExternal))) + break + } + } }🤖 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 `@pkg/asset/agent/installconfig.go` around lines 137 - 154, Refactor the duplicated OCI/IBM Z validation by extracting the repeated logic into a small helper (e.g., validateExternalPlatformCCM) that takes the platform name constant (ExternalPlatformNameOci or ExternalPlatformNameIBMZ), the installConfig.Platform.External values, the shared fieldPath construction, and the allErrs slice; the helper should check External.PlatformName == provided name && External.CloudControllerManager != external.CloudControllerManagerTypeExternal and append the same field.Invalid error using fieldPath and installConfig.Platform.External.CloudControllerManager. Replace the two nearly identical blocks with either calls to that helper for each constant or a loop over []string{ExternalPlatformNameOci, ExternalPlatformNameIBMZ} invoking the helper to keep behavior identical.
🤖 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 `@pkg/asset/agent/installconfig.go`:
- Around line 137-154: Refactor the duplicated OCI/IBM Z validation by
extracting the repeated logic into a small helper (e.g.,
validateExternalPlatformCCM) that takes the platform name constant
(ExternalPlatformNameOci or ExternalPlatformNameIBMZ), the
installConfig.Platform.External values, the shared fieldPath construction, and
the allErrs slice; the helper should check External.PlatformName == provided
name && External.CloudControllerManager !=
external.CloudControllerManagerTypeExternal and append the same field.Invalid
error using fieldPath and
installConfig.Platform.External.CloudControllerManager. Replace the two nearly
identical blocks with either calls to that helper for each constant or a loop
over []string{ExternalPlatformNameOci, ExternalPlatformNameIBMZ} invoking the
helper to keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ad641a8-227a-449c-b6d1-7f8e83fd8d92
📒 Files selected for processing (4)
pkg/asset/agent/common.gopkg/asset/agent/image/agentimage.gopkg/asset/agent/image/kargs.gopkg/asset/agent/installconfig.go
Contributor
|
@Neeraj8418: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enable
platform: externalsupport for s390x architectures in assisted-service.Changes
GetActualCreateClusterPlatformParams()ininternal/provider/platform.goProblem Solved
Fixes runtime error:
Can't set external platform on s390x architecturePreviously, s390x were restricted to
platform: none. This PR enablesplatform: externalfor custom cloud platforms (OCI, Nutanix, etc.) and edge computing scenarios.Signed-off-by: Neeraj Mishra neeraj.mishra3@ibm.com
Summary by CodeRabbit