address review

kcp-dev · Nov 23, 2024 · 7a42025 · 7a42025
1 parent 8c40ccb
commit 7a42025
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 23 deletions.
diff --git a/pkg/reconciler/tenancy/workspace/workspace_reconcile_phase.go b/pkg/reconciler/tenancy/workspace/workspace_reconcile_phase.go
@@ -77,7 +77,7 @@ func (r *phaseReconciler) reconcile(ctx context.Context, workspace *tenancyv1alp
 		conditions.MarkTrue(workspace, tenancyv1alpha1.WorkspaceInitialized)
 
 	case corev1alpha1.LogicalClusterPhaseUnavailable:
-		if updateTerminalConditionsAndPhase(workspace) {
+		if terminalConditionPhase(workspace) {
 			return reconcileStatusStopAndRequeue, nil
 		}
 		return reconcileStatusContinue, nil
@@ -118,7 +118,7 @@ func (r *phaseReconciler) reconcile(ctx context.Context, workspace *tenancyv1alp
 		}
 
 		// if workspace is ready, we check if it suppose to be ready by checking conditions.
-		if updateTerminalConditionsAndPhase(workspace) {
+		if terminalConditionPhase(workspace) {
 			logger.Info("workspace phase changed", "status", workspace.Status)
 			return reconcileStatusStopAndRequeue, nil
 		}
@@ -127,9 +127,9 @@ func (r *phaseReconciler) reconcile(ctx context.Context, workspace *tenancyv1alp
 	return reconcileStatusContinue, nil
 }
 
-// updateTerminalConditionsAndPhase checks if the workspace is ready by checking conditions and sets the phase accordingly.
+// terminalConditionPhase checks if the workspace is ready by checking conditions and sets the phase accordingly.
 // It returns true if the phase was changed, false otherwise.
-func updateTerminalConditionsAndPhase(workspace *tenancyv1alpha1.Workspace) bool {
+func terminalConditionPhase(workspace *tenancyv1alpha1.Workspace) bool {
 	var notReady bool
 	for _, c := range workspace.Status.Conditions {
 		if c.Status == v1.ConditionFalse && strings.HasPrefix(string(c.Type), "Workspace") {

diff --git a/pkg/reconciler/tenancy/workspacemounts/workspacemounts_reconcile_updater.go b/pkg/reconciler/tenancy/workspacemounts/workspacemounts_reconcile_updater.go
@@ -40,19 +40,17 @@ type workspaceStatusUpdater struct {
 
 func (r *workspaceStatusUpdater) reconcile(ctx context.Context, workspace *tenancyv1alpha1.Workspace) (reconcileStatus, error) {
 	var mount *tenancyv1alpha1.Mount
-	if workspace.Annotations != nil {
-		if v, ok := workspace.Annotations[tenancyv1alpha1.ExperimentalWorkspaceMountAnnotationKey]; ok {
-			var err error
-			mount, err = tenancyv1alpha1.ParseTenancyMountAnnotation(v)
-			if err != nil {
-				return reconcileStatusStopAndRequeue, err
-			}
-		} else {
-			// no mount annotation, might be nothing or mount was soft "deleted" by removing the annotation
-			// Delete the condition
-			conditions.Delete(workspace, tenancyv1alpha1.MountConditionReady)
-			return reconcileStatusContinue, nil
+	if v, ok := workspace.Annotations[tenancyv1alpha1.ExperimentalWorkspaceMountAnnotationKey]; ok {
+		var err error
+		mount, err = tenancyv1alpha1.ParseTenancyMountAnnotation(v)
+		if err != nil {
+			return reconcileStatusStopAndRequeue, err
 		}
+	} else {
+		// no mount annotation, might be nothing or mount was soft "deleted" by removing the annotation
+		// Delete the condition
+		conditions.Delete(workspace, tenancyv1alpha1.MountConditionReady)
+		return reconcileStatusContinue, nil
 	}
 
 	if mount == nil {
@@ -108,11 +106,10 @@ func (r *workspaceStatusUpdater) reconcile(ctx context.Context, workspace *tenan
 			if current.Status == v1.ConditionTrue {
 				return reconcileStatusContinue, nil
 			}
-			conditions.MarkTrue(workspace, tenancyv1alpha1.MountConditionReady)
 			return reconcileStatusContinue, nil
 		default:
-			msg := fmt.Sprintf("Mount is not reporting ready. See %s %s status for more details", obj.GroupVersionKind().Kind, obj.GetName())
-			conditions.MarkFalse(workspace, tenancyv1alpha1.MountConditionReady, tenancyv1alpha1.MountReasonNotReady, conditionsv1alpha1.ConditionSeverityError, msg)
+			msg := fmt.Sprintf("Mount is not reporting ready. See %s %q status for more details", obj.GroupVersionKind().Kind, obj.GetName())
+			conditions.MarkFalse(workspace, tenancyv1alpha1.MountConditionReady, "Mount is not reporting ready.", conditionsv1alpha1.ConditionSeverityError, msg)
 		}
 	}
 

diff --git a/sdk/apis/core/v1alpha1/logicalcluster_types.go b/sdk/apis/core/v1alpha1/logicalcluster_types.go
@@ -66,9 +66,11 @@ const (
 	LogicalClusterPhaseScheduling   LogicalClusterPhaseType = "Scheduling"
 	LogicalClusterPhaseInitializing LogicalClusterPhaseType = "Initializing"
 	LogicalClusterPhaseReady        LogicalClusterPhaseType = "Ready"
-	// LogicalClusterPhaseUnavailable phase is used to indicate that the logical cluster us unavailable to be used.
+	// LogicalClusterPhaseUnavailable phase is used to indicate that the logical cluster is unavailable to be used.
 	// It will will not be served via front-proxy when in this state.
 	// Possible state transitions are from Ready to Unavailable and from Unavailable to Ready.
+	// This should be used when we really can't serve the logical cluster content and not some
+	// temporary flakes, like readiness probe failing.
 	LogicalClusterPhaseUnavailable LogicalClusterPhaseType = "Unavailable"
 )
 

diff --git a/sdk/apis/tenancy/v1alpha1/types_mounts.go b/sdk/apis/tenancy/v1alpha1/types_mounts.go
@@ -44,9 +44,6 @@ const (
 const (
 	// MountConditionReady is the condition type for MountReady.
 	MountConditionReady conditionsv1alpha1.ConditionType = "WorkspaceMountReady"
-
-	// MountNotReady is reason when the mount is not ready.
-	MountReasonNotReady = "WorkspaceMountNotReady"
 )
 
 // Mount is a workspace mount that can be used to mount a workspace into another workspace or resource.