Address code review feedback

- Consolidate hardware operation timestamps into single
  HardwareOperationStartTime field
- Simplify ObservedConfigTransactionId type (*int64 -> int64)
- Improve error handling with errors.Join and add elapsed duration to logs
- Add test coverage for edge cases and race conditions

Signed-off-by: Tao Liu <[email protected]>

Author: tliu2021

api/hardwaremanagement/plugins/v1alpha1/node_allocation_requests.go

@@ -134,15 +134,10 @@ type NodeAllocationRequestStatus struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=status
 	SelectedGroups map[string]string `json:"selectedGroups,omitempty"`
 
-	// ProvisioningStartTime tracks when hardware provisioning actually started.
-	// This timestamp is used for timeout calculations for Day 0 provisioning.
+	// HardwareOperationStartTime tracks when the current hardware operation (provisioning or configuration) actually started.
+	// This timestamp is used for timeout calculations. The active operation is determined from the conditions.
 	//+operator-sdk:csv:customresourcedefinitions:type=status
-	ProvisioningStartTime *metav1.Time `json:"provisioningStartTime,omitempty"`
-
-	// ConfiguringStartTime tracks when hardware configuration actually started.
-	// This timestamp is used for timeout calculations for Day 2 configuration changes.
-	//+operator-sdk:csv:customresourcedefinitions:type=status
-	ConfiguringStartTime *metav1.Time `json:"configuringStartTime,omitempty"`
+	HardwareOperationStartTime *metav1.Time `json:"hardwareOperationStartTime,omitempty"`
 }
 
 // NodeAllocationRequest is the schema for an allocation request of nodes

api/hardwaremanagement/plugins/v1alpha1/zz_generated.deepcopy.go

