Merge pull request #809 from jlandowner/robust-reconcililation2

Improve reconciliation

hack/local-run-test/Makefile

@@ -363,7 +363,7 @@ run-manager-local: go
 		--metrics-bind-address :8085 \
 		--cert-dir . \
 		--zap-log-level=$(LOGLEVEL) \
-		--zap-devel=false \
+		--zap-devel=true \
 		--workspace-urlbase-protocol=https \
 		--workspace-urlbase-host=$(DEFAULT_URLBASE_HOST) \
 		--workspace-urlbase-domain=$(DOMAIN)

internal/controllers/__snapshots__/user_controller_test.snap

@@ -34,14 +34,14 @@ SnapShot = """
     },
     \"addons\": [
       {
-        \"kind\": \"Instance\",
-        \"namespace\": \"cosmo-user-ua\",
-        \"name\": \"useraddon-namespaced-addon\",
+        \"kind\": \"ClusterInstance\",
+        \"name\": \"useraddon-ua-cluster-addon\",
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\"
       },
       {
-        \"kind\": \"ClusterInstance\",
-        \"name\": \"useraddon-ua-cluster-addon\",
+        \"kind\": \"Instance\",
+        \"namespace\": \"cosmo-user-ua\",
+        \"name\": \"useraddon-namespaced-addon\",
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\"
       }
     ]
@@ -84,14 +84,14 @@ SnapShot = """
     },
     \"addons\": [
       {
-        \"kind\": \"Instance\",
-        \"namespace\": \"cosmo-user-ua\",
-        \"name\": \"useraddon-namespaced-addon\",
+        \"kind\": \"ClusterInstance\",
+        \"name\": \"useraddon-ua-cluster-addon\",
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\"
       },
       {
-        \"kind\": \"ClusterInstance\",
-        \"name\": \"useraddon-ua-cluster-addon\",
+        \"kind\": \"Instance\",
+        \"namespace\": \"cosmo-user-ua\",
+        \"name\": \"useraddon-namespaced-addon\",
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\"
       }
     ]

internal/controllers/__snapshots__/workspace_controller_test.snap

@@ -5,6 +5,9 @@ SnapShot = """
     \"name\": \"ws-test\",
     \"namespace\": \"cosmo-user-wsctltest\",
     \"creationTimestamp\": null,
+    \"labels\": {
+      \"cosmo-workspace.github.io/type\": \"workspace\"
+    },
     \"ownerReferences\": [
       {
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\",
@@ -33,15 +36,15 @@ SnapShot = """
         {
           \"target\": {
             \"kind\": \"Service\",
-            \"name\": \"ws-svc\",
+            \"name\": \"ws-test-ws-svc\",
             \"apiVersion\": \"v1\"
           },
           \"patch\": \"[{\\\"op\\\": \\\"replace\\\",\\\"path\\\": \\\"/spec/ports\\\",\\\"value\\\": []}]\"
         },
         {
           \"target\": {
             \"kind\": \"Deployment\",
-            \"name\": \"ws-dep\",
+            \"name\": \"ws-test-ws-dep\",
             \"apiVersion\": \"apps/v1\"
           },
           \"patch\": \"[{\\\"op\\\": \\\"replace\\\",\\\"path\\\": \\\"/spec/replicas\\\",\\\"value\\\": 1}]\"
@@ -91,6 +94,9 @@ SnapShot = """
     \"name\": \"ws-test\",
     \"namespace\": \"cosmo-user-wsctltest\",
     \"creationTimestamp\": null,
+    \"labels\": {
+      \"cosmo-workspace.github.io/type\": \"workspace\"
+    },
     \"ownerReferences\": [
       {
         \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\",
@@ -119,15 +125,15 @@ SnapShot = """
         {
           \"target\": {
             \"kind\": \"Service\",
-            \"name\": \"ws-svc\",
+            \"name\": \"ws-test-ws-svc\",
             \"apiVersion\": \"v1\"
           },
           \"patch\": \"[{\\\"op\\\": \\\"replace\\\",\\\"path\\\": \\\"/spec/ports\\\",\\\"value\\\": [{\\\"name\\\":\\\"port3000\\\",\\\"protocol\\\":\\\"TCP\\\",\\\"port\\\":3000,\\\"targetPort\\\":30000}]}]\"
         },
         {
           \"target\": {
             \"kind\": \"Deployment\",
-            \"name\": \"ws-dep\",
+            \"name\": \"ws-test-ws-dep\",
             \"apiVersion\": \"apps/v1\"
           },
           \"patch\": \"[{\\\"op\\\": \\\"replace\\\",\\\"path\\\": \\\"/spec/replicas\\\",\\\"value\\\": 0}]\"

