Take into account number of unavailable replicas to decided if deployment is healthy or not (#270)

* Take into account number of unavailable replicas to decided if deployment is healthy or not

* Run one controller for all e2e tests to reduce tests duration

* Apply reviewer notes: use logic from kubectl/rollout_status.go to check deployment health

VERSION

@@ -1 +1 @@
-0.4.6
+0.4.7

controller/health.go

@@ -60,30 +60,54 @@ func (ctrl *kubeAppHealthManager) getDeploymentHealth(config *rest.Config, names
 	if err != nil {
 		return nil, err
 	}
-	deploy, err := clientSet.AppsV1().Deployments(namespace).Get(name, metav1.GetOptions{})
+	deployment, err := clientSet.AppsV1().Deployments(namespace).Get(name, metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
-	health := appv1.HealthStatus{
-		Status: appv1.HealthStatusUnknown,
-	}
-	for _, condition := range deploy.Status.Conditions {
-		// deployment is healthy is it successfully progressed
-		if condition.Type == v1.DeploymentProgressing && condition.Status == "True" {
-			health.Status = appv1.HealthStatusHealthy
-		} else if condition.Type == v1.DeploymentReplicaFailure && condition.Status == "True" {
-			health.Status = appv1.HealthStatusDegraded
-		} else if condition.Type == v1.DeploymentProgressing && condition.Status == "False" {
-			health.Status = appv1.HealthStatusDegraded
-		} else if condition.Type == v1.DeploymentAvailable && condition.Status == "False" {
-			health.Status = appv1.HealthStatusDegraded
+
+	if deployment.Generation <= deployment.Status.ObservedGeneration {
+		cond := getDeploymentCondition(deployment.Status, v1.DeploymentProgressing)
+		if cond != nil && cond.Reason == "ProgressDeadlineExceeded" {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusDegraded,
+				StatusDetails: fmt.Sprintf("Deployment %q exceeded its progress deadline", name),
+			}, nil
+		} else if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d out of %d new replicas have been updated...\n", deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas),
+			}, nil
+		} else if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas),
+			}, nil
+		} else if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas),
+			}, nil
 		}
-		if health.Status != appv1.HealthStatusUnknown {
-			health.StatusDetails = fmt.Sprintf("%s:%s", condition.Reason, condition.Message)
-			break
+	} else {
+		return &appv1.HealthStatus{
+			Status:        appv1.HealthStatusProgressing,
+			StatusDetails: "Waiting for rollout to finish: observed deployment generation less then desired generation",
+		}, nil
+	}
+
+	return &appv1.HealthStatus{
+		Status: appv1.HealthStatusHealthy,
+	}, nil
+}
+
+func getDeploymentCondition(status v1.DeploymentStatus, condType v1.DeploymentConditionType) *v1.DeploymentCondition {
+	for i := range status.Conditions {
+		c := status.Conditions[i]
+		if c.Type == condType {
+			return &c
 		}
 	}
-	return &health, nil
+	return nil
 }
 
 func (ctrl *kubeAppHealthManager) GetAppHealth(server string, namespace string, comparisonResult *appv1.ComparisonResult) (*appv1.HealthStatus, error) {

examples/guestbook/components/guestbook-ui.jsonnet

@@ -23,7 +23,7 @@ local appDeployment = deployment
     params.replicas,
     container
       .new(params.name, params.image)
-      .withPorts(containerPort.new(targetPort)),
+      .withPorts(containerPort.new(targetPort)) + if params.command != null then { command: [ params.command ] } else {},
     labels);
 
 k.core.v1.list.new([appService, appDeployment])

examples/guestbook/components/params.libsonnet

@@ -12,7 +12,8 @@
       name: "guestbook-ui",
       replicas: 1,
       servicePort: 80,
-      type: "LoadBalancer",
+      type: "ClusterIP",
+      command: null,
     },
   },
 }

server/application/application.go

@@ -510,7 +510,7 @@ func (s *Server) setAppOperation(ctx context.Context, appName string, operationC
 		}
 		a.Operation = op
 		a.Status.OperationState = nil
