Merge pull request #2670 from dtantsur/inspectionmode

✨ Introduce InspectionMode field

Author: metal3-io-bot

apis/metal3.io/v1alpha1/baremetalhost_types.go

@@ -67,9 +67,22 @@ const (
 	// InspectAnnotationValueDisabled is a constant string="disabled"
 	// This is particularly useful to check if inspect annotation is disabled
 	// inspect.metal3.io=disabled.
+	//
+	// Deprecated: use InspectionMode instead.
 	InspectAnnotationValueDisabled = "disabled"
 )
 
+// InspectionMode represents the mode of host inspection.
+type InspectionMode string
+
+const (
+	// InspectionModeDisabled disables host inspection.
+	InspectionModeDisabled InspectionMode = "disabled"
+
+	// InspectionModeAgent runs standard agent-based inspection.
+	InspectionModeAgent InspectionMode = "agent"
+)
+
 // RootDeviceHints holds the hints for specifying the storage location
 // for the root filesystem for the image.
 type RootDeviceHints struct {
@@ -485,6 +498,13 @@ type BareMetalHostSpec struct {
 	// instead, a reboot will be used in place of power on/off
 	// +optional
 	DisablePowerOff bool `json:"disablePowerOff,omitempty"`
+
+	// Specifies the mode for host inspection.
+	// "disabled" - no inspection will be performed
+	// "agent" - normal agent-based inspection will run
+	// +optional
+	// +kubebuilder:validation:Enum=disabled;agent
+	InspectionMode InspectionMode `json:"inspectionMode,omitempty"`
 }
 
 // AutomatedCleaningMode is the interface to enable/disable automated cleaning
@@ -974,6 +994,19 @@ func (host *BareMetalHost) CredentialsKey() types.NamespacedName {
 	}
 }
 
+// InspectionDisabled returns true if inspection is disabled via either
+// the inspect.metal3.io annotation or the inspectionMode field.
+func (host *BareMetalHost) InspectionDisabled() bool {
+	// Check the new InspectionMode field first
+	if host.Spec.InspectionMode == InspectionModeDisabled {
+		return true
+	}
+
+	// Fall back to the legacy annotation for backward compatibility
+	annotations := host.GetAnnotations()
+	return annotations[InspectAnnotationPrefix] == InspectAnnotationValueDisabled
+}
+
 // NeedsHardwareInspection looks at the state of the host to determine
 // if hardware inspection should be run.
 func (host *BareMetalHost) NeedsHardwareInspection() bool {
@@ -987,6 +1020,11 @@ func (host *BareMetalHost) NeedsHardwareInspection() bool {
 		// this host, because we don't want to reboot it.
 		return false
 	}
+	if host.InspectionDisabled() {
+		// Never perform inspection if it's explicitly disabled
+		return false
+	}
+	// FIXME(dtantsur): the HardwareDetails field is deprecated.
 	return host.Status.HardwareDetails == nil
 }
 

apis/metal3.io/v1alpha1/baremetalhost_types_test.go

@@ -665,3 +665,55 @@ func TestHasBMCDetails(t *testing.T) {
 		})
 	}
 }
+
+func TestInspectionDisabled(t *testing.T) {
+	for _, tc := range []struct {
+		Scenario       string
+		InspectionMode InspectionMode
+		Annotations    map[string]string
+		Expected       bool
+	}{
+		{
+			Scenario: "default",
+		},
+		{
+			Scenario: "annotation",
+			Annotations: map[string]string{
+				InspectAnnotationPrefix: "disabled",
+			},
+			Expected: true,
+		},
+		{
+			Scenario: "empty annotation value",
+			Annotations: map[string]string{
+				InspectAnnotationPrefix: "",
+			},
+		},
+		{
+			Scenario:       "InspectionMode",
+			InspectionMode: InspectionModeDisabled,
+			Expected:       true,
+		},
+		{
+			Scenario: "both",
+			Annotations: map[string]string{
+				InspectAnnotationPrefix: "disabled",
+			},
+			InspectionMode: InspectionModeDisabled,
+			Expected:       true,
+		},
+	} {
+		t.Run(tc.Scenario, func(t *testing.T) {
+			host := BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tc.Annotations,
+				},
+				Spec: BareMetalHostSpec{
+					InspectionMode: tc.InspectionMode,
+				},
+			}
+			actual := host.InspectionDisabled()
+			assert.Equal(t, tc.Expected, actual)
+		})
+	}
+}

config/base/crds/bases/metal3.io_baremetalhosts.yaml

@@ -288,6 +288,15 @@ spec:
                 required:
                 - url
                 type: object
