MGMT-24243: Add option for a delay when applying static network config on minimal ISO#10373
Open
giladravid16 wants to merge 1 commit into
Open
MGMT-24243: Add option for a delay when applying static network config on minimal ISO#10373giladravid16 wants to merge 1 commit into
giladravid16 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-24243 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 bug to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughThis PR adds an optional NetworkDiscoveryDelaySeconds field and wires it through API types, CRDs, Swagger, backend models/validation, controller and inventory create/update, nmstatectl systemd unit generation, the pre-network script runtime, ISO generation, and tests. ChangesNetwork Discovery Delay Configuration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
openshift-ci
Bot
added
the
size/L
openshift-ci
Bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/common/nmstatectl_service.go (1)
24-24: ⚡ Quick winConsider adding input validation for discoveryDelaySeconds.
The function does not validate that
discoveryDelaySecondsis non-negative. While the model layer enforcesminimum=0, adding a validation check here makes the function more robust and self-documenting, especially if refactoring changes the call path in the future.🛡️ Proposed defensive validation
func FormatMinimalISONetworkConfigServiceNmstatectl(discoveryDelaySeconds int64) (string, error) { + if discoveryDelaySeconds < 0 { + return "", fmt.Errorf("discoveryDelaySeconds must be non-negative, got %d", discoveryDelaySeconds) + } params := map[string]int64{🤖 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/common/nmstatectl_service.go` at line 24, FormatMinimalISONetworkConfigServiceNmstatectl currently accepts discoveryDelaySeconds without validation; add a defensive check at the start of FormatMinimalISONetworkConfigServiceNmstatectl to ensure discoveryDelaySeconds >= 0 and return a descriptive error (e.g., "discoveryDelaySeconds must be non-negative") when it is negative, so callers immediately get clear feedback; update any callers/tests if they rely on negative values.internal/common/nmstatectl_service_test.go (1)
8-14: ⚡ Quick winConsider expanding test coverage for edge cases.
The test verifies the core functionality with
delay=5, but additional test cases would improve confidence:
delay=0(default/no-delay case)- A larger delay value (e.g., 300 seconds) to ensure the timeout formula handles various scales
- Negative input to verify error handling (if validation is added per the previous suggestion)
📋 Example additional test cases
It("handles zero delay correctly", func() { content, err := FormatMinimalISONetworkConfigServiceNmstatectl(0) Expect(err).ToNot(HaveOccurred()) Expect(content).To(ContainSubstring("Environment=DISCOVERY_DELAY_SECONDS=0")) Expect(content).To(ContainSubstring("TimeoutSec=60")) }) It("handles large delay values", func() { content, err := FormatMinimalISONetworkConfigServiceNmstatectl(300) Expect(err).ToNot(HaveOccurred()) Expect(content).To(ContainSubstring("Environment=DISCOVERY_DELAY_SECONDS=300")) Expect(content).To(ContainSubstring("TimeoutSec=360")) })🤖 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/common/nmstatectl_service_test.go` around lines 8 - 14, Add edge-case tests for FormatMinimalISONetworkConfigServiceNmstatectl: keep the existing positive case and add tests for delay=0 (expect Environment=DISCOVERY_DELAY_SECONDS=0 and TimeoutSec=60) and a large delay like 300 (expect Environment=DISCOVERY_DELAY_SECONDS=300 and TimeoutSec=360); also add a test for a negative delay to assert the function either returns an error or handles validation consistently with any prior change (i.e., expect an error if you implemented validation, or the clamped/normalized output otherwise). Reference the FormatMinimalISONetworkConfigServiceNmstatectl function in these new It blocks and assert both the Environment=DISCOVERY_DELAY_SECONDS=... entry and the computed TimeoutSec value for each case.
🤖 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/bminventory/inventory_test.go`:
- Around line 11798-11813: The gzip.Reader created as gzipReader in the test is
never closed; after calling gzip.NewReader(responsePayload) add an immediate
defer gzipReader.Close() so the reader is always closed on return or failure (or
explicitly call gzipReader.Close() before any early return/Fail if you prefer
explicit close), referencing the gzipReader variable created in this test
function to ensure resources are released.
In `@internal/bminventory/inventory.go`:
- Around line 5611-5615: The update logic currently only applies non-nil
NetworkDiscoveryDelaySeconds, so clearing the override via a nil in params
leaves the old value in the DB; modify the branch handling
params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds to also detect when it
is nil while infraEnv.NetworkDiscoveryDelaySeconds is non-nil and add
updates["network_discovery_delay_seconds"] = nil (or the DB-null equivalent used
in this repo) so the stored override is cleared and the system falls back to
NETWORK_DISCOVERY_DELAY_SECONDS.
In `@internal/controller/controllers/infraenv_controller.go`:
- Around line 242-247: The reconciliation currently only sets
updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds when
infraEnv.Spec.NetworkDiscoveryDelaySeconds is non-nil, so clearing the field
(nil) never sends an update; modify the logic around
infraEnv.Spec.NetworkDiscoveryDelaySeconds /
internalInfraEnv.NetworkDiscoveryDelaySeconds to also detect when the spec is
nil but internalInfraEnv.NetworkDiscoveryDelaySeconds is non-nil and in that
case set updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = nil
(i.e., explicitly request clearing the backend value); keep the existing branch
that updates when values differ and do nothing only when both are nil or equal.
In `@restapi/embedded_spec.go`:
- Around line 9277-9282: The embedded schema for the property
"network_discovery_delay_seconds" in restapi/embedded_spec.go is missing the
"minimum: 0" constraint that appears in the flattened schema; update the
embedded-spec generation/propagation so the generated JSON schema for the field
"network_discovery_delay_seconds" includes "minimum": 0 (the same constraint
present in the flattened output), by adjusting the code path that builds the
embedded property/field (search for where "network_discovery_delay_seconds" or
the generator that emits integer/format int64 properties is assembled) to emit
the minimum constraint consistently across both embedded and flattened schemas.
---
Nitpick comments:
In `@internal/common/nmstatectl_service_test.go`:
- Around line 8-14: Add edge-case tests for
FormatMinimalISONetworkConfigServiceNmstatectl: keep the existing positive case
and add tests for delay=0 (expect Environment=DISCOVERY_DELAY_SECONDS=0 and
TimeoutSec=60) and a large delay like 300 (expect
Environment=DISCOVERY_DELAY_SECONDS=300 and TimeoutSec=360); also add a test for
a negative delay to assert the function either returns an error or handles
validation consistently with any prior change (i.e., expect an error if you
implemented validation, or the clamped/normalized output otherwise). Reference
the FormatMinimalISONetworkConfigServiceNmstatectl function in these new It
blocks and assert both the Environment=DISCOVERY_DELAY_SECONDS=... entry and the
computed TimeoutSec value for each case.
In `@internal/common/nmstatectl_service.go`:
- Line 24: FormatMinimalISONetworkConfigServiceNmstatectl currently accepts
discoveryDelaySeconds without validation; add a defensive check at the start of
FormatMinimalISONetworkConfigServiceNmstatectl to ensure discoveryDelaySeconds
>= 0 and return a descriptive error (e.g., "discoveryDelaySeconds must be
non-negative") when it is negative, so callers immediately get clear feedback;
update any callers/tests if they rely on negative values.
🪄 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: 98b4354a-73dd-4aec-85ac-ee10f71e81a3
⛔ Files ignored due to path filters (12)
api/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (16)
api/v1beta1/infraenv_types.goconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/resources.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamlinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/common/nmstatectl_service.gointernal/common/nmstatectl_service_test.gointernal/constants/nmstatectl_script.gointernal/controller/controllers/infraenv_controller.gointernal/isoeditor/rhcos_test.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gorestapi/embedded_spec.goswagger.yaml
Comment on lines
+11798
to
+11813
| gzipReader, err := gzip.NewReader(responsePayload) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| r := cpio.NewReader(gzipReader) | ||
| for { | ||
| hdr, err := r.Next() | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| if hdr.Name == "/etc/systemd/system/pre-network-manager-config.service" { | ||
| serviceBytes, err := io.ReadAll(r) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(string(serviceBytes)).To(ContainSubstring("Environment=DISCOVERY_DELAY_SECONDS=8")) | ||
| Expect(string(serviceBytes)).To(ContainSubstring("TimeoutSec=68")) | ||
| return |
There was a problem hiding this comment.
Close the gzipReader to avoid resource leak.
The gzipReader is created at line 11798 but never closed. Both the success path (early return at line 11813) and the failure path (Fail at line 11816) leave the reader open. While the impact is minimal in test code, closing the reader is good practice to release internal buffers and finalize decompression state.
🔧 Proposed fix
gzipReader, err := gzip.NewReader(responsePayload)
Expect(err).ToNot(HaveOccurred())
+defer gzipReader.Close()
r := cpio.NewReader(gzipReader)📝 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.
Suggested change
| gzipReader, err := gzip.NewReader(responsePayload) | |
| Expect(err).ToNot(HaveOccurred()) | |
| r := cpio.NewReader(gzipReader) | |
| for { | |
| hdr, err := r.Next() | |
| if err == io.EOF { | |
| break | |
| } | |
| Expect(err).ToNot(HaveOccurred()) | |
| if hdr.Name == "/etc/systemd/system/pre-network-manager-config.service" { | |
| serviceBytes, err := io.ReadAll(r) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(string(serviceBytes)).To(ContainSubstring("Environment=DISCOVERY_DELAY_SECONDS=8")) | |
| Expect(string(serviceBytes)).To(ContainSubstring("TimeoutSec=68")) | |
| return | |
| gzipReader, err := gzip.NewReader(responsePayload) | |
| Expect(err).ToNot(HaveOccurred()) | |
| defer gzipReader.Close() | |
| r := cpio.NewReader(gzipReader) | |
| for { | |
| hdr, err := r.Next() | |
| if err == io.EOF { | |
| break | |
| } | |
| Expect(err).ToNot(HaveOccurred()) | |
| if hdr.Name == "/etc/systemd/system/pre-network-manager-config.service" { | |
| serviceBytes, err := io.ReadAll(r) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(string(serviceBytes)).To(ContainSubstring("Environment=DISCOVERY_DELAY_SECONDS=8")) | |
| Expect(string(serviceBytes)).To(ContainSubstring("TimeoutSec=68")) | |
| return |
🤖 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/bminventory/inventory_test.go` around lines 11798 - 11813, The
gzip.Reader created as gzipReader in the test is never closed; after calling
gzip.NewReader(responsePayload) add an immediate defer gzipReader.Close() so the
reader is always closed on return or failure (or explicitly call
gzipReader.Close() before any early return/Fail if you prefer explicit close),
referencing the gzipReader variable created in this test function to ensure
resources are released.
Comment on lines
+5611
to
+5615
| if params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds != nil { | ||
| if infraEnv.NetworkDiscoveryDelaySeconds == nil || *params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds != *infraEnv.NetworkDiscoveryDelaySeconds { | ||
| updates["network_discovery_delay_seconds"] = *params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds | ||
| } | ||
| } |
There was a problem hiding this comment.
Allow clearing the per-infra-env override.
Line 5611 only handles non-nil updates. After a delay has been persisted, removing the field from the CR/API request leaves the old DB value in place, so minimal ISO generation keeps using the stale override instead of falling back to NETWORK_DISCOVERY_DELAY_SECONDS.
🤖 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/bminventory/inventory.go` around lines 5611 - 5615, The update logic
currently only applies non-nil NetworkDiscoveryDelaySeconds, so clearing the
override via a nil in params leaves the old value in the DB; modify the branch
handling params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds to also detect
when it is nil while infraEnv.NetworkDiscoveryDelaySeconds is non-nil and add
updates["network_discovery_delay_seconds"] = nil (or the DB-null equivalent used
in this repo) so the stored override is cleared and the system falls back to
NETWORK_DISCOVERY_DELAY_SECONDS.
Comment on lines
+242
to
+247
| if infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil { | ||
| if internalInfraEnv.NetworkDiscoveryDelaySeconds == nil || | ||
| *infraEnv.Spec.NetworkDiscoveryDelaySeconds != *internalInfraEnv.NetworkDiscoveryDelaySeconds { | ||
| updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the case when user clears NetworkDiscoveryDelaySeconds back to nil.
The current logic only sends an update when infraEnv.Spec.NetworkDiscoveryDelaySeconds is non-nil. If a user previously set a delay (e.g., 10 seconds) and later removes the field (sets it back to nil in the spec), no update is sent to the backend. The backend will continue using the old value instead of falling back to the default.
Since the PR objective states that "if an infra-env delay is not set, the service will use a default delay," nil is a meaningful state. Users should be able to clear the field.
Proposed fix to handle nil-to-nil and value-to-nil transitions
- if infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil {
- if internalInfraEnv.NetworkDiscoveryDelaySeconds == nil ||
- *infraEnv.Spec.NetworkDiscoveryDelaySeconds != *internalInfraEnv.NetworkDiscoveryDelaySeconds {
- updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds
- }
- }
+ // Always update if values differ (including nil transitions)
+ if (infraEnv.Spec.NetworkDiscoveryDelaySeconds == nil && internalInfraEnv.NetworkDiscoveryDelaySeconds != nil) ||
+ (infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil &&
+ (internalInfraEnv.NetworkDiscoveryDelaySeconds == nil ||
+ *infraEnv.Spec.NetworkDiscoveryDelaySeconds != *internalInfraEnv.NetworkDiscoveryDelaySeconds)) {
+ updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds
+ }📝 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.
Suggested change
| if infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil { | |
| if internalInfraEnv.NetworkDiscoveryDelaySeconds == nil || | |
| *infraEnv.Spec.NetworkDiscoveryDelaySeconds != *internalInfraEnv.NetworkDiscoveryDelaySeconds { | |
| updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds | |
| } | |
| } | |
| // Always update if values differ (including nil transitions) | |
| if (infraEnv.Spec.NetworkDiscoveryDelaySeconds == nil && internalInfraEnv.NetworkDiscoveryDelaySeconds != nil) || | |
| (infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil && | |
| (internalInfraEnv.NetworkDiscoveryDelaySeconds == nil || | |
| *infraEnv.Spec.NetworkDiscoveryDelaySeconds != *internalInfraEnv.NetworkDiscoveryDelaySeconds)) { | |
| updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds | |
| } |
🤖 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/controller/controllers/infraenv_controller.go` around lines 242 -
247, The reconciliation currently only sets
updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds when
infraEnv.Spec.NetworkDiscoveryDelaySeconds is non-nil, so clearing the field
(nil) never sends an update; modify the logic around
infraEnv.Spec.NetworkDiscoveryDelaySeconds /
internalInfraEnv.NetworkDiscoveryDelaySeconds to also detect when the spec is
nil but internalInfraEnv.NetworkDiscoveryDelaySeconds is non-nil and in that
case set updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = nil
(i.e., explicitly request clearing the backend value); keep the existing branch
that updates when values differ and do nothing only when both are nil or equal.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10373 +/- ##
==========================================
- Coverage 44.32% 44.31% -0.02%
==========================================
Files 417 418 +1
Lines 72766 72830 +64
==========================================
+ Hits 32256 32271 +15
- Misses 37591 37637 +46
- Partials 2919 2922 +3
🚀 New features to boost your workflow:
|
…g on minimal ISO A delay can be used on hosts that need time to discover their NICs. The delay can be set per infra-env, to specify how many second to wait before mapping host MACs to interfaces. If it's not set then the delay will be the value of an environment variable with the default value being 0. MCE users can set that environment variable to configure a default delay.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/controllers/infraenv_controller.go (1)
451-483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd NetworkDiscoveryDelaySeconds to create parameters.
The
CreateInfraEnvParamsfunction does not includeNetworkDiscoveryDelaySecondsfrom the InfraEnv spec. When a user creates an InfraEnv withNetworkDiscoveryDelaySecondsset, the field is omitted from the initial registration and only applied on the next reconcile viaupdateInfraEnv.Proposed fix to include NetworkDiscoveryDelaySeconds in create params
if len(infraEnv.Spec.KernelArguments) > 0 { createParams.InfraenvCreateParams.KernelArguments = internalKernelArgs(infraEnv.Spec.KernelArguments) } + + if infraEnv.Spec.NetworkDiscoveryDelaySeconds != nil { + createParams.InfraenvCreateParams.NetworkDiscoveryDelaySeconds = infraEnv.Spec.NetworkDiscoveryDelaySeconds + } return createParams🤖 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/controller/controllers/infraenv_controller.go` around lines 451 - 483, CreateInfraEnvParams currently omits InfraEnv.Spec.NetworkDiscoveryDelaySeconds from the initial installer.RegisterInfraEnvParams/InfraenvCreateParams payload, causing the value to only be applied later in updateInfraEnv; modify CreateInfraEnvParams so that createParams.InfraenvCreateParams.NetworkDiscoveryDelaySeconds is set from infraEnv.Spec.NetworkDiscoveryDelaySeconds (ensure proper pointer/nullable conversion to the model type expected by InfraenvCreateParams, e.g., using the same helper pattern as other optional fields or swag.Int32/similar) so the field is included in the initial registration.
♻️ Duplicate comments (3)
restapi/embedded_spec.go (1)
9277-9282:⚠️ Potential issue | 🟡 MinorEmbedded schemas still missing
minimum: 0constraint.The embedded schema definitions for
network_discovery_delay_seconds(in hunks 1-3) continue to omit the"minimum": 0constraint that appears in the corresponding flattened schemas (hunks 4-6). This inconsistency was previously flagged and remains unresolved. Sinceswagger.yamldefinesminimum: 0, the generation process should propagate this constraint to both embedded and flattened outputs.Also applies to: 9388-9393, 9454-9459
🤖 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 `@restapi/embedded_spec.go` around lines 9277 - 9282, The embedded schema entries for the property "network_discovery_delay_seconds" are missing the "minimum": 0 constraint; update each embedded schema occurrence in embedded_spec.go (the JSON/object entries for "network_discovery_delay_seconds") to include "minimum": 0 alongside "type": "integer" and "format": "int64" (preserve "x-nullable": true), and apply the same change to the other two occurrences mentioned so the embedded output matches the flattened swagger.yaml constraint.internal/bminventory/inventory.go (1)
5611-5615:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow clearing
network_discovery_delay_secondsduring infra-env updates.Line 5611 only handles non-nil values. If an override already exists, clearing the field leaves stale DB state, so minimal initrd continues using the old override instead of falling back to
NETWORK_DISCOVERY_DELAY_SECONDS.Suggested fix
if params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds != nil { if infraEnv.NetworkDiscoveryDelaySeconds == nil || *params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds != *infraEnv.NetworkDiscoveryDelaySeconds { updates["network_discovery_delay_seconds"] = *params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds } + } else if infraEnv.NetworkDiscoveryDelaySeconds != nil { + updates["network_discovery_delay_seconds"] = nil }🤖 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/bminventory/inventory.go` around lines 5611 - 5615, The current update logic only applies a new non-nil NetworkDiscoveryDelaySeconds but never clears an existing override; modify the update handling for params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds so that if the incoming value is nil and infraEnv.NetworkDiscoveryDelaySeconds is non-nil you add an entry to updates["network_discovery_delay_seconds"] that clears the DB value (i.e., set updates to a nil/NULL-equivalent for that column) and otherwise preserve the existing comparison logic when a non-nil value is provided; reference params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds, infraEnv.NetworkDiscoveryDelaySeconds and updates["network_discovery_delay_seconds"] when making the change.internal/controller/controllers/infraenv_controller.go (1)
242-247:⚠️ Potential issue | 🟠 MajorPast review concern remains valid: clearing NetworkDiscoveryDelaySeconds requires explicit nil handling.
The current logic does not send an update when a user clears
NetworkDiscoveryDelaySeconds(sets it to nil in the spec). Once set, users cannot revert to the default delay without deleting and recreating the InfraEnv.🤖 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/controller/controllers/infraenv_controller.go` around lines 242 - 247, The update logic in infraenv_controller.go (around the block comparing infraEnv.Spec.NetworkDiscoveryDelaySeconds and internalInfraEnv.NetworkDiscoveryDelaySeconds) misses the case where the user clears the spec (sets NetworkDiscoveryDelaySeconds to nil); modify the comparison so that if infraEnv.Spec.NetworkDiscoveryDelaySeconds is nil and internalInfraEnv.NetworkDiscoveryDelaySeconds is non-nil you explicitly set updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = nil to clear the stored value, and otherwise keep the existing conditional that copies the new non-nil value from infraEnv.Spec.NetworkDiscoveryDelaySeconds when it differs.
🤖 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.
Outside diff comments:
In `@internal/controller/controllers/infraenv_controller.go`:
- Around line 451-483: CreateInfraEnvParams currently omits
InfraEnv.Spec.NetworkDiscoveryDelaySeconds from the initial
installer.RegisterInfraEnvParams/InfraenvCreateParams payload, causing the value
to only be applied later in updateInfraEnv; modify CreateInfraEnvParams so that
createParams.InfraenvCreateParams.NetworkDiscoveryDelaySeconds is set from
infraEnv.Spec.NetworkDiscoveryDelaySeconds (ensure proper pointer/nullable
conversion to the model type expected by InfraenvCreateParams, e.g., using the
same helper pattern as other optional fields or swag.Int32/similar) so the field
is included in the initial registration.
---
Duplicate comments:
In `@internal/bminventory/inventory.go`:
- Around line 5611-5615: The current update logic only applies a new non-nil
NetworkDiscoveryDelaySeconds but never clears an existing override; modify the
update handling for params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds so
that if the incoming value is nil and infraEnv.NetworkDiscoveryDelaySeconds is
non-nil you add an entry to updates["network_discovery_delay_seconds"] that
clears the DB value (i.e., set updates to a nil/NULL-equivalent for that column)
and otherwise preserve the existing comparison logic when a non-nil value is
provided; reference params.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds,
infraEnv.NetworkDiscoveryDelaySeconds and
updates["network_discovery_delay_seconds"] when making the change.
In `@internal/controller/controllers/infraenv_controller.go`:
- Around line 242-247: The update logic in infraenv_controller.go (around the
block comparing infraEnv.Spec.NetworkDiscoveryDelaySeconds and
internalInfraEnv.NetworkDiscoveryDelaySeconds) misses the case where the user
clears the spec (sets NetworkDiscoveryDelaySeconds to nil); modify the
comparison so that if infraEnv.Spec.NetworkDiscoveryDelaySeconds is nil and
internalInfraEnv.NetworkDiscoveryDelaySeconds is non-nil you explicitly set
updateParams.InfraEnvUpdateParams.NetworkDiscoveryDelaySeconds = nil to clear
the stored value, and otherwise keep the existing conditional that copies the
new non-nil value from infraEnv.Spec.NetworkDiscoveryDelaySeconds when it
differs.
In `@restapi/embedded_spec.go`:
- Around line 9277-9282: The embedded schema entries for the property
"network_discovery_delay_seconds" are missing the "minimum": 0 constraint;
update each embedded schema occurrence in embedded_spec.go (the JSON/object
entries for "network_discovery_delay_seconds") to include "minimum": 0 alongside
"type": "integer" and "format": "int64" (preserve "x-nullable": true), and apply
the same change to the other two occurrences mentioned so the embedded output
matches the flattened swagger.yaml constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4bd26a8b-7d19-480d-a8fc-4669790ed0e0
⛔ Files ignored due to path filters (12)
api/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (16)
api/v1beta1/infraenv_types.goconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/resources.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamlinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/common/nmstatectl_service.gointernal/common/nmstatectl_service_test.gointernal/constants/nmstatectl_script.gointernal/controller/controllers/infraenv_controller.gointernal/isoeditor/rhcos_test.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gorestapi/embedded_spec.goswagger.yaml
✅ Files skipped from review due to trivial changes (1)
- models/infra_env_update_params.go
|
@giladravid16: 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. |
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
2 participants
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.
A delay can be used on hosts that need time to discover their NICs.
The delay can be set per infra-env, to specify how many second to wait before mapping host MACs to interfaces. If it's not set then the delay will be the value of an environment variable with the default value being 0. MCE users can set that environment variable to configure a default delay.
I tested manually that:
List all the issues related to this PR
Closes MGMT-24243
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit