fix: upgrade workspace status condition correctly

[rambohe-ch]

Reason for Change:

fix some bugs according to E2E failure: https://github.com/kaito-project/kaito/actions/runs/21513080521/job/61984690966?pr=1747#step:16:5812

1. error logs:

• background:
  ResourcesReady is set true initially when neither of ConditionTypeNodeClaimStatus and ConditionTypeNodeStatus exist. the logs as follows:

• solution:
  skip set ConditionTypeResourceStatus condition when owned conditions don't exist.

2. error logs:

• background
  Related services are created just after NodesReady condition is true. and then ResourcesReady will never be set to true, because workload creation will block the reconciliation.

so E2E test will timeout for resource status checking.

• solution:
  go forward to workload reconcilation before ConditionTypeResourceStatus condition is true

Requirements

• added unit tests and e2e tests (if applicable).

Issue Fixed:

Notes for Reviewers:

[kaito-pr-agent]

Title

Fix workspace status conditions and node readiness verification

---

Description

• Fix resource status condition handling in workspace controller

• Improve node readiness verification with instance type checks

• Refactor resource condition propagation logic

• Update tests for new condition handling and node validation

---

Changes walkthrough 📝

	Relevant files
Bug fix	workspace_controller.go Improve workload reconciliation flow                                          pkg/workspace/controllers/workspace_controller.go Added logic to proceed with workload reconciliation only after resource condition is True Implemented requeue mechanism when node resources aren't ready Added resource readiness logging +16/-10
Enhancement	node.go Enhance node validation and condition propagation                pkg/workspace/resource/node.go Fixed node readiness check to return immediately on instance type mismatch Refactored resource condition logic to handle node/node claim statuses Added detailed condition propagation with specific failure reasons +35/-26
Tests	node_test.go Update tests for node validation and condition handling    pkg/workspace/resource/node_test.go Added instance type labels to test node objects Updated test expectations for node validation failures Replaced condition propagation tests with new resource condition tests Added comprehensive test cases for various condition scenarios +83/-211

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[kaito-pr-agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Requeue Logic Verify that the 2-second requeue interval when resource status isn't ready doesn't conflict with other reconciliation timing requirements. // The node resource changes can not trigger workspace controller reconcile, so we need to requeue reconcile when don't proceed because of node resource not ready. return &reconcile.Result{RequeueAfter: 2 * time.Second}, nil } // we can go forward to workload reconciliation before ConditionTypeResourceStatus is set to true resourceCondition := meta.FindStatusCondition(wObj.Status.Conditions, string(kaitov1beta1.ConditionTypeResourceStatus)) if resourceCondition == nil || resourceCondition.Status != metav1.ConditionTrue { return &reconcile.Result{RequeueAfter: 2 * time.Second}, nil } Condition Propagation Validate that the new resource status condition logic correctly handles cases where only one of NodeClaimStatus or NodeStatus conditions exists. func (c *NodeManager) SetResourceReadyConditionByStatus(ctx context.Context, wObj *kaitov1beta1.Workspace) error { considerNodeClaim := !featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] nodeClaimCondition := meta.FindStatusCondition(wObj.Status.Conditions, string(kaitov1beta1.ConditionTypeNodeClaimStatus)) nodeCondition := meta.FindStatusCondition(wObj.Status.Conditions, string(kaitov1beta1.ConditionTypeNodeStatus)) if !considerNodeClaim { nodeClaimCondition = nil } nodeClaimExists := nodeClaimCondition != nil nodeExists := nodeCondition != nil // If both conditions don't exist, skip setting the resource condition. if !nodeClaimExists && !nodeExists { return nil } // Only when all considered conditions exist and are true, set the resource condition to true. if nodeExists && nodeCondition.Status == metav1.ConditionTrue && (!nodeClaimExists || nodeClaimCondition.Status == metav1.ConditionTrue) { if err := workspace.UpdateStatusConditionIfNotMatch(ctx, c.Client, wObj, kaitov1beta1.ConditionTypeResourceStatus, metav1.ConditionTrue, "workspaceResourceStatusSuccess", "workspace resource is ready"); err != nil { klog.ErrorS(err, "failed to update workspace status", "workspace", klog.KObj(wObj)) return err } return nil } // Otherwise set the resource condition to false. reason := "workspaceResourceStatusNotReady" message := "node claim or node status condition not ready" if nodeClaimExists && nodeClaimCondition.Status != metav1.ConditionTrue { reason = nodeClaimCondition.Reason message = nodeClaimCondition.Message } else if nodeExists && nodeCondition.Status != metav1.ConditionTrue { reason = nodeCondition.Reason message = nodeCondition.Message } if err := workspace.UpdateStatusConditionIfNotMatch(ctx, c.Client, wObj, kaitov1beta1.ConditionTypeResourceStatus, metav1.ConditionFalse, reason, message); err != nil { klog.ErrorS(err, "failed to update workspace status", "workspace", klog.KObj(wObj)) return err } return nil } Test Coverage Ensure that the modified test cases (e.g., NAP enabled scenarios) adequately cover the new condition handling logic and failure modes. // Mock status update calls - fail the condition update mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("status update failed")).Once() // Mock successful worker nodes status update mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe().Maybe() }, expectedReady: false, expectedError: true, }, { name: "NAP enabled - Should return false when node missing instance type label", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "workload": "test", }, }, }, Status: kaitov1beta1.WorkspaceStatus{ TargetNodeCount: 1, }, }, nodes: []*corev1.Node{ { ObjectMeta: metav1.ObjectMeta{ Name: "node-1", Labels: map[string]string{ "workload": "test", // Missing instance type label - logged but doesn't fail }, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, }, }, }, }, nodeClaims: []*karpenterv1.NodeClaim{}, setup: func(mockClient *test.MockClient) { // Mock status update calls - multiple updates expected (false then true) mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: false, // NAP enabled expectedReady: false, expectedError: false, }, { name: "NAP enabled - Should return false when node has mismatched instance type label", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "workload": "test", }, }, }, Status: kaitov1beta1.WorkspaceStatus{ TargetNodeCount: 1, }, }, nodes: []*corev1.Node{ { ObjectMeta: metav1.ObjectMeta{ Name: "node-1", Labels: map[string]string{ "workload": "test", corev1.LabelInstanceTypeStable: "Standard_NC6s_v3", // Wrong instance type - logged but doesn't fail }, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, }, }, }, }, nodeClaims: []*karpenterv1.NodeClaim{}, setup: func(mockClient *test.MockClient) { // Mock status update calls - multiple updates expected (false then true) mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: false, // NAP enabled expectedReady: false, expectedError: false, }, { name: "NAP enabled - Should succeed when node has correct instance type label", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "workload": "test", }, }, }, Status: kaitov1beta1.WorkspaceStatus{ TargetNodeCount: 1, }, }, nodes: []*corev1.Node{ { ObjectMeta: metav1.ObjectMeta{ Name: "node-1", Labels: map[string]string{ "workload": "test", corev1.LabelInstanceTypeStable: "Standard_NC12s_v3", // Correct instance type }, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, }, }, }, }, nodeClaims: []*karpenterv1.NodeClaim{}, setup: func(mockClient *test.MockClient) { // Mock status update calls for success condition mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: false, // NAP enabled expectedReady: true, expectedError: false, }, { name: "NAP disabled - Should succeed even when node missing instance type label", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "workload": "test", }, }, }, Status: kaitov1beta1.WorkspaceStatus{ TargetNodeCount: 1, }, }, nodes: []*corev1.Node{ { ObjectMeta: metav1.ObjectMeta{ Name: "node-1", Labels: map[string]string{ "workload": "test", // Missing instance type label - should be OK when NAP is disabled }, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, }, }, }, }, nodeClaims: []*karpenterv1.NodeClaim{}, setup: func(mockClient *test.MockClient) { // Mock status update calls for success condition mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: true, // NAP disabled expectedReady: true, expectedError: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Setup feature gate originalValue := featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] = tt.disableNodeAutoProvisioning defer func() { featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] = originalValue }() mockClient := test.NewClient() tt.setup(mockClient) // Default to empty slices if not specified nodes := tt.nodes if nodes == nil { nodes = []*corev1.Node{} } nodeClaims := tt.nodeClaims if nodeClaims == nil { nodeClaims = []*karpenterv1.NodeClaim{} } manager := NewNodeManager(mockClient) ready, err := manager.EnsureNodesReady(context.Background(), tt.workspace, nodes, nodeClaims) assert.Equal(t, tt.expectedReady, ready) if tt.expectedError { assert.Error(t, err) } else { assert.NoError(t, err) } }) } } func TestGetReadyNodesFromNodeClaims(t *testing.T) { tests := []struct { name string workspace *kaitov1beta1.Workspace readyNodeClaims []*karpenterv1.NodeClaim setup func(*test.MockClient) expectedNodes int expectedError bool }{ { name: "Should return error when NodeClaim without assigned node", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{}, }, }, readyNodeClaims: []*karpenterv1.NodeClaim{ { ObjectMeta: metav1.ObjectMeta{Name: "test-nodeclaim"}, Status: karpenterv1.NodeClaimStatus{ NodeName: "", // No node assigned }, }, }, setup: func(mockClient *test.MockClient) { // No setup needed - will skip NodeClaim without NodeName }, expectedNodes: 0, expectedError: false, }, { name: "Should return error when fail to get node", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{}, }, }, readyNodeClaims: []*karpenterv1.NodeClaim{ { ObjectMeta: metav1.ObjectMeta{Name: "test-nodeclaim"}, Status: karpenterv1.NodeClaimStatus{ NodeName: "nonexistent-node", }, }, }, setup: func(mockClient *test.MockClient) { // Mock Get to return NotFound error mockClient.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Resource: "nodes"}, "nonexistent-node")) }, expectedNodes: 0, expectedError: true, }, { name: "Should return error when node is not ready", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{}, }, }, readyNodeClaims: []*karpenterv1.NodeClaim{ { ObjectMeta: metav1.ObjectMeta{Name: "test-nodeclaim"}, Status: karpenterv1.NodeClaimStatus{ NodeName: "test-node", }, }, }, setup: func(mockClient *test.MockClient) { // Create a node that is not ready node := &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: "test-node", }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionFalse, }, }, }, } mockClient.CreateOrUpdateObjectInMap(node) mockClient.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, expectedNodes: 0, expectedError: false, }, { name: "Should return ready node with matching instance type", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Resource: kaitov1beta1.ResourceSpec{ InstanceType: "Standard_NC12s_v3", LabelSelector: &metav1.LabelSelector{}, }, }, readyNodeClaims: []*karpenterv1.NodeClaim{ { ObjectMeta: metav1.ObjectMeta{Name: "test-nodeclaim"}, Status: karpenterv1.NodeClaimStatus{ NodeName: "test-node", }, }, }, setup: func(mockClient *test.MockClient) { // Create a ready node with matching instance type node := &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: "test-node", Labels: map[string]string{ corev1.LabelInstanceTypeStable: "Standard_NC12s_v3", }, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ { Type: corev1.NodeReady, Status: corev1.ConditionTrue, }, }, }, } mockClient.CreateOrUpdateObjectInMap(node) mockClient.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, expectedNodes: 1, expectedError: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockClient := test.NewClient() tt.setup(mockClient) manager := NewNodeManager(mockClient) nodes, err := manager.getReadyNodesFromNodeClaims(context.Background(), tt.workspace, tt.readyNodeClaims) assert.Len(t, nodes, tt.expectedNodes) if tt.expectedError { assert.Error(t, err) } else { assert.NoError(t, err) } }) } } func TestSetResourceReadyCondition(t *testing.T) { tests := []struct { name string workspace *kaitov1beta1.Workspace setup func(*test.MockClient) disableNodeAutoProvisioning bool expectedError bool expectStatusUpdate bool }{ { name: "Should set resource condition to true when all owned conditions are true", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeClaimStatus), Status: metav1.ConditionTrue, }, { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionTrue, }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe().Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe().Maybe() }, disableNodeAutoProvisioning: false, expectedError: false, expectStatusUpdate: true, }, { name: "Should set resource condition to false when NodeClaim condition is false", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeClaimStatus), Status: metav1.ConditionFalse, Reason: "NodeClaimNotReady", Message: "Not enough NodeClaims are ready", }, { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionTrue, }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe().Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe().Maybe() }, disableNodeAutoProvisioning: false, expectedError: false, expectStatusUpdate: true, }, { name: "Should set resource condition to false when Node condition is false", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeClaimStatus), Status: metav1.ConditionTrue, }, { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionFalse, Reason: "NodeNotReady", Message: "Not enough nodes are ready", }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe().Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe().Maybe() }, disableNodeAutoProvisioning: false, expectedError: false, expectStatusUpdate: true, }, { name: "Should set resource condition to false when NodeClaim condition is missing", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionTrue, }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe().Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe().Maybe() }, disableNodeAutoProvisioning: false, expectedError: false, expectStatusUpdate: true, }, { name: "Should skip update when both conditions are missing", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{}, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: false, expectedError: false, expectStatusUpdate: false, }, { name: "Should return error when setting false status update fails", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionFalse, Reason: "NodeNotReady", Message: "Not enough nodes are ready", }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("status update failed")) }, disableNodeAutoProvisioning: false, expectedError: true, expectStatusUpdate: true, }, { name: "Should return error when setting true status update fails", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeClaimStatus), Status: metav1.ConditionTrue, }, { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionTrue, }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("final status update failed")) }, disableNodeAutoProvisioning: false, expectedError: true, expectStatusUpdate: true, }, { name: "NAP disabled - Should set resource condition to true when Node condition is true and NodeClaim is ignored", workspace: &kaitov1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{Name: "test-workspace", Namespace: "default"}, Status: kaitov1beta1.WorkspaceStatus{ Conditions: []metav1.Condition{ { Type: string(kaitov1beta1.ConditionTypeNodeStatus), Status: metav1.ConditionTrue, }, }, }, }, setup: func(mockClient *test.MockClient) { mockClient.On("Get", mock.Anything, mock.Anything, mock.IsType(&kaitov1beta1.Workspace{}), mock.Anything).Return(nil).Maybe() mockClient.StatusMock.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() }, disableNodeAutoProvisioning: true, expectedError: false, expectStatusUpdate: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { originalValue := featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] = tt.disableNodeAutoProvisioning defer func() { featuregates.FeatureGates[consts.FeatureFlagDisableNodeAutoProvisioning] = originalValue }() mockClient := test.NewClient() tt.setup(mockClient) manager := NewNodeManager(mockClient) err := manager.SetResourceReadyConditionByStatus(context.Background(), tt.workspace) if tt.expectedError { assert.Error(t, err) } else { assert.NoError(t, err) } if tt.expectStatusUpdate { mockClient.StatusMock.AssertCalled(t, "Update", mock.Anything, mock.Anything, mock.Anything) } else { mockClient.StatusMock.AssertNotCalled(t, "Update", mock.Anything, mock.Anything, mock.Anything) } }) } }

[kaito-pr-agent]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove stale condition check The condition check occurs before the deferred status update runs, which may lead to using stale status data. Since the status update is deferred until after this function exits, the condition being checked might not reflect the latest state. This could cause unnecessary requeues even when resources are ready. Remove this immediate check and rely on the next reconciliation triggered by the status update. pkg/workspace/controllers/workspace_controller.go [200-205] -// we can go forward to workload reconciliation before ConditionTypeResourceStatus is set to true -resourceCondition := meta.FindStatusCondition(wObj.Status.Conditions, string(kaitov1beta1.ConditionTypeResourceStatus)) -if resourceCondition == nil || resourceCondition.Status != metav1.ConditionTrue { - return &reconcile.Result{RequeueAfter: 2 * time.Second}, nil -} -klog.InfoS("resources for workspace are ready", "workspace", klog.KObj(wObj)) +// Deferred status update will trigger requeue when ready Suggestion importance[1-10]: 8 __ Why: The condition check uses stale status data since the status update is deferred, which could cause unnecessary requeues. Removing it improves correctness by relying on status-triggered reconciliation.	Medium