Fix errors, verification and test failures; add unit test for sj UpdateStatus

pkg/client/clientset_generated/internalclientset/typed/batch/unversioned/batch_client.go

@@ -18,7 +18,6 @@ package unversioned
 
 import (
 	api "k8s.io/kubernetes/pkg/api"
-	//"k8s.io/kubernetes/pkg/api/unversioned"
 	registered "k8s.io/kubernetes/pkg/apimachinery/registered"
 	restclient "k8s.io/kubernetes/pkg/client/restclient"
 )
@@ -82,17 +81,9 @@ func setConfigDefaults(config *restclient.Config) error {
 	}
 	// TODO: Unconditionally set the config.Version, until we fix the config.
 	//if config.Version == "" {
-	// XXX why is above commented out?
 	copyGroupVersion := g.GroupVersion
 	config.GroupVersion = &copyGroupVersion
 	//}
-	// Do we need something like this:
-	// if config.Version == "" {
-	//	copyGroupVersion := g.GroupVersion
-	//	config.GroupVersion = &copyGroupVersion
-	//} else {
-	//	config.GroupVersion = &unversioned.GroupVersion{Group: "batch", Version: config.Version}
-	//}
 
 	config.NegotiatedSerializer = api.Codecs
 

pkg/controller/job/jobcontroller.go

