Testing changes for MCS Firstboot Pivot Failure Reporting v2 #6124
Draft
CourtCourt521 wants to merge 1 commit into
Draft
Testing changes for MCS Firstboot Pivot Failure Reporting v2 #6124CourtCourt521 wants to merge 1 commit into
CourtCourt521 wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe PR adds Machine Config Server (MCS) URL annotation to Ignition node configurations across two server modes: ClusterServer fetches Infrastructure to compute and wire the MCS host, while BootstrapServer uses empty defaults. A shared ChangesMachine Config Server URL Annotation in Ignition Configs
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 12❌ Failed checks (1 warning, 11 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CourtCourt521 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: 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 `@pkg/server/server_test.go`:
- Line 188: The tests currently call getNodeAnnotation and build a local
mcIgnCfg but never assert that MachineConfigServerURLAnnotationKey was written
or configure the test servers to emit that MCS URL; update the tests to (1)
configure the test server(s) used in these cases to return/emit the expected MCS
URL, (2) set the expected URL value and call
getNodeAnnotation(mp.Status.Configuration.Name, "", "<expected-url>", mc) as you
already do, and (3) add explicit assertions that the returned annotation map
contains MachineConfigServerURLAnnotationKey with the expected URL value (use
the same unique symbols: getNodeAnnotation, MachineConfigServerURLAnnotationKey,
and mcIgnCfg to locate where to change behavior and where to add the assertion).
Ensure both occurrences (around lines shown) are updated consistently.
In `@pkg/server/server.go`:
- Around line 322-347: GetIgnitionHost currently uses fmt.Sprintf("%s:%s",
internalURLParsed.Hostname(), securePortStr) which breaks for IPv6 and assumes
infraStatus and API IP slices are non-nil/non-empty; change the fallback to use
net.JoinHostPort(internalURLParsed.Hostname(), securePortStr) and add an initial
nil check for infraStatus to return an error if nil, then in each platform
branch (BareMetal/APIServerInternalIPs, OpenStack/APIServerInternalIPs,
Ovirt/APIServerInternalIPs, VSphere.APIServerInternalIPs) check that the slice
exists and len(...)>0 before indexing [0] to avoid panics, keeping the
SecurePort and ignitionHost assignment logic otherwise the same.
🪄 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: 0a4e5c0a-ff8b-478e-8e72-f71e85d2c927
📒 Files selected for processing (6)
pkg/daemon/constants/constants.gopkg/operator/sync.gopkg/server/bootstrap_server.gopkg/server/cluster_server.gopkg/server/server.gopkg/server/server_test.go
| t.Fatalf("unexpected error while appending file to ignition: %v", err) | ||
| } | ||
| anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", mc) | ||
| anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", "https://api-int.test.example.com:22623", mc) |
There was a problem hiding this comment.
These tests still don’t verify the new MCS URL annotation.
The hardcoded URL only goes into a local mcIgnCfg that never drives the final assertions, and the servers under test are not configured to emit that URL. These cases will keep passing even if MachineConfigServerURLAnnotationKey is never written.
Also applies to: 383-383
🤖 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/server/server_test.go` at line 188, The tests currently call
getNodeAnnotation and build a local mcIgnCfg but never assert that
MachineConfigServerURLAnnotationKey was written or configure the test servers to
emit that MCS URL; update the tests to (1) configure the test server(s) used in
these cases to return/emit the expected MCS URL, (2) set the expected URL value
and call getNodeAnnotation(mp.Status.Configuration.Name, "", "<expected-url>",
mc) as you already do, and (3) add explicit assertions that the returned
annotation map contains MachineConfigServerURLAnnotationKey with the expected
URL value (use the same unique symbols: getNodeAnnotation,
MachineConfigServerURLAnnotationKey, and mcIgnCfg to locate where to change
behavior and where to add the assertion). Ensure both occurrences (around lines
shown) are updated consistently.
Comment on lines
+322
to
+347
| func GetIgnitionHost(infraStatus *configv1.InfrastructureStatus) (string, error) { | ||
| internalURL := infraStatus.APIServerInternalURL | ||
| internalURLParsed, err := url.Parse(internalURL) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| securePortStr := strconv.Itoa(SecurePort) | ||
| ignitionHost := fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr) | ||
| if infraStatus.PlatformStatus != nil { | ||
| switch infraStatus.PlatformStatus.Type { | ||
| case configv1.BareMetalPlatformType: | ||
| ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs[0], securePortStr) | ||
| case configv1.OpenStackPlatformType: | ||
| ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs[0], securePortStr) | ||
| case configv1.OvirtPlatformType: | ||
| ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs[0], securePortStr) | ||
| case configv1.VSpherePlatformType: | ||
| if infraStatus.PlatformStatus.VSphere != nil { | ||
| if len(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs) > 0 { | ||
| ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs[0], securePortStr) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ignitionHost, nil |
There was a problem hiding this comment.
Harden GetIgnitionHost (IPv6 host:port + panic safety)
- Replace the fallback
fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr)withnet.JoinHostPort(...)so IPv6 literals get valid brackets. - Add
infraStatus == nilhandling and checklen(APIServerInternalIPs) > 0before indexing[0]in the BareMetal/OpenStack/Ovirt/VSphere branches to avoid panics.
🤖 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/server/server.go` around lines 322 - 347, GetIgnitionHost currently uses
fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr) which breaks
for IPv6 and assumes infraStatus and API IP slices are non-nil/non-empty; change
the fallback to use net.JoinHostPort(internalURLParsed.Hostname(),
securePortStr) and add an initial nil check for infraStatus to return an error
if nil, then in each platform branch (BareMetal/APIServerInternalIPs,
OpenStack/APIServerInternalIPs, Ovirt/APIServerInternalIPs,
VSphere.APIServerInternalIPs) check that the slice exists and len(...)>0 before
indexing [0] to avoid panics, keeping the SecurePort and ignitionHost assignment
logic otherwise the same.
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.
Testing not an actual pull request yet
Testing not an actual pull request
Testing not an actual pull request yet
Testing not an actual pull request yet
Summary by CodeRabbit
Enhancements