Merge pull request #10234 from mikedanese/rolling-update-weird

Detect if replicas have been defaulted in rolling-update and use old replica count

examples/update-demo/kitten-rc.yaml

@@ -3,7 +3,6 @@ kind: ReplicationController
 metadata:
   name: update-demo-kitten
 spec:
-  replicas: 4
   selector:
     name: update-demo
     version: kitten

pkg/api/v1/types.go

@@ -971,7 +971,7 @@ type PodTemplateList struct {
 
 // ReplicationControllerSpec is the specification of a replication controller.
 type ReplicationControllerSpec struct {
-	// Replicas is the number of desired replicas.
+	// Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified.
 	Replicas *int `json:"replicas,omitempty" description:"number of replicas desired; defaults to 1"`
 
 	// Selector is a label query over pods that should match the Replicas count.

pkg/api/v1beta3/conversion.go

@@ -41,6 +41,8 @@ func addConversionFuncs() {
 		convert_api_StatusDetails_To_v1beta3_StatusDetails,
 		convert_v1beta3_StatusCause_To_api_StatusCause,
 		convert_api_StatusCause_To_v1beta3_StatusCause,
+		convert_api_ReplicationControllerSpec_To_v1beta3_ReplicationControllerSpec,
+		convert_v1beta3_ReplicationControllerSpec_To_api_ReplicationControllerSpec,
 	)
 	if err != nil {
 		// If one of the conversion functions is malformed, detect it immediately.
@@ -726,3 +728,51 @@ func convert_api_StatusCause_To_v1beta3_StatusCause(in *api.StatusCause, out *St
 	out.Field = in.Field
 	return nil
 }
+
+func convert_api_ReplicationControllerSpec_To_v1beta3_ReplicationControllerSpec(in *api.ReplicationControllerSpec, out *ReplicationControllerSpec, s conversion.Scope) error {
+	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
+		defaulting.(func(*api.ReplicationControllerSpec))(in)
+	}
+	out.Replicas = &in.Replicas
+	if in.Selector != nil {
+		out.Selector = make(map[string]string)
+		for key, val := range in.Selector {
+			out.Selector[key] = val
+		}
+	} else {
+		out.Selector = nil
+	}
+	if in.Template != nil {
+		out.Template = new(PodTemplateSpec)
+		if err := convert_api_PodTemplateSpec_To_v1beta3_PodTemplateSpec(in.Template, out.Template, s); err != nil {
+			return err
+		}
+	} else {
+		out.Template = nil
+	}
+	return nil
+}
+
+func convert_v1beta3_ReplicationControllerSpec_To_api_ReplicationControllerSpec(in *ReplicationControllerSpec, out *api.ReplicationControllerSpec, s conversion.Scope) error {
+	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
+		defaulting.(func(*ReplicationControllerSpec))(in)
+	}
+	out.Replicas = *in.Replicas
+	if in.Selector != nil {
+		out.Selector = make(map[string]string)
+		for key, val := range in.Selector {
+			out.Selector[key] = val
+		}
+	} else {
+		out.Selector = nil
+	}
+	if in.Template != nil {
+		out.Template = new(api.PodTemplateSpec)
+		if err := convert_v1beta3_PodTemplateSpec_To_api_PodTemplateSpec(in.Template, out.Template, s); err != nil {
+			return err
+		}
+	} else {
+		out.Template = nil
+	}
+	return nil
+}

pkg/api/v1beta3/conversion_generated.go

@@ -1537,30 +1537,6 @@ func convert_api_ReplicationControllerList_To_v1beta3_ReplicationControllerList(
 	return nil
 }
 
-func convert_api_ReplicationControllerSpec_To_v1beta3_ReplicationControllerSpec(in *api.ReplicationControllerSpec, out *ReplicationControllerSpec, s conversion.Scope) error {
-	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
-		defaulting.(func(*api.ReplicationControllerSpec))(in)
-	}
-	out.Replicas = in.Replicas
-	if in.Selector != nil {
-		out.Selector = make(map[string]string)
-		for key, val := range in.Selector {
-			out.Selector[key] = val
-		}
-	} else {
-		out.Selector = nil
-	}
-	if in.Template != nil {
-		out.Template = new(PodTemplateSpec)
-		if err := convert_api_PodTemplateSpec_To_v1beta3_PodTemplateSpec(in.Template, out.Template, s); err != nil {
-			return err
-		}
-	} else {
-		out.Template = nil
-	}
-	return nil
-}
-
 func convert_api_ReplicationControllerStatus_To_v1beta3_ReplicationControllerStatus(in *api.ReplicationControllerStatus, out *ReplicationControllerStatus, s conversion.Scope) error {
 	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
 		defaulting.(func(*api.ReplicationControllerStatus))(in)
@@ -3603,30 +3579,6 @@ func convert_v1beta3_ReplicationControllerList_To_api_ReplicationControllerList(
 	return nil
 }
 
-func convert_v1beta3_ReplicationControllerSpec_To_api_ReplicationControllerSpec(in *ReplicationControllerSpec, out *api.ReplicationControllerSpec, s conversion.Scope) error {
-	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
-		defaulting.(func(*ReplicationControllerSpec))(in)
-	}
-	out.Replicas = in.Replicas
-	if in.Selector != nil {
-		out.Selector = make(map[string]string)
-		for key, val := range in.Selector {
-			out.Selector[key] = val
-		}
-	} else {
-		out.Selector = nil
-	}
-	if in.Template != nil {
-		out.Template = new(api.PodTemplateSpec)
-		if err := convert_v1beta3_PodTemplateSpec_To_api_PodTemplateSpec(in.Template, out.Template, s); err != nil {
-			return err
-		}
-	} else {
-		out.Template = nil
-	}
-	return nil
-}
-
 func convert_v1beta3_ReplicationControllerStatus_To_api_ReplicationControllerStatus(in *ReplicationControllerStatus, out *api.ReplicationControllerStatus, s conversion.Scope) error {
 	if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {
 		defaulting.(func(*ReplicationControllerStatus))(in)
@@ -4240,7 +4192,6 @@ func init() {
 		convert_api_RBDVolumeSource_To_v1beta3_RBDVolumeSource,
 		convert_api_RangeAllocation_To_v1beta3_RangeAllocation,
 		convert_api_ReplicationControllerList_To_v1beta3_ReplicationControllerList,
-		convert_api_ReplicationControllerSpec_To_v1beta3_ReplicationControllerSpec,
 		convert_api_ReplicationControllerStatus_To_v1beta3_ReplicationControllerStatus,
 		convert_api_ReplicationController_To_v1beta3_ReplicationController,
 		convert_api_ResourceQuotaList_To_v1beta3_ResourceQuotaList,
@@ -4347,7 +4298,6 @@ func init() {
 		convert_v1beta3_RBDVolumeSource_To_api_RBDVolumeSource,
 		convert_v1beta3_RangeAllocation_To_api_RangeAllocation,
 		convert_v1beta3_ReplicationControllerList_To_api_ReplicationControllerList,
-		convert_v1beta3_ReplicationControllerSpec_To_api_ReplicationControllerSpec,
 		convert_v1beta3_ReplicationControllerStatus_To_api_ReplicationControllerStatus,
 		convert_v1beta3_ReplicationController_To_api_ReplicationController,
 		convert_v1beta3_ResourceQuotaList_To_api_ResourceQuotaList,

pkg/api/v1beta3/deep_copy_generated.go

@@ -1514,7 +1514,12 @@ func deepCopy_v1beta3_ReplicationControllerList(in ReplicationControllerList, ou
 }
 
 func deepCopy_v1beta3_ReplicationControllerSpec(in ReplicationControllerSpec, out *ReplicationControllerSpec, c *conversion.Cloner) error {
-	out.Replicas = in.Replicas
+	if in.Replicas != nil {
+		out.Replicas = new(int)
+		*out.Replicas = *in.Replicas
+	} else {
+		out.Replicas = nil
+	}
 	if in.Selector != nil {
 		out.Selector = make(map[string]string)
 		for key, val := range in.Selector {

pkg/api/v1beta3/defaults.go

@@ -40,6 +40,10 @@ func addDefaultingFuncs() {
 					obj.Labels = labels
 				}
 			}
+			if obj.Spec.Replicas == nil {
+				obj.Spec.Replicas = new(int)
+				*obj.Spec.Replicas = 0
+			}
 		},
 		func(obj *Volume) {
 			if util.AllPtrFieldsNil(&obj.VolumeSource) {

pkg/api/v1beta3/types.go

@@ -975,8 +975,8 @@ type PodTemplateList struct {
 
 // ReplicationControllerSpec is the specification of a replication controller.
 type ReplicationControllerSpec struct {
-	// Replicas is the number of desired replicas.
-	Replicas int `json:"replicas,omitempty" description:"number of replicas desired"`
+	// Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified.
+	Replicas *int `json:"replicas,omitempty" description:"number of replicas desired"`
 
 	// Selector is a label query over pods that should match the Replicas count.
 	// If Selector is empty, it is defaulted to the labels present on the Pod template.

pkg/conversion/decode.go

@@ -22,31 +22,38 @@ import (
 	"fmt"
 )
 
+func (s *Scheme) DecodeToVersionedObject(data []byte) (obj interface{}, version, kind string, err error) {
+	version, kind, err = s.DataVersionAndKind(data)
+	if err != nil {
+		return
+	}
+	if version == "" && s.InternalVersion != "" {
+		return nil, "", "", fmt.Errorf("version not set in '%s'", string(data))
+	}
+	if kind == "" {
+		return nil, "", "", fmt.Errorf("kind not set in '%s'", string(data))
+	}
+	obj, err = s.NewObject(version, kind)
+	if err != nil {
+		return nil, "", "", err
+	}
+
+	if err := json.Unmarshal(data, obj); err != nil {
+		return nil, "", "", err
+	}
+	return
+}
+
 // Decode converts a JSON string back into a pointer to an api object.
 // Deduces the type based upon the fields added by the MetaInsertionFactory
 // technique. The object will be converted, if necessary, into the
 // s.InternalVersion type before being returned. Decode will not decode
 // objects without version set unless InternalVersion is also "".
 func (s *Scheme) Decode(data []byte) (interface{}, error) {
-	version, kind, err := s.DataVersionAndKind(data)
+	obj, version, kind, err := s.DecodeToVersionedObject(data)
 	if err != nil {
 		return nil, err
 	}
-	if version == "" && s.InternalVersion != "" {
-		return nil, fmt.Errorf("version not set in '%s'", string(data))
-	}
-	if kind == "" {
-		return nil, fmt.Errorf("kind not set in '%s'", string(data))
-	}
-	obj, err := s.NewObject(version, kind)
-	if err != nil {
-		return nil, err
-	}
-
-	if err := json.Unmarshal(data, obj); err != nil {
-		return nil, err
-	}
-
 	// Version and Kind should be blank in memory.
 	if err := s.SetVersionAndKind("", "", obj); err != nil {
 		return nil, err

pkg/kubectl/cmd/rollingupdate.go

@@ -26,6 +26,8 @@ import (
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl"
 	cmdutil "github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/util"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/resource"
@@ -143,6 +145,7 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg
 	}
 
 	var keepOldName bool
+	var replicasDefaulted bool
 
 	mapper, typer := f.Object()
 
@@ -151,12 +154,12 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg
 		if err != nil {
 			return err
 		}
-		obj, err := resource.NewBuilder(mapper, typer, f.ClientMapperForCommand()).
+		request := resource.NewBuilder(mapper, typer, f.ClientMapperForCommand()).
 			Schema(schema).
 			NamespaceParam(cmdNamespace).RequireNamespace().
 			FilenameParam(filename).
-			Do().
-			Object()
+			Do()
+		obj, err := request.Object()
 		if err != nil {
 			return err
 		}
@@ -177,6 +180,12 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg
 			glog.V(4).Infof("Object %#v is not a ReplicationController", obj)
 			return cmdutil.UsageError(cmd, "%s does not specify a valid ReplicationController", filename)
 		}
+		infos, err := request.Infos()
+		if err != nil || len(infos) != 1 {
+			glog.V(2).Infof("was not able to recover adequate information to discover if .spec.replicas was defaulted")
+		} else {
+			replicasDefaulted = isReplicasDefaulted(infos[0])
+		}
 	}
 	// If the --image option is specified, we need to create a new rc with at least one different selector
 	// than the old rc. This selector is the hash of the rc, which will differ because the new rc has a
@@ -228,7 +237,7 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg
 			filename, oldName)
 	}
 	// TODO: handle scales during rolling update
-	if newRc.Spec.Replicas == 0 {
+	if replicasDefaulted {
 		newRc.Spec.Replicas = oldRc.Spec.Replicas
 	}
 	if dryrun {
@@ -283,3 +292,21 @@ func findNewName(args []string, oldRc *api.ReplicationController) string {
 	}
 	return ""
 }
+
+func isReplicasDefaulted(info *resource.Info) bool {
+	if info == nil || info.VersionedObject == nil {
+		// was unable to recover versioned info
+		return false
+	}
+	switch info.Mapping.APIVersion {
+	case "v1":
+		if rc, ok := info.VersionedObject.(*v1.ReplicationController); ok {
+			return rc.Spec.Replicas == nil
+		}
+	case "v1beta3":
+		if rc, ok := info.VersionedObject.(*v1beta3.ReplicationController); ok {
+			return rc.Spec.Replicas == nil
+		}
+	}
+	return false
+}

pkg/kubectl/resource/mapper.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util/yaml"
@@ -64,13 +65,19 @@ func (m *Mapper) InfoForData(data []byte, source string) (*Info, error) {
 	name, _ := mapping.MetadataAccessor.Name(obj)
 	namespace, _ := mapping.MetadataAccessor.Namespace(obj)
 	resourceVersion, _ := mapping.MetadataAccessor.ResourceVersion(obj)
+
+	var versionedObject interface{}
+
+	if vo, _, _, err := api.Scheme.Raw().DecodeToVersionedObject(data); err == nil {
+		versionedObject = vo
+	}
 	return &Info{
 		Mapping:         mapping,
 		Client:          client,
 		Namespace:       namespace,
 		Name:            name,
 		Source:          source,
-
+		VersionedObject: versionedObject,
 		Object:          obj,
 		ResourceVersion: resourceVersion,
 	}, nil

pkg/kubectl/resource/visitor.go

@@ -69,6 +69,10 @@ type Info struct {
 	// Optional, Source is the filename or URL to template file (.json or .yaml),
 	// or stdin to use to handle the resource
 	Source string
+	// Optional, this is the provided object in a versioned type before defaulting
+	// and conversions into its corresponding internal type. This is useful for
+	// reflecting on user intent which may be lost after defaulting and conversions.
+	VersionedObject interface{}
 	// Optional, this is the most recent value returned by the server if available
 	runtime.Object
 	// Optional, this is the most recent resource version the server knows about for

test/e2e/kubectl.go

@@ -101,8 +101,7 @@ var _ = Describe("Kubectl client", func() {
 			validateController(c, nautilusImage, 2, "update-demo", updateDemoSelector, getUDData("nautilus.jpg", ns), ns)
 			By("rolling-update to new replication controller")
 			runKubectl("rolling-update", "update-demo-nautilus", "--update-period=1s", "-f", kittenPath, fmt.Sprintf("--namespace=%v", ns))
-			// TODO: revisit the expected replicas once #9645 is resolved
-			validateController(c, kittenImage, 4, "update-demo", updateDemoSelector, getUDData("kitten.jpg", ns), ns)
+			validateController(c, kittenImage, 2, "update-demo", updateDemoSelector, getUDData("kitten.jpg", ns), ns)
 			// Everything will hopefully be cleaned up when the namespace is deleted.
 		})
 	})