@@ -113,7 +113,7 @@ func NewJobController(podInformer framework.SharedIndexInformer, kubeClient clie
 		framework.ResourceEventHandlerFuncs{
 			AddFunc: jm.enqueueController,
 			UpdateFunc: func(old, cur interface{}) {
-				if job := cur.(*batch.Job); !isJobFinished(job) {
+				if job := cur.(*batch.Job); !IsJobFinished(job) {
 					jm.enqueueController(job)
 				}
 			},
@@ -347,7 +347,7 @@ func (jm *JobController) syncJob(key string) error {
 		job.Status.StartTime = &now
 	}
 	// if job was finished previously, we don't want to redo the termination
-	if isJobFinished(&job) {
+	if IsJobFinished(&job) {
 		return nil
 	}
 	if pastActiveDeadline(&job) {
@@ -559,15 +559,6 @@ func filterPods(pods []api.Pod, phase api.PodPhase) int {
 	return result
 }
 
-func isJobFinished(j *batch.Job) bool {
-	for _, c := range j.Status.Conditions {
-		if (c.Type == batch.JobComplete || c.Type == batch.JobFailed) && c.Status == api.ConditionTrue {
-			return true
-		}
-	}
-	return false
-}
-
 // byCreationTimestamp sorts a list by creation timestamp, using their names as a tie breaker.
 type byCreationTimestamp []batch.Job
 

pkg/controller/job/jobcontroller_test.go

@@ -657,31 +657,6 @@ func TestWatchJobs(t *testing.T) {
 	<-received
 }
 
-func TestIsJobFinished(t *testing.T) {
-	job := &batch.Job{
-		Status: batch.JobStatus{
-			Conditions: []batch.JobCondition{{
-				Type:   batch.JobComplete,
-				Status: api.ConditionTrue,
-			}},
-		},
-	}
-
-	if !isJobFinished(job) {
-		t.Error("Job was expected to be finished")
-	}
-
-	job.Status.Conditions[0].Status = api.ConditionFalse
-	if isJobFinished(job) {
-		t.Error("Job was not expected to be finished")
-	}
-
-	job.Status.Conditions[0].Status = api.ConditionUnknown
-	if isJobFinished(job) {
-		t.Error("Job was not expected to be finished")
-	}
-}
-
 func TestWatchPods(t *testing.T) {
 	testJob := newJob(2, 2)
 	clientset := fake.NewSimpleClientset(testJob)

pkg/controller/job/utils.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package job
+
+import (
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/apis/batch"
+)
+
+func IsJobFinished(j *batch.Job) bool {
+	for _, c := range j.Status.Conditions {
+		if (c.Type == batch.JobComplete || c.Type == batch.JobFailed) && c.Status == api.ConditionTrue {
+			return true
+		}
+	}
+	return false
+}

pkg/controller/job/utils_test.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package job
+
+import (
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/apis/batch"
+)
+
+func TestIsJobFinished(t *testing.T) {
+	job := &batch.Job{
+		Status: batch.JobStatus{
+			Conditions: []batch.JobCondition{{
+				Type:   batch.JobComplete,
+				Status: api.ConditionTrue,
+			}},
+		},
+	}
+
+	if !IsJobFinished(job) {
+		t.Error("Job was expected to be finished")
+	}
+
+	job.Status.Conditions[0].Status = api.ConditionFalse
+	if IsJobFinished(job) {
+		t.Error("Job was not expected to be finished")
+	}
+
+	job.Status.Conditions[0].Status = api.ConditionUnknown
+	if IsJobFinished(job) {
+		t.Error("Job was not expected to be finished")
+	}
+}

pkg/controller/scheduledjob/controller.go

 /*
-Copyright 2016 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -40,6 +40,7 @@ import (
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	unversionedcore "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/unversioned"
 	"k8s.io/kubernetes/pkg/client/record"
+	"k8s.io/kubernetes/pkg/controller/job"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/metrics"
 	utilruntime "k8s.io/kubernetes/pkg/util/runtime"
@@ -84,8 +85,8 @@ func NewScheduledJobControllerFromClient(kubeClient clientset.Interface) *Schedu
 func (jm *ScheduledJobController) Run(stopCh <-chan struct{}) {
 	defer utilruntime.HandleCrash()
 	glog.Infof("Starting ScheduledJob Manager")
-	// Check things every 1 second.
-	go wait.Until(jm.SyncAll, 1*time.Second, stopCh)
+	// Check things every 10 second.
+	go wait.Until(jm.SyncAll, 10*time.Second, stopCh)
 	<-stopCh
 	glog.Infof("Shutting down ScheduledJob Manager")
 }
@@ -118,13 +119,11 @@ func (jm *ScheduledJobController) SyncAll() {
 
 // SyncOne reconciles a ScheduledJob with a list of any Jobs that it created.
 // All known jobs created by "sj" should be included in "js".
-// Returns a new ScheduledJobStatus if an update to status is required, else nil.
 // The current time is passed in to facilitate testing.
 // It has no receiver, to facilitate testing.
 func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControlInterface, sjc sjControlInterface, recorder record.EventRecorder) {
-	nameForLog := fmt.Sprintf("namespace/%s/scheduledJob/%s", sj.Namespace, sj.Name)
+	nameForLog := fmt.Sprintf("%s/%s", sj.Namespace, sj.Name)
 
-	glog.V(4).Infof("Not starting job for %s because it is suspended", nameForLog)
 	for _, j := range js {
 		found := inActiveList(sj, j.ObjectMeta.UID)
 		if !found {
@@ -140,7 +139,7 @@ func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControl
 			// in the same namespace "adopt" that job.  ReplicaSets and their Pods work the same way.
 			// TBS: how to update sj.Status.LastScheduleTime if the adopted job is newer than any we knew about?
 		} else {
-			if isJobActive(&j) {
+			if job.IsJobFinished(&j) {
 				deleteFromActiveList(&sj, j.ObjectMeta.UID)
 				// TODO: event to call out failure vs success.
 				recorder.Eventf(&sj, api.EventTypeNormal, "SawCompletedJob", "Saw completed job: %v", j.Name)
@@ -152,7 +151,7 @@ func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControl
 		glog.Errorf("Unable to update status for %s: %v", nameForLog, err)
 	}
 
-	if sj.Spec.Suspend {
+	if sj.Spec.Suspend != nil && *sj.Spec.Suspend {
 		glog.V(4).Infof("Not starting job for %s because it is suspended", nameForLog)
 		return
 	}
@@ -200,7 +199,6 @@ func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControl
 		return
 	}
 	if sj.Spec.ConcurrencyPolicy == batch.ReplaceConcurrent {
-		glog.Errorf("Not starting job for %s because of prior execution still running and concurrency policy is Replace and delete is not supported yet", nameForLog)
 		for _, j := range sj.Status.Active {
 			glog.V(4).Infof("Deleting job %s of %s s that was still running at next scheduled start time", j.Name, nameForLog)
 			if err := jc.DeleteJob(j.Namespace, j.Name); err != nil {
@@ -231,7 +229,7 @@ func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControl
 	// iteration of SyncAll, we might not see our own status update, and
 	// then post one again.  So, we need to use the job name as a lock to
 	// prevent us from making the job twice.  TODO: name the job
-	// deterministically.
+	// deterministically (via hash of its scheduled time).
 
 	// Add the just-started job to the status list.
 	ref, err := getRef(jobResp)
@@ -240,7 +238,7 @@ func SyncOne(sj batch.ScheduledJob, js []batch.Job, now time.Time, jc jobControl
 	} else {
 		sj.Status.Active = append(sj.Status.Active, *ref)
 	}
-	sj.Status.LastScheduleTime = &unversioned.Time{scheduledTime}
+	sj.Status.LastScheduleTime = &unversioned.Time{Time: scheduledTime}
 	if err := sjc.UpdateStatus(&sj); err != nil {
 		glog.Infof("Unable to update status for %s: %v", nameForLog, err)
 	}

pkg/controller/scheduledjob/doc.go

 /*
-Copyright 2015 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/controller/scheduledjob/injection.go

 /*
-Copyright 2016 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/controller/scheduledjob/utils.go

 /*
-Copyright 2016 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -119,9 +119,9 @@ func getNextStartTimeAfter(schedule string, now time.Time) (time.Time, error) {
 	return sched.Next(now), nil
 }
 
-// getRecentUnmetScheduleTimes gets a slice of times that have passed when a Job should have started but did not
+// getRecentUnmetScheduleTimes gets a slice of times (from oldest to latest) that have passed when a Job should have started but did not.
 //
-// If there are too many unstarted times, only the most recent may be returned.
+// If there are too many (>100) unstarted times, just give up and return an empty slice.
 // If there were missed times prior to the last known start time, then those are not returned.
 func getRecentUnmetScheduleTimes(sj batch.ScheduledJob, now time.Time) ([]time.Time, error) {
 	starts := []time.Time{}
@@ -166,25 +166,13 @@ func getRecentUnmetScheduleTimes(sj batch.ScheduledJob, now time.Time) ([]time.T
 		// I've somewhat arbitrarily picked 100, as more than 80, but
 		// but less than "lots".
 		if len(starts) > 100 {
+			// We can't get the most recent times so just return an empty slice
 			return []time.Time{}, fmt.Errorf("Too many missed start times to list")
 		}
 	}
 	return starts, nil
 }
 
-func isJobActive(j *batch.Job) bool {
-	return !isJobFinished(j)
-}
-
-func isJobFinished(j *batch.Job) bool {
-	for _, c := range j.Status.Conditions {
-		if (c.Type == batch.JobComplete || c.Type == batch.JobFailed) && c.Status == api.ConditionTrue {
-			return true
-		}
-	}
-	return false
-}
-
 // XXX unit test this
 
 // getJobFromTemplate makes a Job from a ScheduledJob

pkg/controller/scheduledjob/utils_test.go

 /*
-Copyright 2015 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -283,7 +283,7 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	{
 		// Case 1: no known start times, and none needed yet.
 		// Creation time is before T1.
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-10 * time.Minute)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-10 * time.Minute)}
 		// Current time is more than creation time, but less than T1.
 		now := T1.Add(-7 * time.Minute)
 		times, err := getRecentUnmetScheduleTimes(sj, now)
@@ -297,7 +297,7 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	{
 		// Case 2: no known start times, and one needed.
 		// Creation time is before T1.
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-10 * time.Minute)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-10 * time.Minute)}
 		// Current time is after T1
 		now := T1.Add(2 * time.Second)
 		times, err := getRecentUnmetScheduleTimes(sj, now)
@@ -313,9 +313,9 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	{
 		// Case 3: known LastScheduleTime, no start needed.
 		// Creation time is before T1.
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-10 * time.Minute)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-10 * time.Minute)}
 		// Status shows a start at the expected time.
-		sj.Status.LastScheduleTime = &unversioned.Time{T1}
+		sj.Status.LastScheduleTime = &unversioned.Time{Time: T1}
 		// Current time is after T1
 		now := T1.Add(2 * time.Minute)
 		times, err := getRecentUnmetScheduleTimes(sj, now)
@@ -329,9 +329,9 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	{
 		// Case 4: known LastScheduleTime, a start needed
 		// Creation time is before T1.
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-10 * time.Minute)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-10 * time.Minute)}
 		// Status shows a start at the expected time.
-		sj.Status.LastScheduleTime = &unversioned.Time{T1}
+		sj.Status.LastScheduleTime = &unversioned.Time{Time: T1}
 		// Current time is after T1 and after T2
 		now := T2.Add(5 * time.Minute)
 		times, err := getRecentUnmetScheduleTimes(sj, now)
@@ -346,8 +346,8 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	}
 	{
 		// Case 5: known LastScheduleTime, two starts needed
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-2 * time.Hour)}
-		sj.Status.LastScheduleTime = &unversioned.Time{T1.Add(-1 * time.Hour)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-2 * time.Hour)}
+		sj.Status.LastScheduleTime = &unversioned.Time{Time: T1.Add(-1 * time.Hour)}
 		// Current time is after T1 and after T2
 		now := T2.Add(5 * time.Minute)
 		times, err := getRecentUnmetScheduleTimes(sj, now)
@@ -367,8 +367,8 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	}
 	{
 		// Case 6: now is way way ahead of last start time.
-		sj.ObjectMeta.CreationTimestamp = unversioned.Time{T1.Add(-2 * time.Hour)}
-		sj.Status.LastScheduleTime = &unversioned.Time{T1.Add(-1 * time.Hour)}
+		sj.ObjectMeta.CreationTimestamp = unversioned.Time{Time: T1.Add(-2 * time.Hour)}
+		sj.Status.LastScheduleTime = &unversioned.Time{Time: T1.Add(-1 * time.Hour)}
 		now := T2.Add(10 * 24 * time.Hour)
 		_, err := getRecentUnmetScheduleTimes(sj, now)
 		if err == nil {
@@ -377,28 +377,3 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) {
 	}
 
 }
-
-func TestIsJobFinished(t *testing.T) {
-	job := &batch.Job{
-		Status: batch.JobStatus{
-			Conditions: []batch.JobCondition{{
-				Type:   batch.JobComplete,
-				Status: api.ConditionTrue,
-			}},
-		},
-	}
-
-	if !isJobFinished(job) {
-		t.Error("Job was expected to be finished")
-	}
-
-	job.Status.Conditions[0].Status = api.ConditionFalse
-	if isJobFinished(job) {
-		t.Error("Job was not expected to be finished")
-	}
-
-	job.Status.Conditions[0].Status = api.ConditionUnknown
-	if isJobFinished(job) {
-		t.Error("Job was not expected to be finished")
-	}
-}

pkg/kubectl/cmd/util/clientcache.go

@@ -17,8 +17,6 @@ limitations under the License.
 package util
 
 import (
-	"fmt"
-
 	fed_clientset "k8s.io/kubernetes/federation/client/clientset_generated/federation_internalclientset"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
@@ -84,7 +82,6 @@ func (c *ClientCache) ClientConfigForVersion(version *unversioned.GroupVersion)
 		return nil, err
 	}
 	config.GroupVersion = negotiatedVersion
-	fmt.Printf("  Negotiated version %v\n", negotiatedVersion)
 
 	if version != nil {
 		c.configs[*version] = &config

pkg/kubectl/cmd/util/factory.go

@@ -352,7 +352,6 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory {
 		},
 		ClientForMapping: func(mapping *meta.RESTMapping) (resource.RESTClient, error) {
 			cfg, err := clientConfig.ClientConfig()
-			fmt.Printf("Mapping version: %#v", mappingVersion)
 			if err != nil {
 				return nil, err
 			}