-		_, err = s.Update(ctx, a)
+		_, err = s.appclientset.ArgoprojV1alpha1().Applications(s.ns).Update(a)
 		if err != nil && apierr.IsConflict(err) {
 			log.Warnf("Failed to set operation for app '%s' due to update conflict. Retrying again...", appName)
 		} else {

server/server.go

@@ -162,7 +162,6 @@ func (a *ArgoCDServer) Run(ctx context.Context, port int) {
 		go func() { a.checkServeErr("httpsS", httpsS.Serve(httpsL)) }()
 		go func() { a.checkServeErr("tlsm", tlsm.Serve()) }()
 	}
-	go a.initializeOIDCClientApp()
 	go a.watchSettings(ctx)
 	go func() { a.checkServeErr("tcpm", tcpm.Serve()) }()
 
@@ -234,31 +233,6 @@ func (a *ArgoCDServer) watchSettings(ctx context.Context) {
 	close(updateCh)
 }
 
-// initializeOIDCClientApp initializes the OIDC Client application, querying the well known oidc
-// configuration path. Because ArgoCD is a OIDC client to itself, we have a chicken-and-egg problem
-// of (1) serving dex over HTTP, and (2) querying the OIDC provider (ourselves) to initialize the
-// app (HTTP GET http://example-argocd.com/api/dex/.well-known/openid-configuration)
-// This method is expected to be invoked right after we start listening over HTTP
-func (a *ArgoCDServer) initializeOIDCClientApp() {
-	if !a.settings.IsSSOConfigured() {
-		return
-	}
-	// wait for dex to become ready
-	dexClient, err := dexutil.NewDexClient()
-	errors.CheckError(err)
-	dexClient.WaitUntilReady()
-	var realErr error
-	_ = wait.ExponentialBackoff(backoff, func() (bool, error) {
-		_, realErr = a.sessionMgr.OIDCProvider()
-		if realErr != nil {
-			a.log.Warnf("failed to initialize client app: %v", realErr)
-			return false, nil
-		}
-		return true, nil
-	})
-	errors.CheckError(realErr)
-}
-
 func (a *ArgoCDServer) useTLS() bool {
 	if a.Insecure || a.settings.Certificate == nil {
 		return false

test/e2e/app_management_test.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	"context"
 	"strconv"
 	"testing"
 	"time"
@@ -18,9 +17,7 @@ import (
 )
 
 func TestAppManagement(t *testing.T) {
-
 	testApp := &v1alpha1.Application{
-		ObjectMeta: metav1.ObjectMeta{GenerateName: "e2e-test"},
 		Spec: v1alpha1.ApplicationSpec{
 			Source: v1alpha1.ApplicationSource{
 				RepoURL: "https://github.com/argoproj/argo-cd.git", Path: ".", Environment: "minikube",
@@ -72,20 +69,12 @@ func TestAppManagement(t *testing.T) {
 	})
 
 	t.Run("TestTrackAppStateAndSyncApp", func(t *testing.T) {
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
-		// create app and ensure it reaches OutOfSync state
 		app := fixture.CreateApp(t, testApp)
 		WaitUntil(t, func() (done bool, err error) {
 			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
 			return err == nil && app.Status.ComparisonResult.Status != v1alpha1.ComparisonStatusUnknown, err
 		})
 
-		assert.Equal(t, v1alpha1.ComparisonStatusOutOfSync, app.Status.ComparisonResult.Status)
-
 		// sync app and make sure it reaches InSync state
 		_, err := fixture.RunCli("app", "sync", app.Name)
 		if err != nil {
@@ -103,30 +92,30 @@ func TestAppManagement(t *testing.T) {
 
 	t.Run("TestAppRollbackSuccessful", func(t *testing.T) {
 		appWithHistory := testApp.DeepCopy()
-		appWithHistory.Status.History = []v1alpha1.DeploymentInfo{{
-			ID:       1,
-			Revision: "abc",
-		}, {
-			ID:       2,
-			Revision: "cdb",
-		}}
 
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
-		// create app and ensure it reaches OutOfSync state
+		// create app and ensure it's comparion status is not ComparisonStatusUnknown
 		app := fixture.CreateApp(t, appWithHistory)
+		app.Status.History = []v1alpha1.DeploymentInfo{{
+			ID:                          1,
+			Revision:                    "abc",
+			ComponentParameterOverrides: app.Spec.Source.ComponentParameterOverrides,
+		}, {
+			ID:                          2,
+			Revision:                    "cdb",
+			ComponentParameterOverrides: app.Spec.Source.ComponentParameterOverrides,
+		}}
+		app, err := fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Update(app)
+		if err != nil {
+			t.Fatalf("Unable to update app %v", err)
+		}
+
 		WaitUntil(t, func() (done bool, err error) {
 			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
 			return err == nil && app.Status.ComparisonResult.Status != v1alpha1.ComparisonStatusUnknown, err
 		})
 
-		assert.Equal(t, v1alpha1.ComparisonStatusOutOfSync, app.Status.ComparisonResult.Status)
-
 		// sync app and make sure it reaches InSync state
-		_, err := fixture.RunCli("app", "rollback", app.Name, "1")
+		_, err = fixture.RunCli("app", "rollback", app.Name, "1")
 		if err != nil {
 			t.Fatalf("Unable to sync app %v", err)
 		}
@@ -146,11 +135,6 @@ func TestAppManagement(t *testing.T) {
 		invalidApp := testApp.DeepCopy()
 		invalidApp.Spec.Destination.Server = "https://not-registered-cluster/api"
 
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
 		app := fixture.CreateApp(t, invalidApp)
 
 		WaitUntil(t, func() (done bool, err error) {
@@ -165,4 +149,39 @@ func TestAppManagement(t *testing.T) {
 
 		assert.Equal(t, v1alpha1.ComparisonStatusError, app.Status.ComparisonResult.Status)
 	})
+
+	t.Run("TestArgoCDWaitEnsureAppIsNotCrashing", func(t *testing.T) {
+		updatedApp := testApp.DeepCopy()
+
+		// deploy app and make sure it is healthy
+		app := fixture.CreateApp(t, updatedApp)
+		_, err := fixture.RunCli("app", "sync", app.Name)
+		if err != nil {
+			t.Fatalf("Unable to sync app %v", err)
+		}
+
+		WaitUntil(t, func() (done bool, err error) {
+			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
+			return err == nil && app.Status.ComparisonResult.Status == v1alpha1.ComparisonStatusSynced && app.Status.Health.Status == v1alpha1.HealthStatusHealthy, err
+		})
+
+		// deploy app which fails and make sure it became unhealthy
+		app.Spec.Source.ComponentParameterOverrides = append(
+			app.Spec.Source.ComponentParameterOverrides,
+			v1alpha1.ComponentParameter{Name: "command", Value: "wrong-command", Component: "guestbook-ui"})
+		_, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Update(app)
+		if err != nil {
+			t.Fatalf("Unable to set app parameter %v", err)
+		}
+
+		_, err = fixture.RunCli("app", "sync", app.Name)
+		if err != nil {
+			t.Fatalf("Unable to sync app %v", err)
+		}
+
+		WaitUntil(t, func() (done bool, err error) {
+			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
+			return err == nil && app.Status.ComparisonResult.Status == v1alpha1.ComparisonStatusSynced && app.Status.Health.Status == v1alpha1.HealthStatusDegraded, err
+		})
+	})
 }

test/e2e/fixture.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	"bytes"
 	"context"
 	"fmt"
 	"log"
@@ -13,6 +12,8 @@ import (
 
 	"encoding/json"
 
+	"strings"
+
 	"github.com/argoproj/argo-cd/cmd/argocd/commands"
 	"github.com/argoproj/argo-cd/common"
 	"github.com/argoproj/argo-cd/controller"
@@ -128,22 +129,11 @@ func (f *Fixture) setup() error {
 	})
 
 	ctx, cancel := context.WithCancel(context.Background())
-	go func() {
-		err = repoServerGRPC.Serve(repoServerListener)
-	}()
 	go func() {
 		apiServer.Run(ctx, apiServerPort)
 	}()
 
-	f.tearDownCallback = func() {
-		cancel()
-		repoServerGRPC.Stop()
-	}
-	if err != nil {
-		return err
-	}
-
-	return waitUntilE(func() (done bool, err error) {
+	err = waitUntilE(func() (done bool, err error) {
 		clientset, err := f.NewApiClientset()
 		if err != nil {
 			return false, nil
@@ -156,6 +146,22 @@ func (f *Fixture) setup() error {
 		_, err = appClient.List(context.Background(), &application.ApplicationQuery{})
 		return err == nil, nil
 	})
+
+	ctrl := f.createController()
+	ctrlCtx, cancelCtrl := context.WithCancel(context.Background())
+	go ctrl.Run(ctrlCtx, 1, 1)
+
+	go func() {
+		err = repoServerGRPC.Serve(repoServerListener)
+	}()
+
+	f.tearDownCallback = func() {
+		cancel()
+		cancelCtrl()
+		repoServerGRPC.Stop()
+	}
+
+	return err
 }
 
 func (f *Fixture) ensureClusterRegistered() error {
@@ -251,6 +257,8 @@ func NewFixture() (*Fixture, error) {
 
 // CreateApp creates application with appropriate controller instance id.
 func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1alpha1.Application {
+	application = application.DeepCopy()
+	application.Name = fmt.Sprintf("e2e-test-%v", time.Now().Unix())
 	labels := application.ObjectMeta.Labels
 	if labels == nil {
 		labels = make(map[string]string)
@@ -258,6 +266,10 @@ func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1
 	}
 	labels[common.LabelKeyApplicationControllerInstanceID] = f.InstanceID
 
+	application.Spec.Source.ComponentParameterOverrides = append(
+		application.Spec.Source.ComponentParameterOverrides,
+		v1alpha1.ComponentParameter{Name: "name", Value: application.Name, Component: "guestbook-ui"})
+
 	app, err := f.AppClient.ArgoprojV1alpha1().Applications(f.Namespace).Create(application)
 	if err != nil {
 		t.Fatal(fmt.Sprintf("Unable to create app %v", err))
@@ -265,8 +277,8 @@ func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1
 	return app
 }
 
-// CreateController creates new controller instance
-func (f *Fixture) CreateController() *controller.ApplicationController {
+// createController creates new controller instance
+func (f *Fixture) createController() *controller.ApplicationController {
 	appStateManager := controller.NewAppStateManager(
 		f.DB, f.AppClient, reposerver.NewRepositoryServerClientset(f.RepoServerAddress), f.Namespace)
 
@@ -292,12 +304,21 @@ func (f *Fixture) NewApiClientset() (argocdclient.Client, error) {
 }
 
 func (f *Fixture) RunCli(args ...string) (string, error) {
-	cmd := commands.NewCommand()
-	cmd.SetArgs(append(args, "--server", f.ApiServerAddress, "--plaintext"))
-	output := new(bytes.Buffer)
-	cmd.SetOutput(output)
-	err := cmd.Execute()
-	return output.String(), err
+	args = append([]string{"run", "../../cmd/argocd/main.go"}, args...)
+	cmd := exec.Command("go", append(args, "--server", f.ApiServerAddress, "--plaintext")...)
+	outBytes, err := cmd.Output()
+	if err != nil {
+		exErr, ok := err.(*exec.ExitError)
+		if !ok {
+			return "", err
+		}
+		errOutput := string(exErr.Stderr)
+		if outBytes != nil {
+			errOutput = string(outBytes) + "\n" + errOutput
+		}
+		return "", fmt.Errorf(strings.TrimSpace(errOutput))
+	}
+	return string(outBytes), nil
 }
 
 func waitUntilE(condition wait.ConditionFunc) error {

util/db/repository.go

@@ -123,7 +123,8 @@ func repoURLToSecretName(repo string) string {
 	h := fnv.New32a()
 	_, _ = h.Write([]byte(repo))
 	parts := strings.Split(strings.TrimSuffix(repo, ".git"), "/")
-	return fmt.Sprintf("repo-%s-%v", parts[len(parts)-1], h.Sum32())
+	shortName := strings.Replace(parts[len(parts)-1], "_", "-", -1)
+	return fmt.Sprintf("repo-%s-%v", shortName, h.Sum32())
 }
 
 // repoToStringData converts a repository object to string data for serialization to a secret

util/db/repository_test.go

@@ -8,6 +8,7 @@ func TestRepoURLToSecretName(t *testing.T) {
 		"https://github.com/argoproj/ARGO-cd":       "repo-argo-cd-821842295",
 		"https://github.com/argoproj/argo-cd":       "repo-argo-cd-821842295",
 		"https://github.com/argoproj/argo-cd.git":   "repo-argo-cd-821842295",
+		"https://github.com/argoproj/argo_cd.git":   "repo-argo-cd-1049844989",
 		"ssh://git@github.com/argoproj/argo-cd.git": "repo-argo-cd-1019298066",
 	}
 

util/dex/dex.go

@@ -12,17 +12,18 @@ import (
 	"os"
 	"time"
 
-	"github.com/argoproj/argo-cd/common"
-	"github.com/argoproj/argo-cd/errors"
-	"github.com/argoproj/argo-cd/util/cache"
-	"github.com/argoproj/argo-cd/util/session"
-	"github.com/argoproj/argo-cd/util/settings"
 	"github.com/coreos/dex/api"
 	oidc "github.com/coreos/go-oidc"
 	jwt "github.com/dgrijalva/jwt-go"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/oauth2"
 	"google.golang.org/grpc"
+
+	"github.com/argoproj/argo-cd/common"
+	"github.com/argoproj/argo-cd/errors"
+	"github.com/argoproj/argo-cd/util/cache"
+	"github.com/argoproj/argo-cd/util/session"
+	"github.com/argoproj/argo-cd/util/settings"
 )
 
 const (
@@ -141,15 +142,18 @@ func NewClientApp(settings *settings.ArgoCDSettings, sessionMgr *session.Session
 	return &a, nil
 }
 
-func (a *ClientApp) oauth2Config(scopes []string) *oauth2.Config {
-	provider, _ := a.sessionMgr.OIDCProvider()
+func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) {
+	provider, err := a.sessionMgr.OIDCProvider()
+	if err != nil {
+		return nil, err
+	}
 	return &oauth2.Config{
 		ClientID:     a.clientID,
 		ClientSecret: a.clientSecret,
 		Endpoint:     provider.Endpoint(),
 		Scopes:       scopes,
 		RedirectURL:  a.redirectURI,
-	}
+	}, nil
 }
 
 var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
@@ -196,18 +200,23 @@ func (a *ClientApp) verifyAppState(state string) (*appState, error) {
 }
 
 func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
-	var authCodeURL string
+	var opts []oauth2.AuthCodeOption
 	returnURL := r.FormValue("return_url")
 	scopes := []string{"openid", "profile", "email", "groups"}
 	appState := a.generateAppState(returnURL)
 	if r.FormValue("offline_access") != "yes" {
-		authCodeURL = a.oauth2Config(scopes).AuthCodeURL(appState)
+		// no-op
 	} else if a.sessionMgr.OfflineAsScope() {
 		scopes = append(scopes, "offline_access")
-		authCodeURL = a.oauth2Config(scopes).AuthCodeURL(appState)
 	} else {
-		authCodeURL = a.oauth2Config(scopes).AuthCodeURL(appState, oauth2.AccessTypeOffline)
+		opts = append(opts, oauth2.AccessTypeOffline)
 	}
+	oauth2Config, err := a.oauth2Config(scopes)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	authCodeURL := oauth2Config.AuthCodeURL(appState, opts...)
 	http.Redirect(w, r, authCodeURL, http.StatusSeeOther)
 }
 
@@ -219,7 +228,11 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
 	)
 
 	ctx := oidc.ClientContext(r.Context(), a.client)
-	oauth2Config := a.oauth2Config(nil)
+	oauth2Config, err := a.oauth2Config(nil)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
 	switch r.Method {
 	case "GET":
 		// Authorization redirect callback from OAuth2 auth flow.

util/git/git.go

@@ -98,10 +98,12 @@ func TestRepo(repo, username, password string, sshPrivateKey string) error {
 	cmd.Env = env
 	_, err = cmd.Output()
 	if err != nil {
-		exErr := err.(*exec.ExitError)
-		errOutput := strings.Split(string(exErr.Stderr), "\n")[0]
-		errOutput = redactPassword(errOutput, password)
-		return fmt.Errorf("%s: %s", repo, errOutput)
+		if exErr, ok := err.(*exec.ExitError); ok {
+			errOutput := strings.Split(string(exErr.Stderr), "\n")[0]
+			errOutput = redactPassword(errOutput, password)
+			return fmt.Errorf("%s: %s", repo, errOutput)
+		}
+		return err
 	}
 	return nil
 }

util/session/sessionmanager.go

@@ -197,7 +197,11 @@ func MakeCookieMetadata(key, value string, flags ...string) string {
 	return strings.Join(components, "; ")
 }
 
-// OIDCProvider lazily returns the OIDC provider
+// OIDCProvider lazily initializes and returns the OIDC provider, querying the well known oidc
+// configuration path (http://example-argocd.com/api/dex/.well-known/openid-configuration).
+// We have to initialize the proviver lazily since ArgoCD is an OIDC client to itself, which
+// presents a chicken-and-egg problem of (1) serving dex over HTTP, and (2) querying the OIDC
+// provider (ourselves) to initialize the app.
 func (mgr *SessionManager) OIDCProvider() (*oidc.Provider, error) {
 	if mgr.provider != nil {
 		return mgr.provider, nil