Merge pull request #15642 from tnozicka/add-default-for-deploymentcon…

…figs

Automatic merge from submit-queue

Set default for DeploymentConfigSpec.RevisionHistoryLimit in apps/v1

At this point we don't default DeploymentConfigSpec.RevisionHistoryLimit in apps/v1 which is causing the deployments to have unlimited history which will lead to performance issues at scale and when deploying regularly. 

Note that upstream also defaulted RevisionHistoryLimit to 10 for Deployments in kubernetes/kubernetes#49924

Needs to be backported to 3.6.1 as well.

cc: @smarterclayton @mfojtik @Kargakis

api/docs/apis-apps.openshift.io/v1.DeploymentConfig.adoc

@@ -94,7 +94,7 @@ Populated by the system. Read-only. More info: http://kubernetes.io/docs/user-gu
 </summary><div style="margin-left:13px;">  <span title="(integer) MinReadySeconds is the minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)">minReadySeconds</span>:
 </div><div style="margin-left:13px;">  <span title="(boolean) Paused indicates that the deployment config is paused resulting in no new deployments on template changes or changes in the template caused by other triggers.">paused</span>:
 </div><div style="margin-left:13px;">  <span title="(integer) Replicas is the number of desired replicas.">replicas</span>:
-</div><div style="margin-left:13px;">  <span title="(integer) RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.">revisionHistoryLimit</span>:
+</div><div style="margin-left:13px;">  <span title="(integer) RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)">revisionHistoryLimit</span>:
 </div><details><summary>  <span title="(object) Selector is a label query over pods that should match the Replicas count.">selector</span>:
 </summary><div style="margin-left:13px;">    <span title="(string)">[string]</span>:
 </div></details><details><summary>  <span title="(v1.DeploymentStrategy) Strategy describes how a deployment is executed.">strategy</span>:

api/docs/oapi/v1.DeploymentConfig.adoc

@@ -94,7 +94,7 @@ Populated by the system. Read-only. More info: http://kubernetes.io/docs/user-gu
 </summary><div style="margin-left:13px;">  <span title="(integer) MinReadySeconds is the minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)">minReadySeconds</span>:
 </div><div style="margin-left:13px;">  <span title="(boolean) Paused indicates that the deployment config is paused resulting in no new deployments on template changes or changes in the template caused by other triggers.">paused</span>:
 </div><div style="margin-left:13px;">  <span title="(integer) Replicas is the number of desired replicas.">replicas</span>:
-</div><div style="margin-left:13px;">  <span title="(integer) RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.">revisionHistoryLimit</span>:
+</div><div style="margin-left:13px;">  <span title="(integer) RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)">revisionHistoryLimit</span>:
 </div><details><summary>  <span title="(object) Selector is a label query over pods that should match the Replicas count.">selector</span>:
 </summary><div style="margin-left:13px;">    <span title="(string)">[string]</span>:
 </div></details><details><summary>  <span title="(v1.DeploymentStrategy) Strategy describes how a deployment is executed.">strategy</span>:

api/swagger-spec/oapi-v1.json

@@ -23199,7 +23199,7 @@
      "revisionHistoryLimit": {
       "type": "integer",
       "format": "int32",
-      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified."
+      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)"
      },
      "test": {
       "type": "boolean",

api/swagger-spec/openshift-openapi-spec.json

@@ -89392,7 +89392,7 @@
       "format": "int32"
      },
      "revisionHistoryLimit": {
-      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.",
+      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)",
       "type": "integer",
       "format": "int32"
      },

pkg/client/clientset/clientset/clientset.go

@@ -0,0 +1,61 @@
+package clientset
+
+import (
+	discovery "k8s.io/client-go/discovery"
+	rest "k8s.io/client-go/rest"
+	flowcontrol "k8s.io/client-go/util/flowcontrol"
+
+	appsV1 "github.com/openshift/origin/pkg/deploy/generated/clientset/typed/apps/v1"
+)
+
+type Interface interface {
+	AppsV1() appsV1.AppsV1Interface
+}
+
+// Clientset contains the clients for groups. Each group has exactly one
+// version included in a Clientset.
+type Clientset struct {
+	*discovery.DiscoveryClient
+	appsV1 *appsV1.AppsV1Client
+}
+
+// Make sure Clientset implements Interface
+var _ Interface = &Clientset{}
+
+// AppsV1 retrieves the AppsV1Client
+func (c *Clientset) AppsV1() appsV1.AppsV1Interface {
+	return c.appsV1
+}
+
+// NewForConfig creates a new Clientset for the given config.
+func NewForConfig(c *rest.Config) (*Clientset, error) {
+	configShallowCopy := *c
+	if configShallowCopy.RateLimiter == nil && configShallowCopy.QPS > 0 {
+		configShallowCopy.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(configShallowCopy.QPS, configShallowCopy.Burst)
+	}
+
+	var cs Clientset
+	var err error
+
+	cs.DiscoveryClient, err = discovery.NewDiscoveryClientForConfig(&configShallowCopy)
+	if err != nil {
+		return nil, err
+	}
+
+	cs.appsV1, err = appsV1.NewForConfig(&configShallowCopy)
+	if err != nil {
+		return nil, err
+	}
+
+	return &cs, nil
+}
+
+// NewForConfigOrDie creates a new Clientset for the given config and
+// panics if there is an error in the config.
+func NewForConfigOrDie(c *rest.Config) *Clientset {
+	cs, err := NewForConfig(c)
+	if err != nil {
+		panic(err)
+	}
+	return cs
+}

