Clients should not check conditions, UpdateStatus() is inconsistent

Corrects some of the cargo culting that has crept into the API.

pkg/client/client_test.go

@@ -35,7 +35,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/version"
 )
 
-const nameRequiredError = "name is required parameter to Get"
+const nameRequiredError = "resource name may not be empty"
 
 type testRequest struct {
 	Method  string

pkg/client/endpoints.go

@@ -17,7 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -72,10 +71,6 @@ func (c *endpoints) List(selector labels.Selector) (result *api.EndpointsList, e
 
 // Get returns information about the endpoints for a particular service.
 func (c *endpoints) Get(name string) (result *api.Endpoints, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.Endpoints{}
 	err = c.r.Get().Namespace(c.ns).Resource("endpoints").Name(name).Do().Into(result)
 	return

pkg/client/events.go

@@ -17,7 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -114,10 +113,6 @@ func (e *events) List(label labels.Selector, field fields.Selector) (*api.EventL
 
 // Get returns the given event, or an error.
 func (e *events) Get(name string) (*api.Event, error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result := &api.Event{}
 	err := e.client.Get().
 		NamespaceIfScoped(e.namespace, len(e.namespace) > 0).

pkg/client/fake_pods.go

@@ -65,7 +65,7 @@ func (c *FakePods) Bind(bind *api.Binding) error {
 	return nil
 }
 
-func (c *FakePods) UpdateStatus(name string, status *api.PodStatus) (*api.Pod, error) {
+func (c *FakePods) UpdateStatus(pod *api.Pod) (*api.Pod, error) {
-	c.Fake.Actions = append(c.Fake.Actions, FakeAction{Action: "update-status-pod", Value: name})
+	c.Fake.Actions = append(c.Fake.Actions, FakeAction{Action: "update-status-pod", Value: pod.Name})
 	return &api.Pod{}, nil
 }

pkg/client/fake_resource_quotas.go

@@ -55,7 +55,7 @@ func (c *FakeResourceQuotas) Update(resourceQuota *api.ResourceQuota) (*api.Reso
 	return &api.ResourceQuota{}, nil
 }
 
-func (c *FakeResourceQuotas) Status(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error) {
+func (c *FakeResourceQuotas) UpdateStatus(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error) {
 	c.Fake.Actions = append(c.Fake.Actions, FakeAction{Action: "update-status-resourceQuota", Value: resourceQuota.Name})
 	c.Fake.ResourceQuotaStatus = *resourceQuota
 	return &api.ResourceQuota{}, nil

pkg/client/limit_ranges.go

@@ -17,7 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -64,10 +63,6 @@ func (c *limitRanges) List(selector labels.Selector) (result *api.LimitRangeList
 
 // Get takes the name of the limitRange, and returns the corresponding Pod object, and an error if it occurs
 func (c *limitRanges) Get(name string) (result *api.LimitRange, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.LimitRange{}
 	err = c.r.Get().Namespace(c.ns).Resource("limitRanges").Name(name).Do().Into(result)
 	return

pkg/client/minions.go

@@ -17,7 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -74,10 +73,6 @@ func (c *nodes) List() (*api.NodeList, error) {
 
 // Get gets an existing node.
 func (c *nodes) Get(name string) (*api.Node, error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result := &api.Node{}
 	err := c.r.Get().Resource(c.resourceName()).Name(name).Do().Into(result)
 	return result, err

pkg/client/namespaces.go

@@ -17,7 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -104,10 +103,6 @@ func (c *namespaces) Status(namespace *api.Namespace) (result *api.Namespace, er
 
 // Get gets an existing namespace
 func (c *namespaces) Get(name string) (*api.Namespace, error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result := &api.Namespace{}
 	err := c.r.Get().Resource("namespaces").Name(name).Do().Into(result)
 	return result, err

pkg/client/pods.go

@@ -17,9 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
-	"fmt"
-
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
@@ -40,7 +37,7 @@ type PodInterface interface {
 	Update(pod *api.Pod) (*api.Pod, error)
 	Watch(label labels.Selector, field fields.Selector, resourceVersion string) (watch.Interface, error)
 	Bind(binding *api.Binding) error
-	UpdateStatus(name string, status *api.PodStatus) (*api.Pod, error)
+	UpdateStatus(pod *api.Pod) (*api.Pod, error)
 }
 
 // pods implements PodsNamespacer interface
@@ -66,10 +63,6 @@ func (c *pods) List(selector labels.Selector) (result *api.PodList, err error) {
 
 // Get takes the name of the pod, and returns the corresponding Pod object, and an error if it occurs
 func (c *pods) Get(name string) (result *api.Pod, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.Pod{}
 	err = c.r.Get().Namespace(c.ns).Resource("pods").Name(name).Do().Into(result)
 	return
@@ -90,10 +83,6 @@ func (c *pods) Create(pod *api.Pod) (result *api.Pod, err error) {
 // Update takes the representation of a pod to update.  Returns the server's representation of the pod, and an error, if it occurs.
 func (c *pods) Update(pod *api.Pod) (result *api.Pod, err error) {
 	result = &api.Pod{}
-	if len(pod.ResourceVersion) == 0 {
-		err = fmt.Errorf("invalid update object, missing resource version: %v", pod)
-		return
-	}
 	err = c.r.Put().Namespace(c.ns).Resource("pods").Name(pod.Name).Body(pod).Do().Into(result)
 	return
 }
@@ -116,13 +105,8 @@ func (c *pods) Bind(binding *api.Binding) error {
 }
 
 // UpdateStatus takes the name of the pod and the new status.  Returns the server's representation of the pod, and an error, if it occurs.
-func (c *pods) UpdateStatus(name string, newStatus *api.PodStatus) (result *api.Pod, err error) {
+func (c *pods) UpdateStatus(pod *api.Pod) (result *api.Pod, err error) {
 	result = &api.Pod{}
-	pod, err := c.Get(name)
-	if err != nil {
-		return
-	}
-	pod.Status = *newStatus
 	err = c.r.Put().Namespace(c.ns).Resource("pods").Name(pod.Name).SubResource("status").Body(pod).Do().Into(result)
 	return
 }

pkg/client/replication_controllers.go

@@ -17,9 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
-	"fmt"
-
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
@@ -61,10 +58,6 @@ func (c *replicationControllers) List(selector labels.Selector) (result *api.Rep
 
 // Get returns information about a particular replication controller.
 func (c *replicationControllers) Get(name string) (result *api.ReplicationController, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.ReplicationController{}
 	err = c.r.Get().Namespace(c.ns).Resource("replicationControllers").Name(name).Do().Into(result)
 	return
@@ -80,10 +73,6 @@ func (c *replicationControllers) Create(controller *api.ReplicationController) (
 // Update updates an existing replication controller.
 func (c *replicationControllers) Update(controller *api.ReplicationController) (result *api.ReplicationController, err error) {
 	result = &api.ReplicationController{}
-	if len(controller.ResourceVersion) == 0 {
-		err = fmt.Errorf("invalid update object, missing resource version: %v", controller)
-		return
-	}
 	err = c.r.Put().Namespace(c.ns).Resource("replicationControllers").Name(controller.Name).Body(controller).Do().Into(result)
 	return
 }

pkg/client/request.go

@@ -190,6 +190,10 @@ func (r *Request) Name(resourceName string) *Request {
 	if r.err != nil {
 		return r
 	}
+	if len(resourceName) == 0 {
+		r.err = fmt.Errorf("resource name may not be empty")
+		return r
+	}
 	if len(r.resourceName) != 0 {
 		r.err = fmt.Errorf("resource name already set to %q, cannot change to %q", r.resourceName, resourceName)
 		return r

pkg/client/resource_quotas.go

@@ -17,9 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
-	"fmt"
-
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
@@ -38,7 +35,7 @@ type ResourceQuotaInterface interface {
 	Delete(name string) error
 	Create(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error)
 	Update(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error)
-	Status(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error)
+	UpdateStatus(resourceQuota *api.ResourceQuota) (*api.ResourceQuota, error)
 	Watch(label labels.Selector, field fields.Selector, resourceVersion string) (watch.Interface, error)
 }
 
@@ -65,10 +62,6 @@ func (c *resourceQuotas) List(selector labels.Selector) (result *api.ResourceQuo
 
 // Get takes the name of the resourceQuota, and returns the corresponding ResourceQuota object, and an error if it occurs
 func (c *resourceQuotas) Get(name string) (result *api.ResourceQuota, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.ResourceQuota{}
 	err = c.r.Get().Namespace(c.ns).Resource("resourceQuotas").Name(name).Do().Into(result)
 	return
@@ -89,21 +82,13 @@ func (c *resourceQuotas) Create(resourceQuota *api.ResourceQuota) (result *api.R
 // Update takes the representation of a resourceQuota to update spec.  Returns the server's representation of the resourceQuota, and an error, if it occurs.
 func (c *resourceQuotas) Update(resourceQuota *api.ResourceQuota) (result *api.ResourceQuota, err error) {
 	result = &api.ResourceQuota{}
-	if len(resourceQuota.ResourceVersion) == 0 {
-		err = fmt.Errorf("invalid update object, missing resource version: %v", resourceQuota)
-		return
-	}
 	err = c.r.Put().Namespace(c.ns).Resource("resourceQuotas").Name(resourceQuota.Name).Body(resourceQuota).Do().Into(result)
 	return
 }
 
 // Status takes the representation of a resourceQuota to update status.  Returns the server's representation of the resourceQuota, and an error, if it occurs.
-func (c *resourceQuotas) Status(resourceQuota *api.ResourceQuota) (result *api.ResourceQuota, err error) {
+func (c *resourceQuotas) UpdateStatus(resourceQuota *api.ResourceQuota) (result *api.ResourceQuota, err error) {
 	result = &api.ResourceQuota{}
-	if len(resourceQuota.ResourceVersion) == 0 {
-		err = fmt.Errorf("invalid update object, missing resource version: %v", resourceQuota)
-		return
-	}
 	err = c.r.Put().Namespace(c.ns).Resource("resourceQuotas").Name(resourceQuota.Name).SubResource("status").Body(resourceQuota).Do().Into(result)
 	return
 }

pkg/client/resource_quotas_test.go

@@ -174,38 +174,10 @@ func TestResourceQuotaStatusUpdate(t *testing.T) {
 			Query:  buildQueryValues(ns, nil)},
 		Response: Response{StatusCode: 200, Body: resourceQuota},
 	}
-	response, err := c.Setup().ResourceQuotas(ns).Status(resourceQuota)
+	response, err := c.Setup().ResourceQuotas(ns).UpdateStatus(resourceQuota)
 	c.Validate(t, response, err)
 }
 
-func TestInvalidResourceQuotaUpdate(t *testing.T) {
-	ns := api.NamespaceDefault
-	resourceQuota := &api.ResourceQuota{
-		ObjectMeta: api.ObjectMeta{
-			Name:      "abc",
-			Namespace: "foo",
-		},
-		Spec: api.ResourceQuotaSpec{
-			Hard: api.ResourceList{
-				api.ResourceCPU:                    resource.MustParse("100"),
-				api.ResourceMemory:                 resource.MustParse("10000"),
-				api.ResourcePods:                   resource.MustParse("10"),
-				api.ResourceServices:               resource.MustParse("10"),
-				api.ResourceReplicationControllers: resource.MustParse("10"),
-				api.ResourceQuotas:                 resource.MustParse("10"),
-			},
-		},
-	}
-	c := &testClient{
-		Request:  testRequest{Method: "PUT", Path: testapi.ResourcePath(getResourceQuotasResoureName(), ns, "abc"), Query: buildQueryValues(ns, nil)},
-		Response: Response{StatusCode: 200, Body: resourceQuota},
-	}
-	_, err := c.Setup().ResourceQuotas(ns).Update(resourceQuota)
-	if err == nil {
-		t.Errorf("Expected an error due to missing ResourceVersion")
-	}
-}
-
 func TestResourceQuotaDelete(t *testing.T) {
 	ns := api.NamespaceDefault
 	c := &testClient{

pkg/client/secrets.go

@@ -17,9 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
-	"fmt"
-
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
@@ -54,10 +51,6 @@ func newSecrets(c *Client, ns string) *secrets {
 }
 
 func (s *secrets) Create(secret *api.Secret) (*api.Secret, error) {
-	if s.namespace != "" && secret.Namespace != s.namespace {
-		return nil, fmt.Errorf("can't create a secret with namespace '%v' in namespace '%v'", secret.Namespace, s.namespace)
-	}
-
 	result := &api.Secret{}
 	err := s.client.Post().
 		Namespace(secret.Namespace).
@@ -86,10 +79,6 @@ func (s *secrets) List(label labels.Selector, field fields.Selector) (*api.Secre
 
 // Get returns the given secret, or an error.
 func (s *secrets) Get(name string) (*api.Secret, error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result := &api.Secret{}
 	err := s.client.Get().
 		Namespace(s.namespace).
@@ -124,11 +113,6 @@ func (s *secrets) Delete(name string) error {
 
 func (s *secrets) Update(secret *api.Secret) (result *api.Secret, err error) {
 	result = &api.Secret{}
-	if len(secret.ResourceVersion) == 0 {
-		err = fmt.Errorf("invalid update object, missing resource version: %v", secret)
-		return
-	}
-
 	err = s.client.Put().
 		Namespace(s.namespace).
 		Resource("secrets").

pkg/client/services.go

@@ -17,8 +17,6 @@ limitations under the License.
 package client
 
 import (
-	"errors"
-	"fmt"
 	"time"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -71,10 +69,6 @@ func (c *services) List(selector labels.Selector) (result *api.ServiceList, err
 
 // Get returns information about a particular service.
 func (c *services) Get(name string) (result *api.Service, err error) {
-	if len(name) == 0 {
-		return nil, errors.New("name is required parameter to Get")
-	}
-
 	result = &api.Service{}
 	err = c.r.Get().Namespace(c.ns).Resource("services").Name(name).Do().Into(result)
 	return
@@ -83,48 +77,20 @@ func (c *services) Get(name string) (result *api.Service, err error) {
 // Create creates a new service.
 func (c *services) Create(svc *api.Service) (result *api.Service, err error) {
 	result = &api.Service{}
-	// v1beta3 does not allow POST without a namespace.
+	err = c.r.Post().Timeout(extendedTimeout).Namespace(c.ns).Resource("services").Body(svc).Do().Into(result)
-	needNamespace := !api.PreV1Beta3(c.r.APIVersion())
-	namespace := c.ns
-	if needNamespace && len(namespace) == 0 {
-		namespace = api.NamespaceDefault
-	}
-	request := c.r.Post()
-	request.Timeout(extendedTimeout)
-	err = request.Namespace(namespace).Resource("services").Body(svc).Do().Into(result)
 	return
 }
 
 // Update updates an existing service.
 func (c *services) Update(svc *api.Service) (result *api.Service, err error) {
 	result = &api.Service{}
-	if len(svc.ResourceVersion) == 0 {
+	err = c.r.Put().Timeout(extendedTimeout).Timeout(extendedTimeout).Namespace(c.ns).Resource("services").Name(svc.Name).Body(svc).Do().Into(result)
-		err = fmt.Errorf("invalid update object, missing resource version: %v", svc)
-		return
-	}
-	// v1beta3 does not allow PUT without a namespace.
-	needNamespace := !api.PreV1Beta3(c.r.APIVersion())
-	namespace := c.ns
-	if needNamespace && len(namespace) == 0 {
-		namespace = api.NamespaceDefault
-	}
-	request := c.r.Put()
-	request.Timeout(extendedTimeout)
-	err = request.Namespace(namespace).Resource("services").Name(svc.Name).Body(svc).Do().Into(result)
 	return
 }
 
 // Delete deletes an existing service.
 func (c *services) Delete(name string) error {
-	// v1beta3 does not allow DELETE without a namespace.
+	return c.r.Delete().Timeout(extendedTimeout).Namespace(c.ns).Resource("services").Name(name).Do().Error()
-	needNamespace := !api.PreV1Beta3(c.r.APIVersion())
-	namespace := c.ns
-	if needNamespace && len(namespace) == 0 {
-		namespace = api.NamespaceDefault
-	}
-	request := c.r.Delete()
-	request.Timeout(extendedTimeout)
-	return request.Namespace(c.ns).Resource("services").Name(name).Do().Error()
 }
 
 // Watch returns a watch.Interface that watches the requested services.

pkg/kubelet/status_manager.go

@@ -106,16 +106,26 @@ func (s *statusManager) syncBatch() error {
 	podFullName := kubecontainer.GetPodFullName(pod)
 	status := syncRequest.status
 
-	_, err := s.kubeClient.Pods(pod.Namespace).UpdateStatus(pod.Name, &status)
+	var err error
+	statusPod := &api.Pod{
+		ObjectMeta: pod.ObjectMeta,
+	}
+	// TODO: make me easier to express from client code
+	if statusPod, err = s.kubeClient.Pods(statusPod.Namespace).Get(statusPod.Name); err == nil {
+		statusPod.Status = status
+	}
+	if err == nil {
+		statusPod, err = s.kubeClient.Pods(pod.Namespace).UpdateStatus(statusPod)
+		// TODO: handle conflict as a retry, make that easier too.
+	}
 	if err != nil {
 		// We failed to update status. In order to make sure we retry next time
 		// we delete cached value. This may result in an additional update, but
 		// this is ok.
 		s.DeletePodStatus(podFullName)
 		return fmt.Errorf("error updating status for pod %q: %v", pod.Name, err)
-	} else {
-		glog.V(3).Infof("Status for pod %q updated successfully", pod.Name)
 	}
 
+	glog.V(3).Infof("Status for pod %q updated successfully", pod.Name)
 	return nil
 }

pkg/kubelet/status_manager_test.go

@@ -110,5 +110,5 @@ func TestSyncBatch(t *testing.T) {
 	if err != nil {
 		t.Errorf("unexpected syncing error: %v", err)
 	}
-	verifyActions(t, syncer.kubeClient, []string{"update-status-pod"})
+	verifyActions(t, syncer.kubeClient, []string{"get-pod", "update-status-pod"})
 }

pkg/resourcequota/resource_quota_controller.go

@@ -208,7 +208,7 @@ func (rm *ResourceQuotaManager) syncResourceQuota(quota api.ResourceQuota) (err
 
 	// update the usage only if it changed
 	if dirty {
-		_, err = rm.kubeClient.ResourceQuotas(usage.Namespace).Status(&usage)
+		_, err = rm.kubeClient.ResourceQuotas(usage.Namespace).UpdateStatus(&usage)
 		return err
 	}
 	return nil

plugin/pkg/admission/resourcequota/admission.go

@@ -123,7 +123,7 @@ func (q *quota) Admit(a admission.Attributes) (err error) {
 					Annotations:     quota.Annotations},
 			}
 			usage.Status = *status
-			_, err = q.client.ResourceQuotas(usage.Namespace).Status(&usage)
+			_, err = q.client.ResourceQuotas(usage.Namespace).UpdateStatus(&usage)
 			if err != nil {
 				return apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing quota", a.GetOperation(), a.GetResource()))
 			}