Skip to content

Commit 266c68a

Browse files
committed
test no annotation
Signed-off-by: Urvashi <umohnani@redhat.com>
1 parent 0c4d515 commit 266c68a

File tree

7 files changed

+193
-39
lines changed

7 files changed

+193
-39
lines changed

pkg/controller/bootstrap/bootstrap.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -398,19 +398,18 @@ func parseManifests(filename string, r io.Reader) ([]manifest, error) {
398398
// createPreBuiltImageMachineConfigs creates component MachineConfigs that set osImageURL for pools
399399
// that have associated MachineOSConfigs with pre-built image annotations.
400400
// These component MCs will be automatically merged into rendered MCs by the render controller.
401-
// This function performs strict validation at bootstrap time and will fail if:
402-
// - A MachineOSConfig is missing the pre-built image annotation
403-
// - The pre-built image format or digest is invalid
401+
// This function validates pre-built images at bootstrap time and will fail if the image format is invalid.
402+
// MOSCs without the annotation are skipped (this can happen if bootstrap runs again after seeding).
404403
func createPreBuiltImageMachineConfigs(machineOSConfigs []*mcfgv1.MachineOSConfig, pools []*mcfgv1.MachineConfigPool) ([]*mcfgv1.MachineConfig, error) {
405404
var preBuiltImageMCs []*mcfgv1.MachineConfig
406405

407-
// At bootstrap time, we require ALL MachineOSConfigs to have pre-built images
408-
// This is a strict requirement for day-0 hybrid OCL support
409406
for _, mosc := range machineOSConfigs {
410407
preBuiltImage, hasPreBuiltImage := mosc.Annotations[buildconstants.PreBuiltImageAnnotationKey]
408+
409+
// Skip if annotation is not present (could be a re-run after seeding completed)
411410
if !hasPreBuiltImage || preBuiltImage == "" {
412-
return nil, fmt.Errorf("MachineOSConfig %s is missing required annotation %s for bootstrap pre-built image support",
413-
mosc.Name, buildconstants.PreBuiltImageAnnotationKey)
411+
klog.V(4).Infof("Skipping MachineOSConfig %s - no pre-built image annotation (may have already been seeded)", mosc.Name)
412+
continue
414413
}
415414

416415
poolName := mosc.Spec.MachineConfigPool.Name

pkg/controller/build/constants/constants.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,6 @@ const (
4646
const (
4747
// PreBuiltImageAnnotationKey indicates a MachineOSConfig should be seeded with a pre-built image
4848
PreBuiltImageAnnotationKey = "machineconfiguration.openshift.io/pre-built-image"
49-
// PreBuiltImageSeededAnnotationKey indicates that the initial synthetic MOSB has been created for this MOSC
50-
PreBuiltImageSeededAnnotationKey = "machineconfiguration.openshift.io/pre-built-image-seeded"
5149
)
5250

5351
// Component MachineConfig naming for pre-built images

pkg/controller/build/helpers.go

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,6 @@ func hasPreBuiltImageAnnotation(mosc *mcfgv1.MachineOSConfig) bool {
262262
return exists
263263
}
264264

265-
// hasPreBuiltImageSeededAnnotation checks if a MachineOSConfig has been seeded with a pre-built image.
266-
func hasPreBuiltImageSeededAnnotation(mosc *mcfgv1.MachineOSConfig) bool {
267-
_, exists := mosc.Annotations[constants.PreBuiltImageSeededAnnotationKey]
268-
return exists
269-
}
270-
271265
// getPreBuiltImage returns the pre-built image from a MachineOSConfig's annotations.
272266
// Returns the image string and a boolean indicating if it exists and is non-empty.
273267
func getPreBuiltImage(mosc *mcfgv1.MachineOSConfig) (string, bool) {
@@ -278,18 +272,29 @@ func getPreBuiltImage(mosc *mcfgv1.MachineOSConfig) (string, bool) {
278272
// shouldSeedWithPreBuiltImage determines if a MachineOSConfig should be seeded with a pre-built image.
279273
// Returns true if:
280274
// - The MOSC has a pre-built image annotation
281-
// - The MOSC has NOT been seeded yet
282-
// - The MOSC does NOT have a current build annotation
275+
// - The MOSC does NOT have a current build annotation (meaning seeding hasn't happened yet)
283276
func shouldSeedWithPreBuiltImage(mosc *mcfgv1.MachineOSConfig) bool {
284277
return hasPreBuiltImageAnnotation(mosc) &&
285-
!hasPreBuiltImageSeededAnnotation(mosc) &&
286278
!hasCurrentBuildAnnotation(mosc)
287279
}
288280

289281
// isPreBuiltImageAwaitingSeeding checks if a MOSC has pre-built image annotation but hasn't been seeded.
290282
// This is useful for skipping normal build workflows when the seeding workflow should handle it.
283+
// Seeding is considered complete once the currentBuild annotation is set.
291284
func isPreBuiltImageAwaitingSeeding(mosc *mcfgv1.MachineOSConfig) bool {
292-
return hasPreBuiltImageAnnotation(mosc) && !hasPreBuiltImageSeededAnnotation(mosc)
285+
return hasPreBuiltImageAnnotation(mosc) && !hasCurrentBuildAnnotation(mosc)
286+
}
287+
288+
// needsPreBuiltImageAnnotationCleanup determines if a MOSC has completed seeding and
289+
// the pre-built image annotation can be safely removed.
290+
// Returns true if:
291+
// - The MOSC has a current build annotation (seeding is complete)
292+
// - The MOSC status has been populated with CurrentImagePullSpec
293+
// - The MOSC still has the PreBuiltImageAnnotationKey (needs cleanup)
294+
func needsPreBuiltImageAnnotationCleanup(mosc *mcfgv1.MachineOSConfig) bool {
295+
return hasCurrentBuildAnnotation(mosc) &&
296+
mosc.Status.CurrentImagePullSpec != "" &&
297+
hasPreBuiltImageAnnotation(mosc)
293298
}
294299

295300
// Looks at the error chain for the given error and determines if the error

pkg/controller/build/helpers_test.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
99
"github.com/openshift/machine-config-operator/pkg/apihelpers"
10+
"github.com/openshift/machine-config-operator/pkg/controller/build/constants"
1011
"github.com/openshift/machine-config-operator/pkg/controller/build/fixtures"
1112
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
1213
"github.com/stretchr/testify/assert"
@@ -198,3 +199,83 @@ func TestIsMachineOSBuildStatusUpdateNeeded(t *testing.T) {
198199
}
199200
}
200201
}
202+
203+
func TestNeedsPreBuiltImageAnnotationCleanup(t *testing.T) {
204+
t.Parallel()
205+
206+
tests := []struct {
207+
name string
208+
mosc *mcfgv1.MachineOSConfig
209+
expectedCleanup bool
210+
}{
211+
{
212+
name: "needs cleanup - all conditions met",
213+
mosc: &mcfgv1.MachineOSConfig{
214+
ObjectMeta: metav1.ObjectMeta{
215+
Name: "test",
216+
Annotations: map[string]string{
217+
constants.PreBuiltImageAnnotationKey: "registry.example.com/image@sha256:abc123",
218+
constants.CurrentMachineOSBuildAnnotationKey: "test-build-1",
219+
},
220+
},
221+
Status: mcfgv1.MachineOSConfigStatus{
222+
CurrentImagePullSpec: "registry.example.com/image@sha256:abc123",
223+
},
224+
},
225+
expectedCleanup: true,
226+
},
227+
{
228+
name: "no cleanup - missing current build annotation",
229+
mosc: &mcfgv1.MachineOSConfig{
230+
ObjectMeta: metav1.ObjectMeta{
231+
Name: "test",
232+
Annotations: map[string]string{
233+
constants.PreBuiltImageAnnotationKey: "registry.example.com/image@sha256:abc123",
234+
},
235+
},
236+
Status: mcfgv1.MachineOSConfigStatus{
237+
CurrentImagePullSpec: "registry.example.com/image@sha256:abc123",
238+
},
239+
},
240+
expectedCleanup: false,
241+
},
242+
{
243+
name: "no cleanup - status not populated",
244+
mosc: &mcfgv1.MachineOSConfig{
245+
ObjectMeta: metav1.ObjectMeta{
246+
Name: "test",
247+
Annotations: map[string]string{
248+
constants.PreBuiltImageAnnotationKey: "registry.example.com/image@sha256:abc123",
249+
constants.CurrentMachineOSBuildAnnotationKey: "test-build-1",
250+
},
251+
},
252+
Status: mcfgv1.MachineOSConfigStatus{
253+
CurrentImagePullSpec: "",
254+
},
255+
},
256+
expectedCleanup: false,
257+
},
258+
{
259+
name: "no cleanup - prebuilt image annotation already removed",
260+
mosc: &mcfgv1.MachineOSConfig{
261+
ObjectMeta: metav1.ObjectMeta{
262+
Name: "test",
263+
Annotations: map[string]string{
264+
constants.CurrentMachineOSBuildAnnotationKey: "test-build-1",
265+
},
266+
},
267+
Status: mcfgv1.MachineOSConfigStatus{
268+
CurrentImagePullSpec: "registry.example.com/image@sha256:abc123",
269+
},
270+
},
271+
expectedCleanup: false,
272+
},
273+
}
274+
275+
for _, tt := range tests {
276+
t.Run(tt.name, func(t *testing.T) {
277+
result := needsPreBuiltImageAnnotationCleanup(tt.mosc)
278+
assert.Equal(t, tt.expectedCleanup, result, "needsPreBuiltImageAnnotationCleanup() result mismatch")
279+
})
280+
}
281+
}

pkg/controller/build/reconciler.go

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -388,14 +388,22 @@ func (b *buildReconciler) updateMachineOSConfigStatus(ctx context.Context, mosc
388388
return nil
389389
}
390390

391-
// skip the status update if the current image pullspec equals the digest image pushspec.
392-
if mosc.Status.CurrentImagePullSpec == mosb.Status.DigestedImagePushSpec {
393-
klog.Infof("MachineOSConfig %q already has final image pushspec for MachineOSBuild %q", mosc.Name, mosb.Name)
391+
// Check if the machineOSBuild reference matches
392+
machineOSBuildRefMatches := mosc.Status.MachineOSBuild != nil && mosc.Status.MachineOSBuild.Name == mosb.Name
393+
394+
// skip the status update if both the current image pullspec and machineOSBuild reference are already correct.
395+
if mosc.Status.CurrentImagePullSpec == mosb.Status.DigestedImagePushSpec && machineOSBuildRefMatches {
396+
klog.Infof("MachineOSConfig %q already has final image pushspec and machineOSBuild reference for MachineOSBuild %q", mosc.Name, mosb.Name)
394397
return nil
395398
}
396399

397400
mosc.Status.CurrentImagePullSpec = mosb.Status.DigestedImagePushSpec
398401
mosc.Status.ObservedGeneration = mosc.GetGeneration()
402+
mosc.Status.MachineOSBuild = &mcfgv1.ObjectReference{
403+
Name: mosb.Name,
404+
Group: mcfgv1.SchemeGroupVersion.Group,
405+
Resource: "machineosbuilds",
406+
}
399407

400408
_, err = b.mcfgclient.MachineconfigurationV1().MachineOSConfigs().UpdateStatus(ctx, mosc, metav1.UpdateOptions{})
401409
if err == nil {
@@ -1245,6 +1253,19 @@ func (b *buildReconciler) syncMachineOSConfigs(ctx context.Context) error {
12451253
// should be created.
12461254
func (b *buildReconciler) syncMachineOSConfig(ctx context.Context, mosc *mcfgv1.MachineOSConfig) error {
12471255
return b.timeObjectOperation(mosc, syncingVerb, func() error {
1256+
// Check if we need to clean up the pre-built image annotation after successful seeding
1257+
// This happens in a separate reconciliation to ensure status is persisted before removing annotation
1258+
if needsPreBuiltImageAnnotationCleanup(mosc) {
1259+
klog.Infof("MachineOSConfig %q seeding complete and status populated, removing pre-built image annotation", mosc.Name)
1260+
delete(mosc.Annotations, constants.PreBuiltImageAnnotationKey)
1261+
if _, err := b.mcfgclient.MachineconfigurationV1().MachineOSConfigs().Update(ctx, mosc, metav1.UpdateOptions{}); err != nil {
1262+
return fmt.Errorf("could not remove pre-built image annotation from MachineOSConfig %q: %w", mosc.Name, err)
1263+
}
1264+
klog.Infof("Successfully removed pre-built image annotation from MachineOSConfig %q", mosc.Name)
1265+
// Trigger another sync to handle any pending work now that annotation is removed
1266+
return nil
1267+
}
1268+
12481269
mosbs, err := b.getMachineOSBuildsForMachineOSConfig(mosc)
12491270
if err != nil {
12501271
return fmt.Errorf("could not list MachineOSBuilds for MachineOSConfig %q: %w", mosc.Name, err)
@@ -1331,8 +1352,9 @@ func (b *buildReconciler) reconcilePoolChange(ctx context.Context, mcp *mcfgv1.M
13311352

13321353
// If the MachineOSConfig has a pre-built image annotation AND hasn't been seeded yet,
13331354
// the seeding workflow should handle creating the synthetic build. Don't proceed with normal build workflow.
1355+
// Seeding is considered complete once the currentBuild annotation is set.
13341356
firstOptIn := mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey]
1335-
if hasPreBuiltImageAnnotation(mosc) && !hasPreBuiltImageSeededAnnotation(mosc) && firstOptIn == "" {
1357+
if hasPreBuiltImageAnnotation(mosc) && firstOptIn == "" {
13361358
klog.Infof("MachineOSConfig %q has pre-built image annotation but hasn't been seeded yet, skipping pool change reconciliation (seeding workflow should handle this)", mosc.Name)
13371359
return nil
13381360
}
@@ -1861,9 +1883,9 @@ func (b *buildReconciler) createSyntheticMachineOSBuild(ctx context.Context, mos
18611883

18621884
// updateMachineOSConfigForSeeding updates MachineOSConfig for seeded state
18631885
func (b *buildReconciler) updateMachineOSConfigForSeeding(ctx context.Context, mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild, imageSpec string) error {
1864-
// Update annotations - add both current build and seeded marker
1886+
// Update annotations - set current build annotation to mark seeding as complete
1887+
// The currentBuild annotation serves as the marker that seeding has occurred
18651888
metav1.SetMetaDataAnnotation(&mosc.ObjectMeta, constants.CurrentMachineOSBuildAnnotationKey, mosb.Name)
1866-
metav1.SetMetaDataAnnotation(&mosc.ObjectMeta, constants.PreBuiltImageSeededAnnotationKey, "true")
18671889

18681890
// Update the MachineOSConfig object
18691891
updatedMOSC, err := b.mcfgclient.MachineconfigurationV1().MachineOSConfigs().Update(ctx, mosc, metav1.UpdateOptions{})

pkg/controller/build/reconciler_seeding_test.go

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -324,11 +324,13 @@ func TestUpdateMachineOSConfigForSeeding(t *testing.T) {
324324

325325
for _, tt := range tests {
326326
t.Run(tt.name, func(t *testing.T) {
327-
// Create test objects
327+
// Create test objects with pre-built image annotation
328328
mosc := &mcfgv1.MachineOSConfig{
329329
ObjectMeta: metav1.ObjectMeta{
330-
Name: "layered",
331-
Annotations: make(map[string]string),
330+
Name: "layered",
331+
Annotations: map[string]string{
332+
constants.PreBuiltImageAnnotationKey: tt.imageSpec,
333+
},
332334
},
333335
Spec: mcfgv1.MachineOSConfigSpec{
334336
MachineConfigPool: mcfgv1.MachineConfigPoolReference{Name: "layered"},
@@ -353,20 +355,34 @@ func TestUpdateMachineOSConfigForSeeding(t *testing.T) {
353355
t.Errorf("Unexpected error: %v", err)
354356
}
355357

356-
// Verify annotations were set
357-
if mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey] != mosb.Name {
358+
// Fetch the updated MOSC from the fake client
359+
updatedMOSC, err := mcfgClient.MachineconfigurationV1().MachineOSConfigs().Get(ctx, mosc.Name, metav1.GetOptions{})
360+
if err != nil {
361+
t.Fatalf("Failed to get updated MachineOSConfig: %v", err)
362+
}
363+
364+
// Verify current build annotation was set (this marks seeding as complete)
365+
if updatedMOSC.Annotations[constants.CurrentMachineOSBuildAnnotationKey] != mosb.Name {
358366
t.Errorf("Expected current build annotation to be %q, got %q",
359-
mosb.Name, mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey])
367+
mosb.Name, updatedMOSC.Annotations[constants.CurrentMachineOSBuildAnnotationKey])
368+
}
369+
370+
// IMPORTANT: Verify pre-built image annotation is NOT removed during seeding
371+
// It will be removed in a separate reconciliation after status is confirmed persisted
372+
if _, hasAnnotation := updatedMOSC.Annotations[constants.PreBuiltImageAnnotationKey]; !hasAnnotation {
373+
t.Error("Pre-built image annotation should NOT be removed during seeding (removed in separate cleanup step)")
360374
}
361375

362-
// Verify build annotation was set
363-
if mosc.Annotations[constants.CurrentMachineOSBuildAnnotationKey] == "" {
364-
t.Error("Expected current MachineOSBuild annotation to be set")
376+
// Verify status has the image pullspec
377+
if updatedMOSC.Status.CurrentImagePullSpec != mcfgv1.ImageDigestFormat(tt.imageSpec) {
378+
t.Errorf("Expected Status.CurrentImagePullSpec to be %q, got %q",
379+
tt.imageSpec, updatedMOSC.Status.CurrentImagePullSpec)
365380
}
366381

367-
// Verify status was updated - Note: This would require the update to succeed
368-
// In a real test with proper setup, we'd verify the status was updated
369-
// For now, we just verify the function doesn't error
382+
// Verify status has the MachineOSBuild reference
383+
if updatedMOSC.Status.MachineOSBuild == nil || updatedMOSC.Status.MachineOSBuild.Name != mosb.Name {
384+
t.Errorf("Expected Status.MachineOSBuild.Name to be %q", mosb.Name)
385+
}
370386
})
371387
}
372388
}

pkg/operator/sync.go

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2350,8 +2350,41 @@ func (optr *Operator) syncPreBuiltImageMachineConfigs() error {
23502350
return fmt.Errorf("failed to list MachineOSConfigs: %w", err)
23512351
}
23522352

2353-
// Build map of pools that should have pre-built image component MCs using common helper
2354-
poolsWithPreBuiltImages := ctrlcommon.BuildPoolToPreBuiltImageMap(moscs, buildconstants.PreBuiltImageAnnotationKey)
2353+
// Build map of pools that should have pre-built image component MCs
2354+
// Only use the annotation for pre-seeding/bootstrap. After seeding is complete
2355+
// (indicated by Seeded condition), we should not update the component MC anymore
2356+
// to avoid triggering new builds every time the status changes.
2357+
poolsWithPreBuiltImages := make(map[string]string)
2358+
for _, mosc := range moscs {
2359+
poolName := mosc.Spec.MachineConfigPool.Name
2360+
2361+
// Check if seeding is complete by looking for the Seeded condition
2362+
hasSeededCondition := false
2363+
for _, condition := range mosc.Status.Conditions {
2364+
if condition.Type == "Seeded" && condition.Status == metav1.ConditionTrue {
2365+
hasSeededCondition = true
2366+
break
2367+
}
2368+
}
2369+
2370+
// Only use the annotation for component MC creation/updates.
2371+
// Do NOT use status.currentImagePullSpec after seeding, as that would
2372+
// create an infinite loop: build completes → status updated → MC updated →
2373+
// rendered config changes → new build triggered → repeat forever.
2374+
var preBuiltImage string
2375+
if !hasSeededCondition {
2376+
if annotationImage, hasAnnotation := mosc.Annotations[buildconstants.PreBuiltImageAnnotationKey]; hasAnnotation && annotationImage != "" {
2377+
preBuiltImage = annotationImage
2378+
klog.V(4).Infof("MachineOSConfig %s has pre-built image %s in annotation for pool %s (pre-seeding)", mosc.Name, preBuiltImage, poolName)
2379+
}
2380+
} else {
2381+
klog.V(4).Infof("MachineOSConfig %s has been seeded, skipping component MC updates for pool %s to prevent build loops", mosc.Name, poolName)
2382+
}
2383+
2384+
if preBuiltImage != "" {
2385+
poolsWithPreBuiltImages[poolName] = preBuiltImage
2386+
}
2387+
}
23552388

23562389
// Get all existing pre-built image component MCs
23572390
allMCs, err := optr.mcLister.List(labels.Everything())

0 commit comments

Comments
 (0)