Set package initial status if its empty (#1522)

If the user applies package YAML file has no status field,
the package won't be able to be compiled or deployed due
to lack of status. This PR aims to add a check at buildermgr
to set initial package status to those packages.

pkg/apis/core/v1/types.go

@@ -268,6 +268,9 @@ type (
 
 	// PackageStatus contains the build status of a package also the build log for examination.
 	PackageStatus struct {
+		// TODO: Add another status field to indicate whether a package
+		//   is ready for deploy instead of setting "none" in build status.
+
 		// BuildStatus is the package build status.
 		BuildStatus BuildStatus `json:"buildstatus,omitempty"`
 
@@ -684,3 +687,7 @@ type (
 		GetObjectMeta() metav1.Object
 	}
 )
+
+func (a Archive) IsEmpty() bool {
+	return len(a.Literal) == 0 && len(a.URL) == 0
+}

pkg/buildermgr/buildermgr.go

@@ -48,7 +48,7 @@ func Start(logger *zap.Logger, storageSvcUrl string, envBuilderNamespace string)
 
 	pkgWatcher := makePackageWatcher(bmLogger, fissionClient,
 		kubernetesClient, envBuilderNamespace, storageSvcUrl)
-	go pkgWatcher.watchPackages(fissionClient, kubernetesClient, envBuilderNamespace)
+	go pkgWatcher.watchPackages()
 
 	select {}
 }

pkg/buildermgr/pkgwatcher.go

@@ -75,12 +75,6 @@ func makePackageWatcher(logger *zap.Logger, fissionClient *crd.FissionClient, k8
 // 6. Update package status to succeed state
 // *. Update package status to failed state,if any one of steps above failed/time out
 func (pkgw *packageWatcher) build(buildCache *cache.Cache, srcpkg *fv1.Package) {
-
-	// Ignore non-pending state packages.
-	if srcpkg.Status.BuildStatus != fv1.BuildStatusPending {
-		return
-	}
-
 	// Ignore duplicate build requests
 	key := fmt.Sprintf("%v-%v", srcpkg.ObjectMeta.Name, srcpkg.ObjectMeta.ResourceVersion)
 	_, err := buildCache.Set(key, srcpkg)
@@ -228,20 +222,76 @@ func (pkgw *packageWatcher) build(buildCache *cache.Cache, srcpkg *fv1.Package)
 		zap.String("package", fmt.Sprintf("%s.%s", pkg.ObjectMeta.Name, pkg.ObjectMeta.Namespace)))
 }
 
-func (pkgw *packageWatcher) watchPackages(fissionClient *crd.FissionClient,
+func (pkgw *packageWatcher) watchPackages() {
-	kubernetesClient *kubernetes.Clientset, builderNamespace string) {
 	buildCache := cache.MakeCache(0, 0)
 	lw := k8sCache.NewListWatchFromClient(pkgw.fissionClient.CoreV1().RESTClient(), "packages", apiv1.NamespaceAll, fields.Everything())
-	pkgStore, controller := k8sCache.NewInformer(lw, &fv1.Package{}, 60*time.Second, k8sCache.ResourceEventHandlerFuncs{
+
+	processPkg := func(pkg *fv1.Package) {
+		var err error
+
+		if len(pkg.Status.BuildStatus) == 0 {
+			_, err = setInitialBuildStatus(pkgw.fissionClient, pkg)
+			if err != nil {
+				pkgw.logger.Error("error filling package status", zap.Error(err))
+			}
+			// once we update the package status, an update event
+			// will arrive and handle by UpdateFunc later. So we
+			// don't need to build the package at this moment.
+			return
+		}
+
+		// Only build pending state packages.
+		if pkg.Status.BuildStatus == fv1.BuildStatusPending {
+			go pkgw.build(buildCache, pkg)
+		}
+	}
+
+	pkgStore, controller := k8sCache.NewInformer(lw, &fv1.Package{}, 60*time.Minute, k8sCache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			pkg := obj.(*fv1.Package)
-			go pkgw.build(buildCache, pkg)
+			processPkg(pkg)
 		},
 		UpdateFunc: func(oldObj, newObj interface{}) {
+			oldPkg := oldObj.(*fv1.Package)
 			pkg := newObj.(*fv1.Package)
-			go pkgw.build(buildCache, pkg)
+
+			// TODO: Once enable "/status", check generation for spec changed instead.
+			//   Before "/status" is enabled, the generation and resource version will be changed
+			//   if we update the status of a package, hence we are not able to differentiate
+			//   the spec change or status change. So we only build package which's status
+			//   us "pending" and user have to use "kubectl replace" to update a package.
+			if oldPkg.ResourceVersion == pkg.ResourceVersion &&
+				pkg.Status.BuildStatus != fv1.BuildStatusPending {
+				return
+			}
+			processPkg(pkg)
 		},
 	})
+
 	pkgw.pkgStore = pkgStore
 	controller.Run(make(chan struct{}))
 }
+
+// setInitialBuildStatus sets initial build status to a package if it is empty.
+// This normally occurs when the user applies package YAML files that have no status field
+// through kubectl.
+func setInitialBuildStatus(fissionClient *crd.FissionClient, pkg *fv1.Package) (*fv1.Package, error) {
+	pkg.Status = fv1.PackageStatus{
+		LastUpdateTimestamp: metav1.Time{Time: time.Now().UTC()},
+	}
+	if !pkg.Spec.Deployment.IsEmpty() {
+		// if the deployment archive is not empty,
+		// we assume it's a deployable package no matter
+		// the source archive is empty or not.
+		pkg.Status.BuildStatus = fv1.BuildStatusNone
+	} else if !pkg.Spec.Source.IsEmpty() {
+		pkg.Status.BuildStatus = fv1.BuildStatusPending
+	} else {
+		// mark package failed since we cannot do anything with it.
+		pkg.Status.BuildStatus = fv1.BuildStatusFailed
+		pkg.Status.BuildLog = "Both deploy and source archive are empty"
+	}
+
+	// TODO: use UpdateStatus to update status
+	return fissionClient.CoreV1().Packages(pkg.Namespace).Update(pkg)
+}

pkg/controller/controller.go

@@ -18,7 +18,6 @@ package controller
 
 import (
 	"context"
-
 	"go.uber.org/zap"
 
 	"github.com/fission/fission/pkg/crd"

test/test_utils.sh

@@ -541,7 +541,8 @@ run_all_tests() {
         $ROOT/test/tests/test_environments/test_go_env.sh \
         $ROOT/test/tests/mqtrigger/nats/test_mqtrigger.sh \
         $ROOT/test/tests/mqtrigger/nats/test_mqtrigger_error.sh \
-        $ROOT/test/tests/test_huge_response/test_huge_response.sh
+        $ROOT/test/tests/test_huge_response/test_huge_response.sh \
+        $ROOT/test/tests/test_kubectl/test_kubectl.sh
     FAILURES=$?
 
     export JOBS=3

test/tests/test_kubectl/spec-yaml/env-go.yaml

@@ -0,0 +1,18 @@
+apiVersion: fission.io/v1
+kind: Environment
+metadata:
+  creationTimestamp: null
+  name: go-spec-kubectl
+  namespace: default
+spec:
+  builder:
+    command: build
+    image: fission/go-builder-1.12:1.7.1
+  imagepullsecret: ""
+  keeparchive: false
+  poolsize: 3
+  resources: {}
+  runtime:
+    image: fission/go-env-1.12:1.7.1
+  terminationGracePeriod: 5
+  version: 2

test/tests/test_kubectl/spec-yaml/function-go.yaml

@@ -0,0 +1,47 @@
+apiVersion: fission.io/v1
+kind: Package
+metadata:
+  creationTimestamp: null
+  name: go-b4bbb0e0-2d93-47f0-8c4e-eea644eec2a9
+  namespace: default
+spec:
+  deployment:
+    checksum: {}
+  environment:
+    name: go-spec-kubectl
+    namespace: default
+  source:
+    checksum:
+      sum: aa595bb952047c517d849f8fc9e490fdabc37d83795392074a0b15a59748004f
+      type: sha256
+    type: url
+    url: https://raw.githubusercontent.com/fission/fission/master/examples/go/hello.gogo # this is intentional
+
+---
+apiVersion: fission.io/v1
+kind: Function
+metadata:
+  creationTimestamp: null
+  name: go-spec-kubectl
+  namespace: default
+spec:
+  InvokeStrategy:
+    ExecutionStrategy:
+      ExecutorType: poolmgr
+      MaxScale: 0
+      MinScale: 0
+      SpecializationTimeout: 120
+      TargetCPUPercent: 0
+    StrategyType: execution
+  configmaps: null
+  environment:
+    name: go-spec-kubectl
+    namespace: default
+  functionTimeout: 60
+  package:
+    functionName: Handler
+    packageref:
+      name: go-b4bbb0e0-2d93-47f0-8c4e-eea644eec2a9
+      namespace: default
+  resources: {}
+  secrets: null

test/tests/test_kubectl/test_kubectl.sh

@@ -0,0 +1,50 @@
+#!/bin/bash
+
+set -euo pipefail
+source $(dirname $0)/../../utils.sh
+
+TEST_ID=$(generate_test_id)
+echo "TEST_ID = $TEST_ID"
+
+tmp_dir="/tmp/test-$TEST_ID"
+mkdir -p $tmp_dir
+
+ROOT=$(dirname $0)/../../..
+
+cd $ROOT/test/tests/test_kubectl
+
+cleanup() {
+    kubectl delete -f spec-yaml -R || true
+}
+
+if [ -z "${TEST_NOCLEANUP:-}" ]; then
+    trap cleanup EXIT
+else
+    log "TEST_NOCLEANUP is set; not cleaning up test artifacts afterwards."
+fi
+
+name="go-spec-kubectl"
+pkgName="go-b4bbb0e0-2d93-47f0-8c4e-eea644eec2a9"
+
+# cleanup first
+cleanup
+
+# apply environment & function
+kubectl apply -f spec-yaml -R
+
+# wait for build to finish
+timeout 90 bash -c "wait_for_builder $name"
+timeout 90 bash -c "waitBuildExpectedStatus $pkgName failed"
+
+sed -i 's/gogo/go/g' spec-yaml/function-go.yaml
+
+# before we enable "/status" this should be failed.
+kubectl apply -f spec-yaml/function-go.yaml
+timeout 90 bash -c "waitBuildExpectedStatus $pkgName failed"
+
+kubectl replace -f spec-yaml/function-go.yaml
+timeout 90 bash -c "waitBuild $pkgName"
+
+fission fn test --name $name
+
+log "Test PASSED"

test/utils.sh

@@ -152,6 +152,24 @@ waitBuild() {
 }
 export -f waitBuild
 
+waitBuildExpectedStatus() {
+    pkg=$1
+    status=$2
+
+    log "Waiting for builder manager to finish the build with status $status"
+
+    set +e
+    while true; do
+      kubectl --namespace default get packages $pkg -o jsonpath='{.status.buildstatus}'|grep $status
+      if [[ $? -eq 0 ]]; then
+          break
+      fi
+      sleep 1
+    done
+    set -e
+}
+export -f waitBuildExpectedStatus
+
 
 ## Common env parameters
 export FISSION_NAMESPACE=${FISSION_NAMESPACE:-fission}