pkg/cmd/server/origin/legacy.go

@@ -7,6 +7,7 @@ import (
 
 	buildconfig "github.com/openshift/origin/pkg/build/registry/buildconfig"
 	buildconfigetcd "github.com/openshift/origin/pkg/build/registry/buildconfig/etcd"
+	"github.com/openshift/origin/pkg/deploy/registry/deployconfig"
 	deploymentconfigetcd "github.com/openshift/origin/pkg/deploy/registry/deployconfig/etcd"
 	routeregistry "github.com/openshift/origin/pkg/route/registry/route"
 	routeetcd "github.com/openshift/origin/pkg/route/registry/route/etcd"
@@ -206,7 +207,8 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 				case "deploymentConfigs":
 					restStorage := s.(*deploymentconfigetcd.REST)
 					store := *restStorage.Store
-					restStorage.DeleteStrategy = orphanByDefault(store.DeleteStrategy)
+					store.CreateStrategy = deployconfig.LegacyStrategy
+					store.DeleteStrategy = deployconfig.LegacyStrategy
 					legacyStorage[resource] = &deploymentconfigetcd.REST{Store: &store}
 				case "routes":
 					restStorage := s.(*routeetcd.REST)
@@ -223,15 +225,3 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 	}
 	return legacyStorage
 }
-
-type orphaningDeleter struct {
-	rest.RESTDeleteStrategy
-}
-
-func (o orphaningDeleter) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
-	return rest.OrphanDependents
-}
-
-func orphanByDefault(in rest.RESTDeleteStrategy) rest.RESTDeleteStrategy {
-	return orphaningDeleter{in}
-}

pkg/deploy/apis/apps/types.go

@@ -20,6 +20,9 @@ const (
 	// This is set as the default value for ActiveDeadlineSeconds for the deployer pod.
 	// Currently set to 6 hours.
 	MaxDeploymentDurationSeconds int64 = 21600
+	// DefaultRevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
+	// This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.
+	DefaultRevisionHistoryLimit int32 = 10
 )
 
 // These constants represent keys used for correlating objects related to deployments.