@@ -56,6 +56,12 @@ type NodeAllocationRequestRef struct {
 	HardwareProvisioningCheckStart *metav1.Time `json:"hardwareProvisioningCheckStart,omitempty"`
 	// Represents the timestamp of the first status check for hardware configuring
 	HardwareConfiguringCheckStart *metav1.Time `json:"hardwareConfiguringCheckStart,omitempty"`
+	// ObservedConfigTransactionId tracks the ConfigTransactionId that has been observed
+	// by the hardware plugin. This helps with debugging by showing which configuration
+	// transaction the plugin has processed.
+	// A value of 0 indicates the transaction has not been observed yet. Since Kubernetes
+	// Generation starts at 1, 0 serves as a natural sentinel value for "not observed".
+	ObservedConfigTransactionId int64 `json:"observedConfigTransactionId,omitempty"`
 }
 
 type ClusterDetails struct {

api/provisioning/v1alpha1/provisioningrequest_types.go

@@ -60,16 +60,21 @@ var _ = Describe("ProvisioningRequestValidator", func() {
 	})
 
 	Describe("ValidateUpdate", func() {
+		const (
+			testClusterTemplateB = "clustertemplate-b"
+			testVersionB         = "v1.0.2"
+		)
+
 		BeforeEach(func() {
 			// Create a new ClusterTemplate
 			newCt := &ClusterTemplate{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "clustertemplate-b.v1.0.2",
+					Name:      testClusterTemplateB + "." + testVersionB,
 					Namespace: "default",
 				},
 				Spec: ClusterTemplateSpec{
-					Name:       "clustertemplate-b",
-					Version:    "v1.0.2",
+					Name:       testClusterTemplateB,
+					Version:    testVersionB,
 					TemplateID: "57b39bda-ac56-4143-9b10-d1a71517d04f",
 					Templates: Templates{
 						ClusterInstanceDefaults: "clusterinstance-defaults-v1",
@@ -92,8 +97,8 @@ var _ = Describe("ProvisioningRequestValidator", func() {
 
 		Context("when spec.templateName or spec.templateVersion is changed", func() {
 			BeforeEach(func() {
-				newPr.Spec.TemplateName = "clustertemplate-b"
-				newPr.Spec.TemplateVersion = "v1.0.2"
+				newPr.Spec.TemplateName = testClusterTemplateB
+				newPr.Spec.TemplateVersion = testVersionB
 			})
 
 			It("should allow the change when the ProvisioningRequest is fulfilled", func() {
@@ -680,5 +685,133 @@ var _ = Describe("ProvisioningRequestValidator", func() {
 				})
 			})
 		})
+
+		Context("When HardwareProvisioned condition is TimedOut or Failed", func() {
+			BeforeEach(func() {
+				// Create ClusterTemplate for validation
+				ct := &ClusterTemplate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "clustertemplate-a.v1.0.1",
+						Namespace: "default",
+					},
+					Spec: ClusterTemplateSpec{
+						Name:       "clustertemplate-a",
+						Version:    "v1.0.1",
+						TemplateID: "test-template-id",
+						Templates: Templates{
+							ClusterInstanceDefaults: "defaults-v1",
+							PolicyTemplateDefaults:  "policy-defaults-v1",
+						},
+						TemplateParameterSchema: runtime.RawExtension{Raw: []byte(testTemplate)},
+					},
+					Status: ClusterTemplateStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:   string(CTconditionTypes.Validated),
+								Status: metav1.ConditionTrue,
+							},
+						},
+					},
+				}
+				Expect(fakeClient.Create(ctx, ct)).To(Succeed())
+
+				// Create ClusterTemplate-b that will be used by tests
+				newCt := &ClusterTemplate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      testClusterTemplateB + "." + testVersionB,
+						Namespace: "default",
+					},
+					Spec: ClusterTemplateSpec{
+						Name:       testClusterTemplateB,
+						Version:    testVersionB,
+						TemplateID: "test-template-id-b",
+						Templates: Templates{
+							ClusterInstanceDefaults: "defaults-v1",
+							PolicyTemplateDefaults:  "policy-defaults-v1",
+						},
+						TemplateParameterSchema: runtime.RawExtension{Raw: []byte(testTemplate)},
+					},
+					Status: ClusterTemplateStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:   string(CTconditionTypes.Validated),
+								Status: metav1.ConditionTrue,
+							},
+						},
+					},
+				}
+				// Create only if it doesn't exist (to avoid conflicts between tests)
+				existing := &ClusterTemplate{}
+				if err := fakeClient.Get(ctx, client.ObjectKeyFromObject(newCt), existing); err != nil {
+					Expect(fakeClient.Create(ctx, newCt)).To(Succeed())
+				}
+
+				// Set HardwareProvisioned condition to TimedOut
+				newPr.Status.Conditions = []metav1.Condition{
+					{
+						Type:   string(PRconditionTypes.HardwareProvisioned),
+						Status: metav1.ConditionFalse,
+						Reason: string(CRconditionReasons.TimedOut),
+					},
+				}
+			})
+
+			It("should reject spec changes when hardware provisioning has timed out", func() {
+				// clustertemplate-b is already created in BeforeEach
+				// Try to change TemplateName (a hardware provisioning-related field)
+				newPr.Spec.TemplateName = testClusterTemplateB
+				newPr.Spec.TemplateVersion = testVersionB
+
+				_, err := validator.ValidateUpdate(ctx, oldPr, newPr)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("hardware provisioning has timed out or failed"))
+				Expect(err.Error()).To(ContainSubstring("Spec changes are not allowed"))
+				Expect(err.Error()).To(ContainSubstring("delete and recreate"))
+			})
+
+			It("should reject TemplateParameters changes when hardware provisioning has timed out", func() {
+				// Try to change TemplateParameters
+				newPr.Spec.TemplateParameters.Raw = []byte(`{
+					"oCloudSiteId": "local-123",
+					"nodeClusterName": "exampleCluster",
+					"clusterInstanceParameters": {"additionalNTPSources": ["2.2.2.2"]},
+					"policyTemplateParameters": {"sriov-network-vlan-1": "200"}
+				}`)
+
+				_, err := validator.ValidateUpdate(ctx, oldPr, newPr)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("hardware provisioning has timed out or failed"))
+				Expect(err.Error()).To(ContainSubstring("Spec changes are not allowed"))
+			})
+
+			It("should allow non-spec changes when hardware provisioning has timed out", func() {
+				// Only change metadata (like labels/annotations) - should be allowed
+				newPr.Labels = map[string]string{"new-label": "new-value"}
+				newPr.Annotations = map[string]string{"new-annotation": "new-value"}
+
+				_, err := validator.ValidateUpdate(ctx, oldPr, newPr)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("should reject spec changes when hardware provisioning has failed", func() {
+				// Set HardwareProvisioned condition to Failed
+				newPr.Status.Conditions = []metav1.Condition{
+					{
+						Type:   string(PRconditionTypes.HardwareProvisioned),
+						Status: metav1.ConditionFalse,
+						Reason: string(CRconditionReasons.Failed),
+					},
+				}
+
+				// clustertemplate-b is already created in BeforeEach
+				// Try to change TemplateName and TemplateVersion
+				newPr.Spec.TemplateName = testClusterTemplateB
+				newPr.Spec.TemplateVersion = testVersionB
+
+				_, err := validator.ValidateUpdate(ctx, oldPr, newPr)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("hardware provisioning has timed out or failed"))
+			})
+		})
 	})
 })

