MGMT-19732: Add ntp_sources field for exclusive NTP configuration#10408
Merged
openshift-merge-bot[bot] merged 3 commits intoJun 2, 2026
Merged
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@yoavsc0302: This pull request references MGMT-19732 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
size/L
|
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 ignored due to path filters (18)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughAdds an exclusive ChangesNTP Sources Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
api-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.go`:
- Around line 441-443: The current nil-only check for
params.NewClusterParams.AdditionalNtpSource and
params.NewClusterParams.NtpSources can treat an explicit empty string as a
supplied value and skip the default; change the logic so empty-string values are
treated as unset: when deciding to assign b.Config.DefaultNTPSource, treat
AdditionalNtpSource as unset if it is nil or
strings.TrimSpace(*params.NewClusterParams.AdditionalNtpSource) == "" and treat
NtpSources as unset if it is nil or has no non-empty entries (e.g., len==0 or
all entries are empty/whitespace); update both the block around
params.NewClusterParams.AdditionalNtpSource (current check at lines ~441) and
the similar logic around lines ~5318-5331 to use these checks so the default is
only suppressed when the caller truly provides replacement sources.
In `@internal/ignition/discovery.go`:
- Around line 293-300: The split of ntp sources (ntpSourcesRaw →
desiredNtpSources) can include whitespace-only or empty tokens which produce
invalid chrony `server` lines; update the logic around
infraEnv.AdditionalNtpSources / infraEnv.NtpSources and the desiredNtpSources
variable to normalize entries by trimming whitespace (strings.TrimSpace) and
filtering out empty strings after splitting, producing a clean []string for
templating so no blank/whitespace-only servers are emitted.
In `@swagger.yaml`:
- Around line 5239-5242: Update the ntp_sources property description to
explicitly state it is mutually exclusive with the additional_ntp_source and
additional_ntp_sources fields (the server rejects payloads that set both);
similarly update the descriptions for all other ntp_sources occurrences to
include the same mutual-exclusivity note so generated docs don’t present them as
independently valid.
🪄 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: f4725766-e154-432c-aabc-6211b41f0e63
⛔ Files ignored due to path filters (18)
api/vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!**/vendor/**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/**api/vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/cluster_create_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/**client/vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!**/vendor/**,!vendor/**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/**vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (16)
internal/bminventory/inventory.gointernal/common/common.gointernal/ignition/disconnected.gointernal/ignition/discovery.gointernal/ignition/templates/add-ntp-sources.shinternal/ignition/templates/discovery.igninternal/network/manifests_generator.gointernal/network/manifests_generator_test.gomodels/cluster.gomodels/cluster_create_params.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gomodels/v2_cluster_update_params.gorestapi/embedded_spec.goswagger.yaml
Comment on lines
+293
to
+300
| // When ntp_sources is set, use it for discovery so the user's servers are available from the start. | ||
| ntpSourcesRaw := infraEnv.AdditionalNtpSources | ||
| if infraEnv.NtpSources != "" { | ||
| ntpSourcesRaw = infraEnv.NtpSources | ||
| } | ||
| desiredNtpSources := strings.Split(ntpSourcesRaw, ",") | ||
| if len(desiredNtpSources) == 1 && desiredNtpSources[0] == "" { | ||
| desiredNtpSources = []string{} |
There was a problem hiding this comment.
Filter and trim NTP entries before templating.
Line 298 currently preserves whitespace/empty CSV tokens, which can render invalid server lines downstream (e.g., server iburst). Normalize entries here to keep generated chrony config valid.
Suggested fix
- desiredNtpSources := strings.Split(ntpSourcesRaw, ",")
- if len(desiredNtpSources) == 1 && desiredNtpSources[0] == "" {
- desiredNtpSources = []string{}
- }
+ var desiredNtpSources []string
+ for _, source := range strings.Split(ntpSourcesRaw, ",") {
+ source = strings.TrimSpace(source)
+ if source != "" {
+ desiredNtpSources = append(desiredNtpSources, source)
+ }
+ }📝 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
| // When ntp_sources is set, use it for discovery so the user's servers are available from the start. | |
| ntpSourcesRaw := infraEnv.AdditionalNtpSources | |
| if infraEnv.NtpSources != "" { | |
| ntpSourcesRaw = infraEnv.NtpSources | |
| } | |
| desiredNtpSources := strings.Split(ntpSourcesRaw, ",") | |
| if len(desiredNtpSources) == 1 && desiredNtpSources[0] == "" { | |
| desiredNtpSources = []string{} | |
| // When ntp_sources is set, use it for discovery so the user's servers are available from the start. | |
| ntpSourcesRaw := infraEnv.AdditionalNtpSources | |
| if infraEnv.NtpSources != "" { | |
| ntpSourcesRaw = infraEnv.NtpSources | |
| } | |
| var desiredNtpSources []string | |
| for _, source := range strings.Split(ntpSourcesRaw, ",") { | |
| source = strings.TrimSpace(source) | |
| if source != "" { | |
| desiredNtpSources = append(desiredNtpSources, source) | |
| } | |
| } |
🤖 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/ignition/discovery.go` around lines 293 - 300, The split of ntp
sources (ntpSourcesRaw → desiredNtpSources) can include whitespace-only or empty
tokens which produce invalid chrony `server` lines; update the logic around
infraEnv.AdditionalNtpSources / infraEnv.NtpSources and the desiredNtpSources
variable to normalize entries by trimming whitespace (strings.TrimSpace) and
filtering out empty strings after splitting, producing a clean []string for
templating so no blank/whitespace-only servers are emitted.
Comment on lines
+5239
to
+5242
| ntp_sources: | ||
| type: string | ||
| description: A comma-separated list of NTP sources (name or IP) to be used as the only NTP configuration for the cluster hosts. | ||
| x-nullable: true |
There was a problem hiding this comment.
Document the mutual-exclusivity contract here.
The server rejects payloads that set ntp_sources together with additional_ntp_source(s), but these descriptions do not say that. Generated API docs will otherwise present both fields as independently valid.
Suggested wording
ntp_sources:
type: string
- description: A comma-separated list of NTP sources (name or IP) to be used as the only NTP configuration for the cluster hosts.
+ description: A comma-separated list of NTP sources (name or IP) to be used as the only NTP configuration for the cluster hosts. Mutually exclusive with `additional_ntp_source`.
x-nullable: true ntp_sources:
type: string
- description: A comma-separated list of NTP sources (name or IP) to be used as the only NTP configuration for hosts in this infra-env.
+ description: A comma-separated list of NTP sources (name or IP) to be used as the only NTP configuration for hosts in this infra-env. Mutually exclusive with `additional_ntp_sources`.
x-nullable: trueAlso applies to: 5466-5469, 5840-5842, 7808-7810, 7933-7936, 7996-7999
🤖 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 `@swagger.yaml` around lines 5239 - 5242, Update the ntp_sources property
description to explicitly state it is mutually exclusive with the
additional_ntp_source and additional_ntp_sources fields (the server rejects
payloads that set both); similarly update the descriptions for all other
ntp_sources occurrences to include the same mutual-exclusivity note so generated
docs don’t present them as independently valid.
yoavsc0302
force-pushed
the
MGMT-19732/ntp-sources-replace-option-v3
branch
from
caab8e2 to
17455e5
Compare
jhernand
approved these changes
May 28, 2026
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10408 +/- ##
==========================================
+ Coverage 44.31% 44.35% +0.04%
==========================================
Files 418 418
Lines 72844 73051 +207
==========================================
+ Hits 32280 32403 +123
- Misses 37639 37713 +74
- Partials 2925 2935 +10
🚀 New features to boost your workflow:
|
Contributor
openshift-ci
Bot
added
the
needs-rebase
Add ntp_sources string field to cluster, cluster-create-params, v2-cluster-update-params, infra-env, infra-env-create-params, and infra-env-update-params models.
Auto-generated by skipper make generate.
Add a new ntp_sources field to cluster and infra-env models that allows users to specify the complete NTP configuration. When set, only the user-specified sources are included — the default NTP pool is not added. This field is mutually exclusive with additional_ntp_source — the API rejects requests that set both, checking the resulting DB state on updates to prevent conflicts. - Discovery: when ntp_sources is set, add-ntp-sources.sh overwrites the ISO chrony.conf with only the user's servers instead of appending to the default configuration - Installation: the default NTP pool is excluded from the generated chrony MachineConfig, while agent-discovered sources from the user's specified servers are preserved - NTP resolution: GetClusterNTPSources() and GetHostNTPSources() return ntp_sources when set, preserving backward compatibility with additional_ntp_source when ntp_sources is not set
yoavsc0302
force-pushed
the
MGMT-19732/ntp-sources-replace-option-v3
branch
from
17455e5 to
f090822
Compare
openshift-ci
Bot
removed
lgtm
Contributor
Author
|
/retest |
3 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
jhernand
approved these changes
Jun 2, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, yoavsc0302 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 |
|
@yoavsc0302: 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
3 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.
Add a new ntp_sources field to cluster and infra-env models that allows users to specify the complete NTP configuration. When set, only the user-specified sources are included — the default NTP pool is not added.
This field is mutually exclusive with additional_ntp_source — the API rejects requests that set both, checking the resulting DB state on updates to prevent conflicts.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
New Features
Improvements
Tests