+              inspectionMode:
+                description: |-
+                  Specifies the mode for host inspection.
+                  "disabled" - no inspection will be performed
+                  "agent" - normal agent-based inspection will run
+                enum:
+                - disabled
+                - agent
+                type: string
               metaData:
                 description: |-
                   MetaData holds the reference to the Secret containing host metadata

config/render/capm3.yaml

@@ -288,6 +288,15 @@ spec:
                 required:
                 - url
                 type: object
+              inspectionMode:
+                description: |-
+                  Specifies the mode for host inspection.
+                  "disabled" - no inspection will be performed
+                  "agent" - normal agent-based inspection will run
+                enum:
+                - disabled
+                - agent
+                type: string
               metaData:
                 description: |-
                   MetaData holds the reference to the Secret containing host metadata

internal/controller/metal3.io/baremetalhost_controller.go

@@ -269,7 +269,7 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 // inspect.metal3.io=disabled or there are no existing HardwareDetails.
 func (r *BareMetalHostReconciler) updateHardwareDetails(ctx context.Context, request ctrl.Request, host *metal3api.BareMetalHost) (bool, error) {
 	updated := false
-	if host.Status.HardwareDetails == nil || inspectionDisabled(host) {
+	if host.Status.HardwareDetails == nil || host.InspectionDisabled() {
 		objHardwareDetails, err := r.getHardwareDetailsFromAnnotation(host)
 		if err != nil {
 			return updated, fmt.Errorf("error parsing HardwareDetails from annotation: %w", err)
@@ -446,16 +446,9 @@ func clearRebootAnnotations(host *metal3api.BareMetalHost) (dirty bool) {
 	return
 }
 
-// inspectionDisabled checks for existence of inspect.metal3.io=disabled
-// which means we don't inspect even in Inspecting state.
-func inspectionDisabled(host *metal3api.BareMetalHost) bool {
-	annotations := host.GetAnnotations()
-	return annotations[metal3api.InspectAnnotationPrefix] == metal3api.InspectAnnotationValueDisabled
-}
-
-// hasInspectAnnotation checks for existence of inspect.metal3.io annotation
-// and returns true if it exist.
-func hasInspectAnnotation(host *metal3api.BareMetalHost) bool {
+// inspectionRefreshRequested checks for existence of inspect.metal3.io
+// annotation and returns true if it exist.
+func inspectionRefreshRequested(host *metal3api.BareMetalHost) bool {
 	annotations := host.GetAnnotations()
 	if annotations != nil {
 		if expect, ok := annotations[metal3api.InspectAnnotationPrefix]; ok && expect != metal3api.InspectAnnotationValueDisabled {
@@ -974,15 +967,15 @@ func updateRootDeviceHints(host *metal3api.BareMetalHost, info *reconcileInfo) (
 func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("inspecting hardware")
 
-	if inspectionDisabled(info.host) {
-		info.log.Info("inspection disabled by annotation")
-		info.publishEvent("InspectionSkipped", "disabled by annotation")
+	if info.host.InspectionDisabled() {
+		info.log.Info("inspection disabled by user")
+		info.publishEvent("InspectionSkipped", "disabled by user")
 		return actionComplete{}
 	}
 
 	info.log.Info("inspecting hardware")
 
-	refresh := hasInspectAnnotation(info.host)
+	refresh := inspectionRefreshRequested(info.host)
 	forceReboot, _ := hasRebootAnnotation(info, true)
 
 	provResult, started, details, err := prov.InspectHardware(
@@ -1004,7 +997,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 		dirty := false
 
 		// Delete inspect annotation if exists
-		if hasInspectAnnotation(info.host) {
+		if inspectionRefreshRequested(info.host) {
 			delete(info.host.Annotations, metal3api.InspectAnnotationPrefix)
 			dirty = true
 		}

internal/controller/metal3.io/baremetalhost_controller_test.go

@@ -592,16 +592,6 @@ func TestSetLastUpdated(t *testing.T) {
 	)
 }
 
-func TestInspectionDisabledAnnotation(t *testing.T) {
-	host := newDefaultHost(t)
-	host.Annotations = make(map[string]string)
-
-	assert.False(t, inspectionDisabled(host))
-
-	host.Annotations[metal3api.InspectAnnotationPrefix] = "disabled"
-	assert.True(t, inspectionDisabled(host))
-}
-
 func makeReconcileInfo(host *metal3api.BareMetalHost) *reconcileInfo {
 	return &reconcileInfo{
 		log:  logf.Log.WithName("controllers").WithName("BareMetalHost").WithName("baremetal_controller"),

internal/controller/metal3.io/host_state_machine.go

@@ -408,7 +408,7 @@ func (hsm *hostStateMachine) handleRegistering(_ *reconcileInfo) actionResult {
 	// if the credentials change and the Host must be re-registered.
 	if hsm.Host.Spec.ExternallyProvisioned {
 		hsm.NextState = metal3api.StateExternallyProvisioned
-	} else if inspectionDisabled(hsm.Host) {
+	} else if hsm.Host.InspectionDisabled() {
 		hsm.NextState = metal3api.StatePreparing
 	} else {
 		hsm.NextState = metal3api.StateInspecting
@@ -439,8 +439,7 @@ func (hsm *hostStateMachine) handleExternallyProvisioned(info *reconcileInfo) ac
 		return hsm.Reconciler.actionManageSteadyState(hsm.Provisioner, info)
 	}
 
-	// TODO(dtantsur): move this logic inside NeedsHardwareInspection?
-	if hsm.Host.NeedsHardwareInspection() && !inspectionDisabled(hsm.Host) {
+	if hsm.Host.NeedsHardwareInspection() {
 		hsm.NextState = metal3api.StateInspecting
 	} else {
 		hsm.NextState = metal3api.StatePreparing
@@ -464,7 +463,7 @@ func (hsm *hostStateMachine) handleAvailable(info *reconcileInfo) actionResult {
 		return actionComplete{}
 	}
 
-	if hasInspectAnnotation(hsm.Host) {
+	if inspectionRefreshRequested(hsm.Host) {
 		hsm.NextState = metal3api.StateInspecting
 		return actionComplete{}
 	}

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

@@ -82,6 +82,10 @@ func (webhook *BareMetalHost) validateHost(host *metal3api.BareMetalHost) []erro
 		errs = append(errs, annotationErrors...)
 	}
 
+	if err := validateInspectionMode(host); err != nil {
+		errs = append(errs, err)
+	}
+
 	if err := validatePowerStatus(host); err != nil {
 		errs = append(errs, err)
 	}
@@ -219,8 +223,7 @@ func validateAnnotations(host *metal3api.BareMetalHost) []error {
 		case annotation == metal3api.InspectAnnotationPrefix:
 			err = validateInspectAnnotation(value)
 		case annotation == metal3api.HardwareDetailsAnnotation:
-			inspect := host.Annotations[metal3api.InspectAnnotationPrefix]
-			err = validateHwdDetailsAnnotation(value, inspect)
+			err = validateHwdDetailsAnnotation(value, host.InspectionDisabled())
 		default:
 			err = nil
 		}
@@ -298,13 +301,14 @@ func checkStatusAnnotation(bmhStatus *metal3api.BareMetalHostStatus) error {
 	return nil
 }
 
-func validateHwdDetailsAnnotation(hwdDetAnnotation string, inspect string) error {
+func validateHwdDetailsAnnotation(hwdDetAnnotation string, inspectionDisabled bool) error {
 	if hwdDetAnnotation == "" {
 		return nil
 	}
 
-	if inspect != "disabled" {
-		return errors.New("when hardware details are provided, the inspect.metal3.io annotation must be set to disabled")
+	// Check both the annotation and the new field for disabled inspection
+	if !inspectionDisabled {
+		return errors.New("when hardware details are provided, inspection must be disabled (either via inspect.metal3.io annotation or inspectionMode field)")
 	}
 
 	objHwdDet := &metal3api.HardwareDetails{}
@@ -380,6 +384,31 @@ func (webhook *BareMetalHost) validateCrossNamespaceSecretReferences(host *metal
 	return errs
 }
 
+func validateInspectionMode(host *metal3api.BareMetalHost) error {
+	inspectAnnotation := host.Annotations[metal3api.InspectAnnotationPrefix]
+
+	// Check for contradicting values when both are set
+	if inspectAnnotation != "" && host.Spec.InspectionMode != "" {
+		// Annotation logic: "disabled" means disabled, anything else means enabled
+		annotationDisabled := inspectAnnotation == metal3api.InspectAnnotationValueDisabled
+		fieldDisabled := host.Spec.InspectionMode == metal3api.InspectionModeDisabled
+
+		if annotationDisabled != fieldDisabled {
+			return errors.New("inspect.metal3.io annotation and inspectionMode field have contradicting values")
+		}
+	}
+
+	// If InspectionMode is set, it must be a valid value (kubebuilder validation handles this for the API,
+	// but we validate here for safety)
+	if host.Spec.InspectionMode != "" &&
+		host.Spec.InspectionMode != metal3api.InspectionModeDisabled &&
+		host.Spec.InspectionMode != metal3api.InspectionModeAgent {
+		return fmt.Errorf("invalid inspectionMode value: %s, allowed values are 'disabled' or 'agent'", host.Spec.InspectionMode)
+	}
+
+	return nil
+}
+
 func validatePowerStatus(host *metal3api.BareMetalHost) error {
 	if host.Spec.DisablePowerOff && !host.Spec.Online {
 		return errors.New("node can't simultaneously have online set to false and have power off disabled")