From 2906f85227beea5c86872e0d5ca310672a63ffbf Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Mon, 22 Jun 2015 17:52:55 -0700 Subject: [PATCH 1/4] allow conversions.Scheme to expose intermidiate versioned api object --- pkg/conversion/decode.go | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/pkg/conversion/decode.go b/pkg/conversion/decode.go index 2fed94cfc5..6a0577c185 100644 --- a/pkg/conversion/decode.go +++ b/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 From f79736d76771ca7b004ce4c73c5cbfa094d817ca Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Mon, 22 Jun 2015 18:33:15 -0700 Subject: [PATCH 2/4] make replicas a pointer in v1beta3 api --- pkg/api/v1/types.go | 2 +- pkg/api/v1beta3/conversion.go | 50 +++++++++++++++++++++++++ pkg/api/v1beta3/conversion_generated.go | 50 ------------------------- pkg/api/v1beta3/deep_copy_generated.go | 7 +++- pkg/api/v1beta3/defaults.go | 4 ++ pkg/api/v1beta3/types.go | 4 +- 6 files changed, 63 insertions(+), 54 deletions(-) diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index 23f0865190..d28e82ce53 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -953,7 +953,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. diff --git a/pkg/api/v1beta3/conversion.go b/pkg/api/v1beta3/conversion.go index 13ecd4411f..759c9fc0c1 100644 --- a/pkg/api/v1beta3/conversion.go +++ b/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 +} diff --git a/pkg/api/v1beta3/conversion_generated.go b/pkg/api/v1beta3/conversion_generated.go index 73be8ca761..61627f7c9d 100644 --- a/pkg/api/v1beta3/conversion_generated.go +++ b/pkg/api/v1beta3/conversion_generated.go @@ -1536,30 +1536,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) @@ -3601,30 +3577,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) @@ -4238,7 +4190,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, @@ -4345,7 +4296,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, diff --git a/pkg/api/v1beta3/deep_copy_generated.go b/pkg/api/v1beta3/deep_copy_generated.go index ef17355939..67f493f6b4 100644 --- a/pkg/api/v1beta3/deep_copy_generated.go +++ b/pkg/api/v1beta3/deep_copy_generated.go @@ -1513,7 +1513,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 { diff --git a/pkg/api/v1beta3/defaults.go b/pkg/api/v1beta3/defaults.go index 5e8793d331..c7d78bef3e 100644 --- a/pkg/api/v1beta3/defaults.go +++ b/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) { diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index efe404203c..fcb367a6b8 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -957,8 +957,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. From 0c8f71aa0b492bacb9a353a3d97d25019dbedf74 Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Mon, 22 Jun 2015 18:56:53 -0700 Subject: [PATCH 3/4] make rolling update check if the replication controller has been defaulted --- pkg/kubectl/cmd/rollingupdate.go | 35 ++++++++++++++++++++++++++++---- pkg/kubectl/resource/mapper.go | 19 +++++++++++------ pkg/kubectl/resource/visitor.go | 4 ++++ 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/pkg/kubectl/cmd/rollingupdate.go b/pkg/kubectl/cmd/rollingupdate.go index 23443fb919..3d034db057 100644 --- a/pkg/kubectl/cmd/rollingupdate.go +++ b/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 +} diff --git a/pkg/kubectl/resource/mapper.go b/pkg/kubectl/resource/mapper.go index e8a1d6184c..f46c6dc60d 100644 --- a/pkg/kubectl/resource/mapper.go +++ b/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) - return &Info{ - Mapping: mapping, - Client: client, - Namespace: namespace, - Name: name, - Source: source, + 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 diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index 194305fc18..a6013fa978 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/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 From dd07df00ae0724a27cc96217d00307368ce950dc Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Mon, 22 Jun 2015 20:58:48 -0700 Subject: [PATCH 4/4] reenable e2e test --- examples/update-demo/kitten-rc.yaml | 1 - test/e2e/kubectl.go | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/examples/update-demo/kitten-rc.yaml b/examples/update-demo/kitten-rc.yaml index 5644987eae..91f1aa06c3 100644 --- a/examples/update-demo/kitten-rc.yaml +++ b/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 diff --git a/test/e2e/kubectl.go b/test/e2e/kubectl.go index 538eeac4b9..0c98f6badb 100644 --- a/test/e2e/kubectl.go +++ b/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. }) })