From bb16a4f00467ff0a7c2c07e1d7748b855b21777c Mon Sep 17 00:00:00 2001 From: Pawan Pinjarkar Date: Thu, 12 Mar 2026 20:28:30 -0400 Subject: [PATCH] OCPBUGS-78420: preserve user-provided configs when multiple on-disk assets coexist The asset store's dependency tracking previously marked any asset as "dirty" when a parent dependency was loaded from disk, triggering regeneration even when both parent and child were user-provided configuration files that should coexist. This broke the OVE use case where agent-config.yaml (rendezvousIP only) and nmstateconfig.yaml (network config) can coexist without install-config.yaml. The installer would discard nmstateconfig.yaml, then fail to regenerate it since agent-config has no hosts. Fix by adding "dirtyDueToUserConfigs" tracking to assetState: - Propagates through dependency chains including intermediate assets - Preserves on-disk assets when dirty only due to user configs AND not in state file (newly user-provided) - Still regenerates assets in state file when dependencies change (backward compatibility) Co-Authored-By: Claude Sonnet 4.5 --- .../agent/image/unconfigured_ignition_test.go | 31 ++++ pkg/asset/store/store.go | 37 ++++- pkg/asset/store/store_test.go | 136 +++++++++++++++++- 3 files changed, 197 insertions(+), 7 deletions(-) diff --git a/pkg/asset/agent/image/unconfigured_ignition_test.go b/pkg/asset/agent/image/unconfigured_ignition_test.go index 858c62fc73d..2d1e2edd35d 100644 --- a/pkg/asset/agent/image/unconfigured_ignition_test.go +++ b/pkg/asset/agent/image/unconfigured_ignition_test.go @@ -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) { @@ -24,6 +25,7 @@ func TestUnconfiguredIgnition_Generate(t *testing.T) { defer setupEmbeddedResources(t)() nmStateConfig := getTestNMStateConfig() + agentConfigWithRendezvousIPOnly := getTestAgentConfigWithRendezvousIPOnly() cases := []struct { name string @@ -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) { @@ -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", diff --git a/pkg/asset/store/store.go b/pkg/asset/store/store.go index ca202b9323c..4cde8c65e94 100644 --- a/pkg/asset/store/store.go +++ b/pkg/asset/store/store.go @@ -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 } // storeImpl is the implementation of Store. @@ -243,6 +247,7 @@ 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 { @@ -250,6 +255,14 @@ func (s *storeImpl) load(a asset.Asset, indent string) (*assetState, error) { } 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 + } } } @@ -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 { @@ -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 // A parent is dirty. The asset must be re-generated. case anyParentsDirty: if foundOnDisk { @@ -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 diff --git a/pkg/asset/store/store_test.go b/pkg/asset/store/store_test.go index 4378d6f8b3c..09cb9aaa50e 100644 --- a/pkg/asset/store/store_test.go +++ b/pkg/asset/store/store_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "encoding/json" "os" "path/filepath" "reflect" @@ -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 @@ -305,6 +307,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) { "b": {}, }, onDiskAssets: nil, + stateFileAssets: nil, target: "a", expectedGenerationLog: []string{"b", "a"}, expectedDirty: false, @@ -316,6 +319,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) { "b": {}, }, onDiskAssets: []string{"a"}, + stateFileAssets: nil, target: "a", expectedGenerationLog: []string{}, expectedDirty: false, @@ -327,6 +331,7 @@ func TestStoreFetchOnDiskAssets(t *testing.T) { "b": {}, }, onDiskAssets: []string{"b"}, + stateFileAssets: nil, target: "a", expectedGenerationLog: []string{"a"}, expectedDirty: true, @@ -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, @@ -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 { @@ -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) @@ -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