@@ -156,6 +159,7 @@ type DeploymentConfigSpec struct {
 
 	// RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
 	// This field is a pointer to allow for differentiation between an explicit zero and not specified.
+	// Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)
 	RevisionHistoryLimit *int32
 
 	// Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the

pkg/deploy/apis/apps/v1/defaults.go

@@ -6,6 +6,17 @@ import (
 	deployapi "github.com/openshift/origin/pkg/deploy/apis/apps"
 )
 
+// Applies defaults only for API group "apps.openshift.io" and not for the legacy API.
+// This function is called from storage layer where differentiation
+// between legacy and group API can be made and is not related to other functions here
+// which are called fom auto-generated code.
+func AppsV1DeploymentConfigLayeredDefaults(dc *deployapi.DeploymentConfig) {
+	if dc.Spec.RevisionHistoryLimit == nil {
+		v := deployapi.DefaultRevisionHistoryLimit
+		dc.Spec.RevisionHistoryLimit = &v
+	}
+}
+
 // Keep this in sync with pkg/api/serialization_test.go#defaultHookContainerName
 func defaultHookContainerName(hook *LifecycleHook, containerName string) {
 	if hook == nil {

pkg/deploy/apis/apps/v1/swagger_doc.go

@@ -101,7 +101,7 @@ var map_DeploymentConfigSpec = map[string]string{
 	"minReadySeconds":      "MinReadySeconds is the minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)",
 	"triggers":             "Triggers determine how updates to a DeploymentConfig result in new deployments. If no triggers are defined, a new deployment can only occur as a result of an explicit client update to the DeploymentConfig with a new LatestVersion. If null, defaults to having a config change trigger.",
 	"replicas":             "Replicas is the number of desired replicas.",
-	"revisionHistoryLimit": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.",
+	"revisionHistoryLimit": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)",
 	"test":                 "Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the deployment config to be used as a continuous deployment test - triggering on images, running the deployment, and then succeeding or failing. Post strategy hooks and After actions can be used to integrate successful deployment with an action.",
 	"paused":               "Paused indicates that the deployment config is paused resulting in no new deployments on template changes or changes in the template caused by other triggers.",
 	"selector":             "Selector is a label query over pods that should match the Replicas count.",

pkg/deploy/apis/apps/v1/types.go

@@ -51,6 +51,7 @@ type DeploymentConfigSpec struct {
 
 	// RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
 	// This field is a pointer to allow for differentiation between an explicit zero and not specified.
+	// Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)
 	RevisionHistoryLimit *int32 `json:"revisionHistoryLimit,omitempty" protobuf:"varint,4,opt,name=revisionHistoryLimit"`
 
 	// Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the

pkg/deploy/registry/deployconfig/etcd/etcd.go

@@ -37,9 +37,9 @@ func NewREST(optsGetter restoptions.Getter) (*REST, *StatusREST, *ScaleREST, err
 		PredicateFunc:            deployconfig.Matcher,
 		DefaultQualifiedResource: deployapi.Resource("deploymentconfigs"),
 
-		CreateStrategy: deployconfig.Strategy,
-		UpdateStrategy: deployconfig.Strategy,
-		DeleteStrategy: deployconfig.Strategy,
+		CreateStrategy: deployconfig.GroupStrategy,
+		UpdateStrategy: deployconfig.GroupStrategy,
+		DeleteStrategy: deployconfig.GroupStrategy,
 	}
 
 	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: deployconfig.GetAttrs}

pkg/deploy/registry/deployconfig/strategy.go

@@ -10,11 +10,13 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/apiserver/pkg/registry/rest"
 	kstorage "k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/names"
 	kapi "k8s.io/kubernetes/pkg/api"
 
 	deployapi "github.com/openshift/origin/pkg/deploy/apis/apps"
+	deployapiv1 "github.com/openshift/origin/pkg/deploy/apis/apps/v1"
 	"github.com/openshift/origin/pkg/deploy/apis/apps/validation"
 )
 
@@ -24,8 +26,16 @@ type strategy struct {
 	names.NameGenerator
 }
 
-// Strategy is the default logic that applies when creating and updating DeploymentConfig objects.
-var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+// CommonStrategy is the default logic that applies when creating and updating DeploymentConfig objects.
+var CommonStrategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+
+// LegacyStrategy is the logic that applies when creating and updating DeploymentConfig objects in the legacy API.
+// An example would be setting different defaults depending on API group
+var LegacyStrategy = legacyStrategy{CommonStrategy}
+
+// GroupStrategy is the logic that applies when creating and updating DeploymentConfig objects in the group API.
+// An example would be setting different defaults depending on API group
+var GroupStrategy = groupStrategy{CommonStrategy}
 
 // NamespaceScoped is true for DeploymentConfig objects.
 func (strategy) NamespaceScoped() bool {
@@ -105,12 +115,40 @@ func (strategy) CheckGracefulDelete(obj runtime.Object, options *metav1.DeleteOp
 	return false
 }
 
+// legacyStrategy implements behavior for DeploymentConfig objects in the legacy API
+type legacyStrategy struct {
+	strategy
+}
+
+// PrepareForCreate delegates to the common strategy.
+func (s legacyStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+	s.strategy.PrepareForCreate(ctx, obj)
+}
+
+// DefaultGarbageCollectionPolicy for legacy DeploymentConfigs will orphan dependents.
+func (s legacyStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
+	return rest.OrphanDependents
+}
+
+// groupStrategy implements behavior for DeploymentConfig objects in the Group API
+type groupStrategy struct {
+	strategy
+}
+
+// PrepareForCreate delegates to the common strategy and sets defaults applicable only to Group API
+func (s groupStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+	s.strategy.PrepareForCreate(ctx, obj)
+
+	dc := obj.(*deployapi.DeploymentConfig)
+	deployapiv1.AppsV1DeploymentConfigLayeredDefaults(dc)
+}
+
 // statusStrategy implements behavior for DeploymentConfig status updates.
 type statusStrategy struct {
 	strategy
 }
 
-var StatusStrategy = statusStrategy{Strategy}
+var StatusStrategy = statusStrategy{CommonStrategy}
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update of status.
 func (statusStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) {