internal/controllers/cluster_instance_controller.go

@@ -81,7 +81,7 @@ func (r *ClusterInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 			log.Error(err, "failed to update InstanceStatus")
 			return ctrl.Result{}, err
 		}
-		log.Debug().Info("status updated")
+		log.Info("status updated")
 	}
 
 	log.Debug().Info("finish reconcile")

internal/controllers/instance_controller.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"sort"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
@@ -90,7 +91,7 @@ func (r *InstanceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			log.Error(err, "failed to update InstanceStatus")
 			return ctrl.Result{}, err
 		}
-		log.Debug().Info("status updated")
+		log.Info("status updated")
 	}
 
 	log.Debug().Info("finish reconcile")
@@ -197,7 +198,7 @@ func (r *instanceReconciler) reconcileObjects(ctx context.Context, inst cosmov1a
 
 	// garbage collection
 	if len(errs) == 0 && !cosmov1alpha1.IsPruneDisabled(inst) {
-		log.Info("start garbage collection")
+		log.Debug().Info("checking garbage collection")
 		shouldDeletes := objectRefNotExistsInMap(lastApplied, currAppliedMap)
 		for _, d := range shouldDeletes {
 			if skip, err := prune(ctx, r.Client, d); err != nil {
@@ -247,12 +248,25 @@ func sliceToObjectMap(s []cosmov1alpha1.ObjectRef) map[types.UID]cosmov1alpha1.O
 }
 
 func objectRefMapToSlice(m map[types.UID]cosmov1alpha1.ObjectRef) []cosmov1alpha1.ObjectRef {
+	if len(m) == 0 {
+		return nil
+	}
 	s := make([]cosmov1alpha1.ObjectRef, len(m))
 	var i int
 	for _, v := range m {
 		s[i] = v
 		i++
 	}
+	sort.SliceStable(s, func(i, j int) bool {
+		x, y := s[i], s[j]
+		if x.APIVersion != y.APIVersion {
+			return x.APIVersion < y.APIVersion
+		} else if x.Kind != y.Kind {
+			return x.Kind < y.Kind
+		} else {
+			return x.Name < y.Name
+		}
+	})
 	return s
 }
 

internal/controllers/user_controller.go

@@ -19,7 +19,7 @@ import (
 	cosmov1alpha1 "github.com/cosmo-workspace/cosmo/api/v1alpha1"
 	"github.com/cosmo-workspace/cosmo/pkg/auth/password"
 	"github.com/cosmo-workspace/cosmo/pkg/clog"
-	"github.com/cosmo-workspace/cosmo/pkg/kubeutil"
+	"github.com/cosmo-workspace/cosmo/pkg/instance"
 	"github.com/cosmo-workspace/cosmo/pkg/useraddon"
 )
 
@@ -68,11 +68,10 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	user.Status.Namespace = cosmov1alpha1.ObjectRef{
 		ObjectReference: corev1.ObjectReference{
-			APIVersion:      gvk.GroupVersion().String(),
-			Kind:            gvk.Kind,
-			Name:            ns.GetName(),
-			UID:             ns.GetUID(),
-			ResourceVersion: ns.GetResourceVersion(),
+			APIVersion: gvk.GroupVersion().String(),
+			Kind:       gvk.Kind,
+			Name:       ns.GetName(),
+			UID:        ns.GetUID(),
 		},
 		CreationTimestamp: &ns.CreationTimestamp,
 	}
@@ -105,9 +104,18 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 			log.Error(errors.New("instance is nil"), "addon has no Template or ClusterTemplate", "addon", addon)
 			continue
 		}
+		tmpl := useraddon.EmptyTemplateObject(addon)
+		if err := r.Get(ctx, types.NamespacedName{Name: tmpl.GetName()}, tmpl); err != nil {
+			addonErrs = append(addonErrs, fmt.Errorf("failed to create or update addon %s: failed to fetch template: %w", tmpl.GetName(), err))
+			continue
+		}
 
-		op, err := kubeutil.CreateOrUpdate(ctx, r.Client, inst, func() error {
-			return useraddon.PatchUserAddonInstanceAsDesired(inst, addon, user, r.Scheme)
+		op, err := controllerutil.CreateOrUpdate(ctx, r.Client, inst, func() error {
+			if err := useraddon.PatchUserAddonInstanceAsDesired(inst, addon, user, r.Scheme); err != nil {
+				return err
+			}
+			instance.Mutate(inst, tmpl)
+			return nil
 		})
 		if err != nil {
 			addonErrs = append(addonErrs, fmt.Errorf("failed to create or update addon %s :%w", inst.GetSpec().Template.Name, err))
@@ -117,6 +125,8 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		if op != controllerutil.OperationResultNone {
 			log.Info("addon synced", "addon", addon)
 			r.Recorder.Eventf(&user, corev1.EventTypeNormal, "Addon Synced", fmt.Sprintf("addon %s is %s", addon.Template.Name, op))
+		} else {
+			log.Debug().Info("the result of update addon instance operation is None", "addon", addon)
 		}
 
 		ct := inst.GetCreationTimestamp()
@@ -127,12 +137,11 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		}
 		currAddonsMap[inst.GetUID()] = cosmov1alpha1.ObjectRef{
 			ObjectReference: corev1.ObjectReference{
-				APIVersion:      gvk.GroupVersion().String(),
-				Kind:            gvk.Kind,
-				Name:            inst.GetName(),
-				Namespace:       inst.GetNamespace(),
-				UID:             inst.GetUID(),
-				ResourceVersion: inst.GetResourceVersion(),
+				APIVersion: gvk.GroupVersion().String(),
+				Kind:       gvk.Kind,
+				Name:       inst.GetName(),
+				Namespace:  inst.GetNamespace(),
+				UID:        inst.GetUID(),
 			},
 			CreationTimestamp: &ct,
 		}
@@ -145,19 +154,20 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 			log.Error(e, "failed to create or update user addon")
 		}
 		user.Status.Phase = "AddonFailed"
+		err = addonErrs[0]
 	}
 
 	// update user status
-	if !equality.Semantic.DeepEqual(currentUser, user) {
+	if !equality.Semantic.DeepEqual(currentUser, &user) {
 		log.Debug().PrintObjectDiff(currentUser, &user)
 		if err := r.Status().Update(ctx, &user); err != nil {
 			return ctrl.Result{}, err
 		}
-		log.Debug().Info("status updated")
+		log.Info("status updated")
 	}
 
 	if user.Status.Phase != "AddonFailed" && !cosmov1alpha1.IsPruneDisabled(&user) {
-		log.Info("start garbage collection")
+		log.Debug().Info("checking garbage collection")
 		shouldDeletes := objectRefNotExistsInMap(lastAddons, currAddonsMap)
 		for _, d := range shouldDeletes {
 			if skip, err := prune(ctx, r.Client, d); err != nil {

internal/controllers/user_controller_test.go

@@ -208,7 +208,6 @@ spec:
 
 			Expect(user.Status.Namespace.Name).Should(BeEquivalentTo(createdNs.GetName()))
 			Expect(user.Status.Namespace.UID).Should(BeEquivalentTo(createdNs.GetUID()))
-			Expect(user.Status.Namespace.ResourceVersion).Should(BeEquivalentTo(createdNs.GetResourceVersion()))
 
 			By("check password secret is created")
 
@@ -323,7 +322,6 @@ spec:
 
 			Expect(user.Status.Namespace.Name).Should(BeEquivalentTo(createdNs.GetName()))
 			Expect(user.Status.Namespace.UID).Should(BeEquivalentTo(createdNs.GetUID()))
-			Expect(user.Status.Namespace.ResourceVersion).Should(BeEquivalentTo(createdNs.GetResourceVersion()))
 
 			By("check password secret is not created")
 

internal/controllers/workspace_controller.go

@@ -18,7 +18,7 @@ import (
 
 	cosmov1alpha1 "github.com/cosmo-workspace/cosmo/api/v1alpha1"
 	"github.com/cosmo-workspace/cosmo/pkg/clog"
-	"github.com/cosmo-workspace/cosmo/pkg/kubeutil"
+	"github.com/cosmo-workspace/cosmo/pkg/instance"
 	"github.com/cosmo-workspace/cosmo/pkg/workspace"
 )
 
@@ -60,29 +60,41 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 	ws.Status.Config = cfg
 
-	// sync instance
 	inst := &cosmov1alpha1.Instance{}
 	inst.SetName(ws.Name)
 	inst.SetNamespace(ws.Namespace)
-	op, err := kubeutil.CreateOrUpdate(ctx, r.Client, inst, func() error {
-		return workspace.PatchWorkspaceInstanceAsDesired(inst, ws, r.Scheme)
+
+	tmpl := &cosmov1alpha1.Template{}
+	if err := r.Get(ctx, types.NamespacedName{Name: ws.Spec.Template.Name}, tmpl); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to fetch template %s: %w", ws.Spec.Template.Name, err)
+	}
+
+	// sync
+	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, inst, func() error {
+		if err := workspace.PatchWorkspaceInstanceAsDesired(inst, ws, r.Scheme); err != nil {
+			return err
+		}
+		instance.Mutate(inst, tmpl)
+		return nil
 	})
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 	if op != controllerutil.OperationResultNone {
 		log.Info("instance synced", "instance", inst.Name)
 		r.Recorder.Eventf(&ws, corev1.EventTypeNormal, string(op), "successfully reconciled. instance synced")
+	} else {
+		log.Debug().Info("the result of update workspace instance operation is None", "instance", inst.Name)
 	}
+
 	gvk, _ := apiutil.GVKForObject(inst, r.Scheme)
 	ws.Status.Instance = cosmov1alpha1.ObjectRef{
 		ObjectReference: corev1.ObjectReference{
-			APIVersion:      gvk.GroupVersion().String(),
-			Kind:            gvk.Kind,
-			Name:            inst.GetName(),
-			Namespace:       inst.GetNamespace(),
-			ResourceVersion: inst.GetResourceVersion(),
-			UID:             inst.GetUID(),
+			APIVersion: gvk.GroupVersion().String(),
+			Kind:       gvk.Kind,
+			Name:       inst.GetName(),
+			Namespace:  inst.GetNamespace(),
+			UID:        inst.GetUID(),
 		},
 		CreationTimestamp: &inst.CreationTimestamp,
 	}
@@ -91,7 +103,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	ir := traefikv1.IngressRoute{}
 	ir.SetName(ws.Name)
 	ir.SetNamespace(ws.Namespace)
-	op, err = kubeutil.CreateOrUpdate(ctx, r.Client, &ir, func() error {
+	op, err = controllerutil.CreateOrUpdate(ctx, r.Client, &ir, func() error {
 		return r.TraefikIngressRouteCfg.PatchTraefikIngressRouteAsDesired(&ir, ws, r.Scheme)
 	})
 	if err != nil {
@@ -113,7 +125,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		if err := r.Status().Update(ctx, &ws); err != nil {
 			return ctrl.Result{}, err
 		}
-		log.Debug().Info("status updated")
+		log.Info("status updated")
 	}
 
 	log.Debug().Info("finish reconcile")

internal/controllers/workspace_controller_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"
 
+	"github.com/cosmo-workspace/cosmo/pkg/instance"
 	. "github.com/cosmo-workspace/cosmo/pkg/snap"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -217,7 +218,7 @@ spec:
 
 				for _, v := range createdInst.Spec.Override.PatchesJson6902 {
 					if kubeutil.DeploymentGVK == v.Target.GroupVersionKind() &&
-						v.Target.Name == wsConfig.DeploymentName {
+						v.Target.Name == instance.InstanceResourceName(wsName, wsConfig.DeploymentName) {
 						return v.Patch
 					}
 				}

internal/dashboard/workspace_handler_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"time"
 
 	. "github.com/cosmo-workspace/cosmo/pkg/snap"
 	. "github.com/onsi/ginkgo/v2"
@@ -144,6 +145,7 @@ var _ = Describe("Dashboard server [Workspace]", func() {
 			testUtil.CreateWorkspace("admin-user", "ws1", "template1", nil)
 			testUtil.CreateWorkspace("normal-user", "ws1", "template1", map[string]string{"HOGE": "HOGEHOGE"})
 			testUtil.UpsertNetworkRule("normal-user", "ws1", "add", 18080, "/", false, -1)
+			time.Sleep(100)
 			By("---------------test start----------------")
 			ctx := context.Background()
 			res, err := client.GetWorkspace(ctx, NewRequestWithSession(req, getSession(loginUser)))
@@ -348,6 +350,7 @@ var _ = Describe("Dashboard server [Workspace]", func() {
 			testUtil.UpsertNetworkRule("normal-user", "ws1", "nw1", 9999, "/", false, -1)
 			testUtil.CreateWorkspace("admin-user", "ws1", "template1", map[string]string{})
 			testUtil.UpsertNetworkRule("admin-user", "ws1", "nw1", 9999, "/", false, -1)
+			time.Sleep(100)
 			By("---------------test start----------------")
 			ctx := context.Background()
 			res, err := client.DeleteNetworkRule(ctx, NewRequestWithSession(req, getSession(loginUser)))