AGENT-1491: reclaim disk space from the node when IRI resource is deleted #5988

pablintino · 2026-05-13T14:33:50Z

nit: Claude and other AI usually violates some basic principles like encapsulation or to avoid testing directly private APIs. Would be nice to teach Claude to test everything based on the public interfaces.

rwsu · 2026-05-05T05:48:08Z

Should the base directory /var/lib/iri-registry also be removed?

That's a very good question. I preferred not to remove it since MCD does not technically own it (it's created by the Assisted Installer during the installation). On the other hand, it's also true that we don't support re-enabling the feature once removed, so maybe it could be a safer option

@andfasano if removing the entire directory is an option, why don't we just remove the entire dir without handling all of this?:

path := filepath.Join(base, entry.Name()) rel, err := filepath.Rel(base, path) if err != nil { return fmt.Errorf("failed to validate registry data path %q: %w", path, err) } if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { return fmt.Errorf("refusing to remove path outside registry data path: %q", path) } if err := os.RemoveAll(path); err != nil { return fmt.Errorf("failed to remove registry data path %q: %w", path, err) }

That said, it seems that if we remove the entire directory we would need to rework wasFeatureActivated.

Yeah, atm it seems safer/simpler to just remove the entire content of the folder, and leave the folder where it was. I don't think it's a real concern in general, given that it's essentially a one-off operation

@andfasano yes, not a blocker, just a nit/idea. The way it's is fine and simplifies detecting if the feature was ever used, what's nice too.

-Original file line number
+Diff line change
@@ Expand Up / @@ -154,6 +154,7 @@ const ( @@
     	IRILoadImageScriptPath = "/usr/local/bin/load-registry-image.sh"
     	IRIRootCAPath          = "/etc/pki/ca-trust/source/anchors/iri-root-ca.crt"
     	IRIRegistryServiceName = "iri-registry.service"
+    	IRIRegistryDataPath    = "/var/lib/iri-registry"
     	// rpm-ostree command arguments
     	RPMOSTreeUpdateArg    = "update"
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,49 @@
+    ---
+    name: iri-mcd
+    description: Describes the acceptance criteria and BDD tests to be used for new/existing features of the InternalReleaseImage MachineConfigDaemon manager. The manager implements the NoRegistryClusterInstall main feature for the part related to manage/monitor a single control plane node
+    user-invocable: false
+    ---
+    # InternalReleaseImage daemon manager BDD workflow
+    When working on InternalReleaseImage daemon manager changes:
+. Identify the specific behavior being changed.
+. Read only the relevant acceptance file under `acceptance/`.
+. If the change touches multiple behaviors, read all matching files.
+. Do not invent behavior that is not covered by the acceptance criteria.
+. When behavior changes, update the relevant acceptance file before implementing code.
+    # General implementation notes
+    When defining the implementation:
+    - Ensure that any new portion of code is covered at least by one or more unit tests.
+    - Keep production changes minimal.
+    - Avoid duplications, prefer a coding style that improves the readability and maintenance.
+    - Do not perform broad refactors unless needed to make the behavior testable.
+    - If a behavior is not covered by acceptance criteria, stop and ask before implementing it.
+    - When reporting completion, map each changed test back to the acceptance scenario it covers.
+    - Minimize comments, and keep them short.
+    # Specific IRI MCD manager implementation notes
+    - Keep the `syncInternalReleaseImage` method short and readable. Prefer to refactor included tasks into private type methods.
+    - Do not instantiate the IRI registry within `syncInternalReleaseImage` method more than once per loop: keep it simple stupid.
+    # Test implementation notes
+    - When testing different cases for the same scenario, use the cases := []struct{} to capture the relevant key fields for the test. Add always a speaking
+      name field to represent the current case.
+    - Reuse the existing test methods if possible.
+    - Keep the test focused on the behaviors, avoid testing unnecessary technical details.
+    - Do not add too many comments to the tests
+    # Functional testing notes
+    The NoRegistryClusterInstall feature is also tested functionally in `/test/e2e-iri` folder.
+    ## Supporting files
+    - `acceptance/storage-reclaim.md`: IRI deletion workflow, how to reclaim the disk node space previously used.
+    - `acceptance/feature-disabled.md`: the expected behavior when the feature is not enabled.

-Original file line number
+Diff line change
@@ -0,0 +1,17 @@
+    # IRI feature disabled
+    ## Goal
+    Avoid to consume cpu/mem resources when the NoRegistryClusterInstall is disabled. There could be two different scenarios to consider:
+    - The feature was never activated
+    - The user explicitly disabled the feature by deleting the IRI resource
+    In both the cases, we'd like to consume as few as possible resources when the feature is not enabled
+    ## Scenario: skip everything if feature is disabled
+    Given the `/var/lib/iri-registry` does not exist
+    When the manager reconciles
+    Then skip all the actions and return immediately (requeue with a longer time)

-Original file line number
+Diff line change
@@ -0,0 +1,31 @@
+    # IRI storage reclaim acceptance criteria
+    ## Goal
+    When the singleton InternalReleaseImage resource (name "cluster") is deleted, MCD should reclaim storage used by the local IRI registry backend without racing against registry shutdown.
+    Since the deletion of the IRI singleton resource will be used to opt-out from the NoRegistryClusterInstall feature, there's no specific need to
+    report the progress. It's important that the manager will ensure that the storage is cleaned up when the IRI is not present and the registry is down.
+    The local IRI registry service will be stopped automatically when the IRI MachineConfigs will be deleted by the IRI controller
+    ## Scenario: ensure disk cleanup on IRI deletion
+    Given the IRI resource is deleted
+    And the local IRI registry is not active on port 22625
+    And `/var/lib/iri-registry` is not empty
+    When the manager reconciles
+    Then it should remove `/var/lib/iri-registry` content
+    ## Scenario: storage is not cleaned up during IRI deletion
+    Given the IRI resource is being deleted
+    And the local IRI registry is still active
+    When the manager reconciles
+    Then it should not remove `/var/lib/iri-registry`
+    And it should wait for the local IRI registry to be stopped
+    And avoid to perform any other task

-Original file line number
+Diff line change
@@ Expand Up / @@ -31,6 +31,12 @@ func iri() *iriBuilder { @@
     	}
     }
