[MGMT-21038] Add hostName to host - used to manage inventory hostname.#10093
Open
bkopilov wants to merge 1 commit intoopenshift:masterfrom
Open
[MGMT-21038] Add hostName to host - used to manage inventory hostname.#10093bkopilov wants to merge 1 commit intoopenshift:masterfrom
bkopilov wants to merge 1 commit intoopenshift:masterfrom
Conversation
Current code in host model set RequestedHostname when learning hostname from UI or from inventoy hostname. Create additional hostname param to keep inventory hostname. User can change the requestedHostname while inventory hostname may be different.
openshift-ci
bot
added
the
size/S
WalkthroughThis PR introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bkopilov 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 |
openshift-ci
bot
added
the
api-review
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @bkopilov. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/bminventory/inventory.go`:
- Around line 4708-4711: The current logic sets host.RequestedHostname
unconditionally when host.Hostname is empty, which overwrites any user-provided
requested hostname; change it so host.Hostname is set to host.ID.String() if
empty, but only set host.RequestedHostname to host.ID.String() if
host.RequestedHostname is also empty—i.e., update the block around host.Hostname
and host.RequestedHostname so host.RequestedHostname is preserved when already
set (referencing host.Hostname and host.RequestedHostname).
In `@internal/host/host.go`:
- Around line 410-417: The current logic assigns hostName = h.ID.String()
whenever inventory.Hostname is empty, which allows a transient empty
inventory.Hostname to overwrite a previously persisted hostname; change the
logic in the block that sets hostName and h.RequestedHostname so that you only
fall back to h.ID.String() if both inventory.Hostname and h.RequestedHostname
are empty (i.e., preserve any existing h.RequestedHostname/persisted hostname),
and ensure the persistence code that writes h.RequestedHostname does not save
the h.ID.String() fallback when a persisted hostname already exists.
In `@internal/host/hostutil/host_utils.go`:
- Around line 51-55: The code currently returns an error when host.Hostname is
empty; update the hostname resolution logic to try host.RequestedHostname first,
then host.Hostname, and as a final defensive fallback attempt to extract a
hostname from host.Inventory (the unpacked inventory JSON) before returning an
error; locate the hostname check around the Hostname empty-return block and add
logic to unmarshal host.Inventory (or use existing inventory parsing helpers) to
read a hostname field and return it if present, otherwise keep returning the
error.
In `@restapi/embedded_spec.go`:
- Around line 8477-8479: The swagger.yaml base spec currently defines the
hostname property with only "type: string"; update the hostname schema to add a
clear description and validation constraints (for example add description
explaining it's the server host name, a maxLength like 255, a suitable
pattern/regex to allow valid hostnames and disallow dangerous characters, and
optional minLength) so the generated embedded_spec.go reflects documentation and
enforces input validation for the hostname field.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 349ee228-1ef8-405c-a978-e87813c5b50f
⛔ Files ignored due to path filters (3)
api/vendor/github.com/openshift/assisted-service/models/host.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/host.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/models/host.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
internal/bminventory/inventory.gointernal/host/host.gointernal/host/hostutil/host_utils.gomodels/host.gorestapi/embedded_spec.goswagger.yaml
Comment on lines
+4708
to
+4711
| if host.Hostname == "" { | ||
| host.Hostname = host.ID.String() | ||
| host.RequestedHostname = host.ID.String() | ||
| } |
There was a problem hiding this comment.
Preserve user-set RequestedHostname when inventory hostname is missing
On Line 4710, host.RequestedHostname is overwritten whenever host.Hostname is empty. That can clobber a user-updated requested hostname before inventory arrives, which conflicts with the hostname-separation goal.
Proposed fix
- if host.Hostname == "" {
+ if host.Hostname == "" && host.RequestedHostname == "" {
host.Hostname = host.ID.String()
host.RequestedHostname = host.ID.String()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/bminventory/inventory.go` around lines 4708 - 4711, The current
logic sets host.RequestedHostname unconditionally when host.Hostname is empty,
which overwrites any user-provided requested hostname; change it so
host.Hostname is set to host.ID.String() if empty, but only set
host.RequestedHostname to host.ID.String() if host.RequestedHostname is also
empty—i.e., update the block around host.Hostname and host.RequestedHostname so
host.RequestedHostname is preserved when already set (referencing host.Hostname
and host.RequestedHostname).
Comment on lines
+410
to
+417
| var hostName string = inventory.Hostname | ||
| //in case RequestedHostname already set from UpdateHostname, can not change it from inventory | ||
| if hostName == "" { | ||
| hostName = h.ID.String() | ||
| } | ||
| if h.RequestedHostname == "" { | ||
| h.RequestedHostname = hostName | ||
| } |
There was a problem hiding this comment.
Do not overwrite persisted inventory hostname with host ID when inventory hostname is empty.
On Line 412-Line 414, the fallback goes straight to h.ID.String(). With Line 425 persisting that value, a transient empty inventory.Hostname can erase a previously learned inventory hostname, which conflicts with the preservation goal of this PR.
Proposed fix
- var hostName string = inventory.Hostname
+ var hostName string = inventory.Hostname
//in case RequestedHostname already set from UpdateHostname, can not change it from inventory
if hostName == "" {
- hostName = h.ID.String()
+ hostName = h.Hostname
+ }
+ if hostName == "" {
+ hostName = h.ID.String()
}
if h.RequestedHostname == "" {
h.RequestedHostname = hostName
}Also applies to: 425-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/host/host.go` around lines 410 - 417, The current logic assigns
hostName = h.ID.String() whenever inventory.Hostname is empty, which allows a
transient empty inventory.Hostname to overwrite a previously persisted hostname;
change the logic in the block that sets hostName and h.RequestedHostname so that
you only fall back to h.ID.String() if both inventory.Hostname and
h.RequestedHostname are empty (i.e., preserve any existing
h.RequestedHostname/persisted hostname), and ensure the persistence code that
writes h.RequestedHostname does not save the h.ID.String() fallback when a
persisted hostname already exists.
Comment on lines
+51
to
+55
| // in case inventory Hostname was not set. | ||
| if host.Hostname == "" { | ||
| return "", errors.Errorf("hostname is empty") | ||
| } | ||
| return inventory.Hostname, nil | ||
| return host.Hostname, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for host hostname schema migration/backfill =="
fd -i '*.sql' | xargs -r rg -n -i 'alter\s+table.*host|add\s+column.*hostname|update.*host.*set.*hostname'
rg -n --iglob '*.go' 'AutoMigrate|Migrate|AddColumn|hostname'
echo
echo "== Check Hostname assignment paths =="
rg -n --iglob '*.go' '\.Hostname\s*=|Hostname\s*:'
rg -n --iglob '*.go' '\.RequestedHostname\s*=|RequestedHostname\s*:'
echo
echo "== Audit GetCurrentHostName call sites for hard-fail behavior =="
rg -n --iglob '*.go' '\bGetCurrentHostName\s*\(' -C 3Repository: openshift/assisted-service
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for SQL migration files and hostname-related changes =="
find . -name "*.sql" -type f | head -20
echo
echo "== Search for migrations in Go code =="
rg -n 'AutoMigrate|Migrate.*Host|AddColumn.*hostname' --type go | head -30
echo
echo "== Search for Hostname field assignments =="
rg -n '\.Hostname\s*=' --type go -B 2 -A 2 | head -50
echo
echo "== Search for RequestedHostname assignments =="
rg -n '\.RequestedHostname\s*=' --type go -B 2 -A 2 | head -50
echo
echo "== Find all GetCurrentHostName call sites =="
rg -n 'GetCurrentHostName\s*\(' --type go -B 2 -A 3Repository: openshift/assisted-service
Length of output: 12692
🏁 Script executed:
#!/bin/bash
echo "== Host model definition =="
rg -n 'type Host struct' --type go -A 50 | head -80
echo
echo "== Context around inventory.go backfill (lines 4700-4720) =="
sed -n '4700,4720p' internal/bminventory/inventory.go
echo
echo "== Full GetCurrentHostName function =="
sed -n '47,56p' internal/host/hostutil/host_utils.go
echo
echo "== GetHostnameForMsg function (shows the fallback) =="
sed -n '58,65p' internal/host/hostutil/host_utils.go
echo
echo "== When/where is the inventory.go code (4708-4711) called? =="
rg -n 'if host.Hostname == ""' --type go -B 5 -A 5Repository: openshift/assisted-service
Length of output: 9656
🏁 Script executed:
#!/bin/bash
echo "== Check Inventory field type and usage =="
rg -n 'Inventory.*string' --type go -B 2 -A 2 | grep -A 3 'type Host'
echo
echo "== Search for UnmarshalInventory (mentioned in suggested patch) =="
rg -n 'UnmarshalInventory' --type go -B 2 -A 10 | head -50
echo
echo "== Check for any hostname extraction from Inventory JSON =="
rg -n 'inventory.*[Hh]ostname|[Hh]ostname.*inventory' --type go -i -B 3 -A 3 | head -80
echo
echo "== Check when customizeHost is called in the flow =="
rg -n 'customizeHost' --type go -B 2 -A 2Repository: openshift/assisted-service
Length of output: 17890
🏁 Script executed:
#!/bin/bash
echo "== Check when ignition generation happens and flow to GetCurrentHostName =="
rg -n 'GenerateIgnitionManifest|generateIgnitionManifest' --type go -B 5 -A 5 | head -100
echo
echo "== Check where GetCurrentHostName is called - full flow context =="
rg -n 'GetCurrentHostName' --type go -B 10 -A 5 | head -150
echo
echo "== Check if Hostname field is set in any pre-processing before ignition =="
rg -n 'host.Hostname\s*=' --type go -B 3 -A 1 | grep -v vendor | head -60Repository: openshift/assisted-service
Length of output: 5236
🏁 Script executed:
#!/bin/bash
echo "== Check git history for GetCurrentHostName changes =="
git log --oneline -20 -- internal/host/hostutil/host_utils.go 2>/dev/null | head -10
echo
echo "== Check git diff to see what was changed =="
git show HEAD:internal/host/hostutil/host_utils.go 2>/dev/null | sed -n '47,56p'
echo
echo "== Check if any test covers the error case =="
rg -n 'GetCurrentHostName' --type go test | head -30
echo
echo "== Check updateInventory in host.go (mentioned in comment) =="
rg -n 'updateInventory' --type go -B 3 -A 10 | grep -A 8 'func.*updateInventory'Repository: openshift/assisted-service
Length of output: 3187
🏁 Script executed:
#!/bin/bash
echo "== Look at the full git commit that made this change =="
git show faaaf6ca --stat 2>/dev/null | head -30
echo
echo "== Check where ignition gets host object from =="
rg -n 'writeSingleHostFile|SetHostnameForNodeIgnition' --type go -B 15 | grep -A 15 'func.*' | head -100
echo
echo "== Check if host is fetched from DB before ignition generation =="
rg -n 'GetHost|db.First|db.Where.*Host' --type go internal/ignition/ -A 3 | head -80
echo
echo "== Verify what GetHostByID or similar returns =="
rg -n 'func.*GetHost' --type go internal/host/ -A 15 | head -100Repository: openshift/assisted-service
Length of output: 16340
🏁 Script executed:
#!/bin/bash
echo "== Check who calls GenerateIgnitionManifests/GenerateInstallManifests =="
rg -n 'GenerateInstallManifests|GenerateIgnitionManifests' --type go -B 5 | grep -v test | head -100
echo
echo "== Check if hosts are loaded fresh from DB before ignition generation =="
rg -n 'GetCluster.*Hosts|GetClusterHosts|cluster.Hosts' --type go internal/ignition/ -B 3 -A 3 | head -80
echo
echo "== Look at the actual test to see host setup =="
sed -n '1067,1085p' internal/ignition/installmanifests_test.goRepository: openshift/assisted-service
Length of output: 7839
🏁 Script executed:
#!/bin/bash
echo "== Check Host model struct for Hostname field definition =="
rg -n 'type Host struct' --type go -A 100 vendor/github.com/openshift/assisted-service/models/host.go | grep -i 'hostname\|inventory' | head -20
echo
echo "== Check if Hostname has a default value or is explicitly backfilled =="
rg -n 'host.Hostname' --type go internal/ | grep -v test | grep -v vendor | head -40
echo
echo "== Check when ignition generation is triggered and from where =="
rg -n 'NewGenerator|GenerateInstallManifests' --type go -B 3 -A 3 | grep -v test | head -80
echo
echo "== Check if there's any code that explicitly sets Hostname before ignition =="
rg -n 'host.Hostname\s*=' --type go internal/ | grep -v test | grep -v vendorRepository: openshift/assisted-service
Length of output: 2850
🏁 Script executed:
#!/bin/bash
echo "== Check where cluster is loaded before passing to generator =="
rg -n 'NewGenerator\(' --type go -B 20 | head -150
echo
echo "== Check if cluster hosts are guaranteed to go through customizeHost =="
rg -n 'GetCluster\|GetClusterByID' --type go internal/ -A 10 | grep -v test | grep -v vendor | head -100
echo
echo "== Check the generator.Generate method to see if it loads fresh cluster =="
sed -n '190,250p' internal/ignition/installmanifests.goRepository: openshift/assisted-service
Length of output: 14931
🏁 Script executed:
#!/bin/bash
echo "== Check who calls the generator from pkg/generator/generator.go =="
rg -n 'Generate\(' --type go pkg/generator/ -B 5 -A 3 | head -80
echo
echo "== Check the flow in main or cmd that calls this =="
rg -n 'generator.Generate\|GenerateIgnitionManifests' --type go cmd/ -B 5 -A 3 | head -100
echo
echo "== Check if there's any GetCluster call that goes through customizeHost before generator =="
rg -n 'GetCluster|cluster :=' --type go pkg/generator/ -B 3 -A 3Repository: openshift/assisted-service
Length of output: 1059
🏁 Script executed:
#!/bin/bash
echo "== Search for the caller of pkg/generator/generator.go Generate =="
rg -n 'generator.GenerateIgnitionManifests\|GenerateAssets' --type go -B 10 | head -150
echo
echo "== Check if hosts are loaded fresh from DB in the ignition generation flow =="
rg -n 'GetClusterHosts\|cluster.Hosts\s*=' --type go pkg/ internal/ | grep -v test | grep -v vendor | head -60
echo
echo "== Look for the entry point that triggers ignition generation =="
rg -n 'func.*Generate.*ignition\|func.*GenerateAssets' --type go -B 5 | grep -v test | head -100Repository: openshift/assisted-service
Length of output: 281
🏁 Script executed:
#!/bin/bash
echo "== Check previous versions of GetCurrentHostName =="
git log --oneline -30 -- internal/host/hostutil/host_utils.go 2>/dev/null
echo
echo "== Show the actual changes in the commit =="
git show faaaf6ca:internal/host/hostutil/host_utils.go 2>/dev/null | sed -n '40,70p'
echo
echo "== Check the parent commit version =="
git show faaaf6ca^:internal/host/hostutil/host_utils.go 2>/dev/null | sed -n '40,70p'
echo
echo "== Verify if UnmarshalInventory exists and can extract hostname =="
rg -n 'func UnmarshalInventory' --type go -A 20 internal/common/Repository: openshift/assisted-service
Length of output: 1772
🏁 Script executed:
#!/bin/bash
echo "== Check git log before this commit to find GetCurrentHostName =="
git log --all --oneline -- internal/host/hostutil/host_utils.go 2>/dev/null | tail -20
echo
echo "== Show file content at HEAD for context =="
git show HEAD:internal/host/hostutil/host_utils.go 2>/dev/null | sed -n '45,65p'
echo
echo "== Verify what callers expect - check error handling =="
rg -n 'GetCurrentHostName' --type go -B 5 -A 5 | grep -E '(wraps|wrap|Wrapf|errors.Wrap)'Repository: openshift/assisted-service
Length of output: 2417
🏁 Script executed:
#!/bin/bash
echo "== Check updateInventory in host.go (lines 305-400) =="
sed -n '305,400p' internal/host/host.go
echo
echo "== Look for where Hostname is set from inventory =="
rg -n 'inventory.Hostname' --type go -B 3 -A 3 internal/host/Repository: openshift/assisted-service
Length of output: 5765
🏁 Script executed:
#!/bin/bash
echo "== Check the Host model commit to see when Hostname was added =="
git log --all --oneline --grep="Hostname\|hostname" -- models/host.go 2>/dev/null | head -10
echo
echo "== Check if Hostname has a database migration =="
git show faaaf6ca -- internal/migrations/ 2>/dev/null | head -100
echo
echo "== Verify the exact change in the Host model =="
git show faaaf6ca -- vendor/github.com/openshift/assisted-service/models/host.go 2>/dev/null | grep -A 5 -B 5 'Hostname string'
echo
echo "== Check if the Hostname field is initialized anywhere by default =="
rg -n 'Hostname.*=.*""' --type go internal/ | grep -v test | grep -v vendorRepository: openshift/assisted-service
Length of output: 5594
🏁 Script executed:
#!/bin/bash
echo "== Check where generator is called from - trace back to cluster source =="
rg -n 'GenerateAssets\|Generate\(' --type go pkg/generator/ internal/ignition/ -B 20 | grep -E '(func|cluster|GetCluster|db.First)' | head -100
echo
echo "== Check if there's direct DB loading that bypasses customizeHost =="
rg -n 'db.Preload.*Host|db.First.*Cluster.*Host' --type go internal/ | grep -v test | grep -v vendor | head -20
echo
echo "== Verify ignition generation entry point gets cluster from API or DB =="
rg -n 'func.*Generate.*context\|func.*GenerateAssets' --type go cmd/ pkg/ -A 20 | head -80Repository: openshift/assisted-service
Length of output: 680
Add inventory hostname fallback for defensive robustness.
While host.Hostname is typically populated via customizeHost() in API response paths, adding a fallback to host.Inventory when both RequestedHostname and Hostname are empty would provide better defensiveness against edge cases. The current code returns an error if both fields are empty, but hosts in the Inventory field may still contain valid hostname data. Consider adding the suggested compatibility patch to extract hostname from the unpacked inventory as a final fallback before returning an error, ensuring no valid hostname data is discarded.
Suggested defensive fallback patch
func GetCurrentHostName(host *models.Host) (string, error) {
if host.RequestedHostname != "" {
return host.RequestedHostname, nil
}
- // in case inventory Hostname was not set.
- if host.Hostname == "" {
- return "", errors.Errorf("hostname is empty")
+ if host.Hostname != "" {
+ return host.Hostname, nil
}
- return host.Hostname, nil
+ // Fallback for hosts not yet backfilled with Hostname field.
+ inventory, err := common.UnmarshalInventory(host.Inventory)
+ if err == nil && inventory != nil && inventory.Hostname != "" {
+ return inventory.Hostname, nil
+ }
+ return "", errors.Errorf("hostname is empty")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/host/hostutil/host_utils.go` around lines 51 - 55, The code
currently returns an error when host.Hostname is empty; update the hostname
resolution logic to try host.RequestedHostname first, then host.Hostname, and as
a final defensive fallback attempt to extract a hostname from host.Inventory
(the unpacked inventory JSON) before returning an error; locate the hostname
check around the Hostname empty-return block and add logic to unmarshal
host.Inventory (or use existing inventory parsing helpers) to read a hostname
field and return it if present, otherwise keep returning the error.
Comment on lines
+8477
to
+8479
| "hostname": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check swagger.yaml for hostname field definition and file generation markers
# Check if swagger.yaml has the hostname field with description
echo "=== Checking swagger.yaml for hostname field ==="
rg -A 5 -B 2 '^\s+hostname:' --glob 'swagger.yaml' --glob 'swagger.yml' --glob '**/openapi.yaml'
# Check for generation markers/comments in embedded_spec.go
echo "=== Checking for auto-generation markers in embedded_spec.go ==="
head -20 restapi/embedded_spec.go | rg -i 'generated|auto|do not edit'
# Check if there's a Makefile or script that regenerates the spec
echo "=== Checking for spec generation commands ==="
rg -i 'swagger.*generate' --glob 'Makefile' --glob '*.mk' --glob '*.sh'Repository: openshift/assisted-service
Length of output: 1724
Update the base swagger.yaml to include description and validation for the hostname field.
The embedded_spec.go file is correctly auto-generated from swagger.yaml. However, the base spec defines hostname with only a type and no description or validation constraints. Update swagger.yaml to include:
- A clear description of the field's purpose
- Validation constraints (e.g.,
maxLength,pattern) to prevent injection issues and ensure data quality
Snippet from swagger.yaml
hostname:
type: string
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@restapi/embedded_spec.go` around lines 8477 - 8479, The swagger.yaml base
spec currently defines the hostname property with only "type: string"; update
the hostname schema to add a clear description and validation constraints (for
example add description explaining it's the server host name, a maxLength like
255, a suitable pattern/regex to allow valid hostnames and disallow dangerous
characters, and optional minLength) so the generated embedded_spec.go reflects
documentation and enforces input validation for the hostname field.
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.
Current code in host model set RequestedHostname when learning hostname from UI or from inventoy hostname.
Create additional hostname param to keep inventory hostname. User can change the requestedHostname while inventory hostname may be different.
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