Skip to content

OCPBUGS-78420: preserve user-provided configs when multiple on-disk assets coexist #10390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pawanpinjarkar wants to merge 1 commit into
base: main
Choose a base branch
from
+197 −7
Open

OCPBUGS-78420: preserve user-provided configs when multiple on-disk assets coexist #10390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions pkg/asset/agent/image/unconfigured_ignition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/openshift/installer/pkg/asset/agent/common"
"github.com/openshift/installer/pkg/asset/agent/manifests"
"github.com/openshift/installer/pkg/asset/agent/workflow"
agenttypes "github.com/openshift/installer/pkg/types/agent"
)

func TestUnconfiguredIgnition_Generate(t *testing.T) {
Expand All @@ -24,6 +25,7 @@ func TestUnconfiguredIgnition_Generate(t *testing.T) {
defer setupEmbeddedResources(t)()

nmStateConfig := getTestNMStateConfig()
agentConfigWithRendezvousIPOnly := getTestAgentConfigWithRendezvousIPOnly()

cases := []struct {
name string
Expand Down Expand Up @@ -53,6 +55,19 @@ func TestUnconfiguredIgnition_Generate(t *testing.T) {
"oci-eval-user-data.service": true,
"agent-check-config-image.service": true},
},
{
name: "OVE case: agent-config with rendezvousIP only and nmstateconfig",
overrideDeps: []asset.Asset{
&nmStateConfig,
&agentConfigWithRendezvousIPOnly,
},
expectedFiles: generatedFilesUnconfiguredIgnition("/etc/assisted/network/host0/eth0.nmconnection",
"/etc/assisted/network/host0/mac_interface.ini", "/usr/local/bin/pre-network-manager-config.sh", "/usr/local/bin/oci-eval-user-data.sh"),
serviceEnabledMap: map[string]bool{
"pre-network-manager-config.service": true,
"oci-eval-user-data.service": true,
"agent-check-config-image.service": true},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -173,6 +188,22 @@ func getTestNMStateConfig() manifests.NMStateConfig {
}
}

func getTestAgentConfigWithRendezvousIPOnly() agentconfig.AgentConfig {
return agentconfig.AgentConfig{
Config: &agenttypes.Config{
RendezvousIP: "192.168.111.20",
// No hosts defined - this is the OVE case where agent-config
// only provides rendezvousIP for the SaaS UI, and network
// configuration comes from nmstateconfig.yaml
Hosts: []agenttypes.Host{},
},
File: &asset.File{
Filename: "agent-config.yaml",
Data: []byte("apiVersion: v1beta1\nkind: AgentConfig\nrendezvousIP: 192.168.111.20\n"),
},
}
}

func generatedFilesUnconfiguredIgnition(otherFiles ...string) []string {
unconfiguredIgnitionFiles := []string{
"/etc/assisted/manifests/pull-secret.yaml",
Expand Down
37 changes: 32 additions & 5 deletions pkg/asset/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ type assetState struct {
// presentOnDisk is true if the asset in on-disk. This is set whether the
// asset is sourced from on-disk or not. It is used in purging consumed assets.
presentOnDisk bool
// dirtyDueToUserConfigs is true if this asset is dirty only because of user-provided
// WritableAssets loaded from disk (not because of actual code changes or regeneration needs).
// This is used to preserve on-disk WritableAssets when multiple user configs coexist.
dirtyDueToUserConfigs bool
Copy link
Contributor

@andfasano andfasano Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store type is a very fundamental type for the whole installer and I don't think it's a good idea to introduce such low level change with potentially a huge impact area across the repo. It looks like more an issue on how we shaped the specific assets dependencies, so I'd prefer to review that first

/hold

Copy link
Contributor Author

@pawanpinjarkar pawanpinjarkar Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this is NOT the best way as it changes the core asset store functionality.

}

// storeImpl is the implementation of Store.
Expand Down Expand Up @@ -243,13 +247,22 @@ func (s *storeImpl) load(a asset.Asset, indent string) (*assetState, error) {

// Load dependencies from on-disk.
anyParentsDirty := false
dirtyDueToUserConfigs := true // Track if dirty only due to user-provided configs
for _, d := range a.Dependencies() {
state, err := s.load(d, increaseIndent(indent))
if err != nil {
return nil, err
}
if state.anyParentsDirty || state.source == onDiskSource {
anyParentsDirty = true
// Check if this dependency's dirtiness is due to user configs:
// - If it's a WritableAsset loaded from disk, it's a user config
// - If it's dirty only because of user configs (propagated), preserve that status
_, isWritable := d.(asset.WritableAsset)
isDueToUserConfig := (isWritable && state.source == onDiskSource) || state.dirtyDueToUserConfigs
if !isDueToUserConfig {
dirtyDueToUserConfigs = false
}
}
}

Expand All @@ -273,10 +286,13 @@ func (s *storeImpl) load(a asset.Asset, indent string) (*assetState, error) {
foundInStateFile bool
onDiskMatchesStateFile bool
)
// Check if asset is in state file (needed for preserving user-provided configs).
// Normally we skip this check if anyParentsDirty, but we need it for the special case below.
foundInStateFile = s.isAssetInState(a)

// Do not need to bother with loading from state file if any of the parents
// are dirty because the asset must be re-generated in this case.
if !anyParentsDirty {
foundInStateFile = s.isAssetInState(a)
if foundInStateFile {
stateFileAsset = reflect.New(reflect.TypeOf(a).Elem()).Interface().(asset.Asset)
if err := s.loadAssetFromState(stateFileAsset); err != nil {
Expand All @@ -301,6 +317,16 @@ func (s *storeImpl) load(a asset.Asset, indent string) (*assetState, error) {
source assetSource
)
switch {
// Special case: A parent is dirty, but the dirtiness is only due to user-provided configs from disk,
// AND this asset is also found on disk AND is not in the state file (newly user-provided).
// In this case, preserve the on-disk version instead of regenerating, since multiple user-provided
// configs should coexist (e.g., agent-config.yaml with rendezvousIP + nmstateconfig.yaml with
// network config in the OVE use case). If the asset IS in the state file, it was previously generated
// and should be regenerated when its inputs change.
case anyParentsDirty && dirtyDueToUserConfigs && foundOnDisk && !foundInStateFile:
logrus.Debugf("%sUsing %s loaded from target directory (preserving user-provided config despite on-disk dependencies)", indent, a.Name())
assetToStore = onDiskAsset
source = onDiskSource
Comment on lines +320 to +329
Copy link

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This preservation disappears after the first save/load cycle.

This branch treats !foundInStateFile as "user-provided", but saveStateFile() on Lines 172-180 still writes every onDiskSource asset into .openshift_install_state.json. After one successful run, the same on-disk config will now look state-file-backed on the next invocation, so it falls out of this branch and gets regenerated/discarded again even though it was never generated in the first place. You need to persist provenance/source, not infer it from state-file presence alone.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/asset/store/store.go` around lines 320 - 329, The current logic treats
!foundInStateFile as "user-provided" but saveStateFile() still writes every
onDiskSource asset into the state file, so after one run the provenance is lost;
fix by persisting explicit provenance/source in the stored state and restoring
it on load instead of inferring from presence. Specifically: extend the state
entry struct written by saveStateFile() to include a Source/Provenance enum/flag
(e.g., "user-provided" vs "generated") and set that when you populate entries
from onDiskSource/onDiskAsset; update the state load path to read that field and
set a new boolean (or use it to compute foundInStateFile/provenanceUserProvided)
used by the branch that checks anyParentsDirty, dirtyDueToUserConfigs,
foundOnDisk, and foundInStateFile so the branch uses the persisted provenance
value rather than mere presence; ensure saveStateFile still writes the
provenance for on-disk assets created by users.

// A parent is dirty. The asset must be re-generated.
case anyParentsDirty:
if foundOnDisk {
Expand All @@ -324,10 +350,11 @@ func (s *storeImpl) load(a asset.Asset, indent string) (*assetState, error) {
}

state := &assetState{
asset: assetToStore,
source: source,
anyParentsDirty: anyParentsDirty,
presentOnDisk: foundOnDisk,
asset: assetToStore,
source: source,
anyParentsDirty: anyParentsDirty,
presentOnDisk: foundOnDisk,
dirtyDueToUserConfigs: dirtyDueToUserConfigs && anyParentsDirty,
}
s.assets[reflect.TypeOf(a)] = state
return state, nil
Expand Down
136 changes: 134 additions & 2 deletions pkg/asset/store/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package store

import (
"context"
"encoding/json"
"os"
"path/filepath"
"reflect"
Expand Down Expand Up @@ -294,6 +295,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
name string
assets map[string][]string
onDiskAssets []string
stateFileAssets []string
target string
expectedGenerationLog []string
expectedDirty bool
Expand All @@ -305,6 +307,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
"b": {},
},
onDiskAssets: nil,
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{"b", "a"},
expectedDirty: false,
Expand All @@ -316,6 +319,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
"b": {},
},
onDiskAssets: []string{"a"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{},
expectedDirty: false,
Expand All @@ -327,6 +331,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
"b": {},
},
onDiskAssets: []string{"b"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{"a"},
expectedDirty: true,
Expand All @@ -340,17 +345,31 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
"d": {},
},
onDiskAssets: []string{"d"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{"b", "c", "a"},
expectedDirty: true,
},
{
name: "re-generate when both parents and children are on-disk",
name: "preserve both parent and child when both are user-provided on-disk configs",
assets: map[string][]string{
"a": {"b"},
"b": {},
},
onDiskAssets: []string{"a", "b"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{},
expectedDirty: true,
},
{
name: "re-generate when child was previously generated (in state file) and parent is on-disk",
assets: map[string][]string{
"a": {"b"},
"b": {},
},
onDiskAssets: []string{"a", "b"},
stateFileAssets: []string{"a"}, // a was previously generated
target: "a",
expectedGenerationLog: []string{"a"},
expectedDirty: true,
Expand All @@ -360,7 +379,8 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
clearAssetBehaviors()
store := &storeImpl{
assets: map[reflect.Type]*assetState{},
assets: map[reflect.Type]*assetState{},
stateFileAssets: map[string]json.RawMessage{},
}
assets := make(map[string]asset.Asset, len(tc.assets))
for name := range tc.assets {
Expand All @@ -376,6 +396,12 @@ func TestStoreFetchOnDiskAssets(t *testing.T) {
for _, name := range tc.onDiskAssets {
onDiskAssets[reflect.TypeOf(assets[name])] = true
}

// Simulate assets in state file
for _, name := range tc.stateFileAssets {
store.stateFileAssets[reflect.TypeOf(assets[name]).String()] = json.RawMessage("{}")
}

err := store.fetch(context.Background(), assets[tc.target], "")
assert.NoError(t, err, "unexpected error")
assert.EqualValues(t, tc.expectedGenerationLog, generationLog)
Expand Down Expand Up @@ -421,6 +447,112 @@ func TestStoreFetchIdempotency(t *testing.T) {
assert.Equal(t, expectedFiles, actualFiles, "unexpected files on disk")
}

func TestStoreFetchUserProvidedConfigs(t *testing.T) {
cases := []struct {
name string
assets map[string][]string
onDiskAssets []string
stateFileAssets []string
target string
expectedGenerationLog []string
expectedSource assetSource
description string
}{
{
name: "OVE case: both agent-config and nmstateconfig on disk, not in state file",
assets: map[string][]string{
"a": {"b"}, // a = nmstateconfig, b = agent-config
"b": {},
},
onDiskAssets: []string{"a", "b"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{},
expectedSource: onDiskSource,
description: "Both user configs should be preserved without regeneration",
},
{
name: "OVE case with intermediate asset: agent-config -> intermediate -> nmstateconfig",
assets: map[string][]string{
"a": {"c"}, // a = nmstateconfig (WritableAsset, on disk)
"c": {"b"}, // c = intermediate non-writable asset (AgentHosts, generated)
"b": {}, // b = agent-config (WritableAsset, on disk)
},
onDiskAssets: []string{"a", "b"}, // c is NOT on disk (it's generated)
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{},
expectedSource: onDiskSource,
description: "nmstateconfig should be preserved even with intermediate non-writable asset",
},
{
name: "Normal case: previously generated asset regenerated when parent changes",
assets: map[string][]string{
"a": {"b"},
"b": {},
},
onDiskAssets: []string{"b"},
stateFileAssets: []string{"a"},
target: "a",
expectedGenerationLog: []string{"a"},
expectedSource: generatedSource,
description: "When child is in state file (previously generated), regenerate it when parent on disk",
},
{
name: "Multiple user configs: all on disk, none in state file",
assets: map[string][]string{
"a": {"b", "c"},
"b": {},
"c": {},
},
onDiskAssets: []string{"a", "b", "c"},
stateFileAssets: nil,
target: "a",
expectedGenerationLog: []string{},
expectedSource: onDiskSource,
description: "All user-provided configs should be preserved",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
clearAssetBehaviors()
tempDir := t.TempDir()

// Create store with state file if needed
store := &storeImpl{
directory: tempDir,
assets: map[reflect.Type]*assetState{},
stateFileAssets: map[string]json.RawMessage{},
}

assets := make(map[string]asset.Asset, len(tc.assets))
for name := range tc.assets {
assets[name] = newTestStoreAsset(name)
}
for name, deps := range tc.assets {
dependenciesOfAsset := make([]asset.Asset, len(deps))
for i, d := range deps {
dependenciesOfAsset[i] = assets[d]
}
dependencies[reflect.TypeOf(assets[name])] = dependenciesOfAsset
}
for _, name := range tc.onDiskAssets {
onDiskAssets[reflect.TypeOf(assets[name])] = true
}

// Simulate assets in state file
for _, name := range tc.stateFileAssets {
store.stateFileAssets[reflect.TypeOf(assets[name]).String()] = json.RawMessage("{}")
}

err := store.fetch(context.Background(), assets[tc.target], "")
assert.NoError(t, err, "unexpected error: %s", tc.description)
assert.EqualValues(t, tc.expectedGenerationLog, generationLog, tc.description)
assert.Equal(t, tc.expectedSource, store.assets[reflect.TypeOf(assets[tc.target])].source, tc.description)
})
}
}

func TestStoreLoadOnDiskAssets(t *testing.T) {
cases := []struct {
name string
Expand Down