+    func (ib *iriBuilder) withDeletionTimestamp() *iriBuilder {
+    	now := metav1.Now()
+    	ib.obj.DeletionTimestamp = &now
+    	return ib
+    }
     func (ib *iriBuilder) build() runtime.Object {
     	return ib.obj
     }
@@ Expand Down @@

Navigation Menu

Uh oh!

AGENT-1491: reclaim disk space from the node when IRI resource is deleted #5988

Uh oh!

Diff view

Diff view

There are no files selected for viewing

pablintino May 13, 2026

Uh oh!

rwsu May 5, 2026

Uh oh!

andfasano May 5, 2026

Uh oh!

pablintino May 13, 2026

Uh oh!

andfasano May 14, 2026

Uh oh!

pablintino May 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -3,7 +3,10 @@ package internalreleaseimage @@
     import (
     	"context"
     	"fmt"
+    	"net"
     	"net/http"
+    	"os"
+    	"path/filepath"
     	"reflect"
     	"strings"
     	"time"
@@ Expand All / @@ -27,6 +30,7 @@ import ( @@
     	mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1"
     	mcfglistersv1alpha1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1alpha1"
     	"github.com/openshift/machine-config-operator/pkg/controller/common"
+    	"github.com/openshift/machine-config-operator/pkg/daemon/constants"
     )
     const (
@@ Expand All / @@ -44,6 +48,8 @@ type Manager struct { @@
     	registryClient *http.Client
     	// authToken overrides the token read from the kubelet auth file; used in tests.
     	authToken string
+    	// registryDataPath overrides the default registry data path; used in tests.
+    	registryDataPath string
     	syncHandler                 func(iri string) error
     	enqueueInternalReleaseImage func(*mcfgv1alpha1.InternalReleaseImage)
@@ Expand Down Expand Up @@
     	return i.updateMCNStatus(mcn, mcnUpdated)
     }
+    // reclaimRegistryStorage removes the IRI registry data directory contents when safe.
+    // Returns an error if the directory cannot be removed, or nil if removal succeeded
+    // or if the directory doesn't exist.
+    func (i *Manager) reclaimRegistryStorage() error {
+    	registryDataPath := i.registryDataPath
+    	if registryDataPath == "" {
+    		registryDataPath = constants.IRIRegistryDataPath
+    	}
+    	// Check if directory exists
+    	info, err := os.Stat(registryDataPath)
+    	if err != nil {
+    		if os.IsNotExist(err) {
+    			klog.V(2).Infof("Registry data directory %s does not exist, nothing to reclaim", registryDataPath)
+    			return nil
+    		}
+    		return fmt.Errorf("failed to stat registry data path %s: %w", registryDataPath, err)
+    	}
+    	// Verify it's a directory
+    	if !info.IsDir() {
+    		return fmt.Errorf("registry data path %s exists but is not a directory", registryDataPath)
+    	}
+    	base, err := filepath.Abs(registryDataPath)
+    	if err != nil {
+    		return fmt.Errorf("failed to resolve registry data path %q: %w", registryDataPath, err)
+    	}
+    	base = filepath.Clean(base)
+    	if base == string(os.PathSeparator) {
+    		return fmt.Errorf("invalid registry data path")
+    	}
+    	entries, err := os.ReadDir(base)
+    	if err != nil {
+    		return fmt.Errorf("failed to read registry data path %q: %w", base, err)
+    	}
+    	for _, entry := range entries {
+    		path := filepath.Join(base, entry.Name())
+    		rel, err := filepath.Rel(base, path)
+    		if err != nil {
+    			return fmt.Errorf("failed to validate registry data path %q: %w", path, err)
+    		}
+    		if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+    			return fmt.Errorf("refusing to remove path outside registry data path: %q", path)
+    		}
+    		if err := os.RemoveAll(path); err != nil {
+    			return fmt.Errorf("failed to remove registry data path %q: %w", path, err)
+    		}
+    		klog.V(2).Infof("Removed registry data path: %s", path)
+    	}
+    	klog.Infof("Successfully reclaimed storage: removed %d entries from %s", len(entries), registryDataPath)
+    	return nil
+    }
+    // getIRIRegistry creates and returns an IRI registry client.
+    // Returns the registry and an error indicating whether the registry is reachable.
+    func (i *Manager) getIRIRegistry() (*iriRegistry, error) {
+    	authToken := i.authToken
+    	if authToken == "" {
+    		var err error
+    		authToken, err = readIRIAuthToken(net.JoinHostPort(iriRegistryHost, fmt.Sprintf("%d", iriRegistryPort)))
+    		if err != nil {
+    			return nil, fmt.Errorf("could not read IRI auth token: %w", err)
+    		}
+    	}
+    	iriReg := newIRIRegistry(i.nodeName, i.registryClient, authToken)
+    	err := iriReg.CheckLocalRegistry()
+    	return iriReg, err
+    }
+    // wasFeatureActivated checks if the NoRegistryClusterInstall feature was ever activated on this node.
+    // Returns true if the registry data directory exists (feature was activated),
+    // false if the directory doesn't exist (feature never used),
+    // or an error if the check failed (permission denied, I/O error, etc.).
+    func (i *Manager) wasFeatureActivated() (bool, error) {
+    	registryDataPath := i.registryDataPath
+    	if registryDataPath == "" {
+    		registryDataPath = constants.IRIRegistryDataPath
+    	}
+    	_, err := os.Stat(registryDataPath)
+    	if err != nil {
+    		if os.IsNotExist(err) {
+    			return false, nil // Directory doesn't exist - feature never activated
+    		}
+    		return false, fmt.Errorf("failed to check registry data path %s: %w", registryDataPath, err)
+    	}
+    	return true, nil
+    }
+    // isRegistryPortListening checks if the registry port is accepting connections.
+    // Returns true if port is listening (registry service is running), false otherwise.
+    // This is a cheap TCP dial check (~microseconds for localhost) that provides a stronger
+    // signal than HTTP errors, which can occur for reasons other than service being down.
+    func (i *Manager) isRegistryPortListening() bool {
+    	address := net.JoinHostPort(iriRegistryHost, fmt.Sprintf("%d", iriRegistryPort))
+    	conn, err := net.DialTimeout("tcp", address, 100*time.Millisecond)
+    	if err != nil {
+    		return false
+    	}
+    	conn.Close()
+    	return true
+    }
+    // handleIRIDeletion handles the IRI deletion scenario (both in-progress and completed).
+    // This method is only called when the registry directory exists (feature was activated).
+    // If registry port is still listening, it waits for shutdown.
+    // If registry port is down, it cleans up MCN status and reclaims storage.
+    func (i *Manager) handleIRIDeletion(mcn *mcfgv1.MachineConfigNode) error {
+    	// Check if registry port is listening
+    	if i.isRegistryPortListening() {
+    		// Registry port is still listening - wait for shutdown and avoid any other task
+    		klog.V(2).Infof("Registry port is still listening, waiting for shutdown before cleanup")
+    		i.queue.AddAfter(common.InternalReleaseImageInstanceName, syncRetryInterval)
+    		return nil
+    	}
+    	// Registry port is not listening - safe to clean up and reclaim storage
+    	klog.V(2).Infof("Registry port is not listening - proceeding with cleanup")
+    	// Clean up MCN status
+    	if err := i.cleanupMachineConfigNodeStatus(mcn); err != nil {
+    		return fmt.Errorf("failed to cleanup MCN: %w", err)
+    	}
+    	// Reclaim storage
+    	if err := i.reclaimRegistryStorage(); err != nil {
+    		return fmt.Errorf("failed to reclaim registry storage: %w", err)
+    	}
+    	return nil
+    }
     func (i *Manager) syncInternalReleaseImage(key string) error {
     	klog.V(4).Infof("Syncing InternalReleaseImage %q", key)
+    	// Check if feature was ever activated - if not, skip all work
+    	wasActivated, err := i.wasFeatureActivated()
+    	if err != nil {
+    		return err
+    	}
+    	if !wasActivated {
+    		klog.V(4).Infof("InternalReleaseImage feature never activated")
+    		i.queue.AddAfter(common.InternalReleaseImageInstanceName, 5*time.Minute)
+    		return nil
+    	}
     	// Get the MachineConfigNode for the current node.
     	mcn, err := i.mcnLister.Get(i.nodeName)
     	if err != nil {
@@ Expand All @@
     		return err
     	}
-    	// Fetch the InternalReleaseImage.
-    	_, err = i.iriLister.Get(common.InternalReleaseImageInstanceName)
-    	if apierrors.IsNotFound(err) {
-    		// Manage the feature only when the IRI resource was defined.
-    		// If not present, refresh the related MCN resource if required.
-    		err = i.cleanupMachineConfigNodeStatus(mcn)
-    		if err != nil {
-    			return err
-    		}
-    		return nil
-    	}
+    	// Check if IRI resource exists
+    	iri, err := i.iriLister.Get(common.InternalReleaseImageInstanceName)
     	if err != nil {
+    		if apierrors.IsNotFound(err) {
+    			return i.handleIRIDeletion(mcn)
+    		}
     		return err
     	}
-    	authToken := i.authToken
-    	var registryErr error
-    	if authToken == "" {
-    		authToken, registryErr = readIRIAuthToken(fmt.Sprintf("%s:%d", iriRegistryHost, iriRegistryPort))
-    	}
-    	var iriReg *iriRegistry
-    	if registryErr == nil {
-    		iriReg = newIRIRegistry(i.nodeName, i.registryClient, authToken)
-    		registryErr = iriReg.CheckLocalRegistry()
+    	// Check if IRI is being deleted
+    	if !iri.DeletionTimestamp.IsZero() {
+    		return i.handleIRIDeletion(mcn)
     	}
+    	// Update MCN status based on registry availability
+    	iriReg, registryErr := i.getIRIRegistry()
     	if registryErr != nil {
     		err = i.setMachineConfigNodeAsDegraded(mcn, registryErr)
     	} else {
@@ Expand Down @@

Uh oh!

AGENT-1491: reclaim disk space from the node when IRI resource is deleted #5988

Uh oh!

AGENT-1491: reclaim disk space from the node when IRI resource is deleted #5988

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

pablintino May 13, 2026

Choose a reason for hiding this comment

Uh oh!

rwsu May 5, 2026

Choose a reason for hiding this comment

Uh oh!

andfasano May 5, 2026

Choose a reason for hiding this comment

Uh oh!

pablintino May 13, 2026

Choose a reason for hiding this comment

Uh oh!

andfasano May 14, 2026

Choose a reason for hiding this comment

Uh oh!

pablintino May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!