Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set default for DeploymentConfigSpec.RevisionHistoryLimit in apps/v1 #15642

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/docs/apis-apps.openshift.io/v1.DeploymentConfig.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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>:
Expand Down
2 changes: 1 addition & 1 deletion api/docs/oapi/v1.DeploymentConfig.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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>:
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion api/swagger-spec/oapi-v1.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion api/swagger-spec/openshift-openapi-spec.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
61 changes: 61 additions & 0 deletions pkg/client/clientset/clientset/clientset.go
Original file line number Diff line number Diff line change
@@ -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
}
16 changes: 3 additions & 13 deletions pkg/cmd/server/origin/legacy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have my doubts about if this ever worked - restStorage should have been store

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton @mfojtik This seems to be indeed a bug that we have carried for a long time.

What do we want to do with legacy API cascade deletion? We enabled it for new DCs by

"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],
but fixing this bug seems to mitigate it. You can see it broken in our test using the legacy client:
err = oc.Client().DeploymentConfigs(namespace).Delete(dcName)
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install_update/4085/testReport/junit/(root)/Extended/deploymentconfigs__should_adhere_to_Three_Laws_of_Controllers__Conformance_/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong that the patch would change the propagation policy. (We used to set a finalizer here, but now there is only blockOwnerDeletion which doesn't affect it in that way. So I will rephrase the question:

@smarterclayton @mfojtik I guess we initially wanted DCs created by legacy API not to have cascading deletion but given that we have failed at it due to this bug. Given that it has already been in a release (https://github.com/openshift/origin/blame/master/pkg/cmd/server/origin/legacy.go#L209) do we want to turn it back again? What's the policy for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnozicka that is right, it should be store and not restStorage (@deads2k).

I think we should fix this bug and backport to 3.5, @deads2k @smarterclayton thoughts?

store.CreateStrategy = deployconfig.LegacyStrategy
store.DeleteStrategy = deployconfig.LegacyStrategy
legacyStorage[resource] = &deploymentconfigetcd.REST{Store: &store}
case "routes":
restStorage := s.(*routeetcd.REST)
Expand All @@ -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}
}
4 changes: 4 additions & 0 deletions pkg/deploy/apis/apps/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions pkg/deploy/apis/apps/v1/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions pkg/deploy/apis/apps/v1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/deploy/apis/apps/v1/swagger_doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
1 change: 1 addition & 0 deletions pkg/deploy/apis/apps/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions pkg/deploy/registry/deployconfig/etcd/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
44 changes: 41 additions & 3 deletions pkg/deploy/registry/deployconfig/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
Loading