api/provisioning/v1alpha1/provisioningrequest_webhook_test.go

@@ -197,6 +197,15 @@ spec:
                       nodeAllocationRequestID:
                         description: Contains the identifier of the created NodeAllocationRequest.
                         type: string
+                      observedConfigTransactionId:
+                        description: |-
+                          ObservedConfigTransactionId tracks the ConfigTransactionId that has been observed
+                          by the hardware plugin. This helps with debugging by showing which configuration
+                          transaction the plugin has processed.
+                          A value of 0 indicates the transaction has not been observed yet. Since Kubernetes
+                          Generation starts at 1, 0 serves as a natural sentinel value for "not observed".
+                        format: int64
+                        type: integer
                     type: object
                   policies:
                     description: Holds policies that are matched with the ManagedCluster

bundle/manifests/clcm.openshift.io_provisioningrequests.yaml

@@ -285,10 +285,10 @@ spec:
                   - type
                   type: object
                 type: array
-              configuringStartTime:
+              hardwareOperationStartTime:
                 description: |-
-                  ConfiguringStartTime tracks when hardware configuration actually started.
-                  This timestamp is used for timeout calculations for Day 2 configuration changes.
+                  HardwareOperationStartTime tracks when the current hardware operation (provisioning or configuration) actually started.
+                  This timestamp is used for timeout calculations. The active operation is determined from the conditions.
                 format: date-time
                 type: string
               hwMgrPlugin:
@@ -310,12 +310,6 @@ spec:
                       type: string
                     type: array
                 type: object
-              provisioningStartTime:
-                description: |-
-                  ProvisioningStartTime tracks when hardware provisioning actually started.
-                  This timestamp is used for timeout calculations for Day 0 provisioning.
-                format: date-time
-                type: string
               selectedGroups:
                 additionalProperties:
                   type: string

bundle/manifests/oran-o2ims.clusterserviceversion.yaml

@@ -585,22 +585,17 @@ spec:
         displayName: Conditions
         path: conditions
       - description: |-
-          ConfiguringStartTime tracks when hardware configuration actually started.
-          This timestamp is used for timeout calculations for Day 2 configuration changes.
-        displayName: Configuring Start Time
-        path: configuringStartTime
+          HardwareOperationStartTime tracks when the current hardware operation (provisioning or configuration) actually started.
+          This timestamp is used for timeout calculations. The active operation is determined from the conditions.
+        displayName: Hardware Operation Start Time
+        path: hardwareOperationStartTime
       - displayName: Hw Mgr Plugin
         path: hwMgrPlugin
       - displayName: Observed Config Transaction Id
         path: observedConfigTransactionId
       - description: Properties represent the node properties in the pool
         displayName: Properties
         path: properties
-      - description: |-
-          ProvisioningStartTime tracks when hardware provisioning actually started.
-          This timestamp is used for timeout calculations for Day 0 provisioning.
-        displayName: Provisioning Start Time
-        path: provisioningStartTime
       - displayName: Selected Groups
         path: selectedGroups
       version: v1alpha1