Set default for DeploymentConfigSpec.RevisionHistoryLimit in apps/v1

[tnozicka]

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

[tnozicka]

@openshift/api-review

[0xmichalis]

/approve
/lgtm

[tnozicka]

infra failed
/test unit

[0xmichalis]

/retest

[tnozicka]

/retest

[tnozicka]

/retest

[smarterclayton]

My email never got delivered it looks like. The defaults can only be applied when creating via the "apps.openshift.io" group, not the default group. There needs to be specific code to check for that just like: https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/legacy.go#L190

Removing label

[tnozicka]

@smarterclayton my bad :( It was already like 9 PM on Friday for me and I was misguided by defaulters located under path deploy/apis/apps/v1/defaults.go. Should have tested creating resources in both API groups though...

I am feeling a bit hesitant to do this at storage layer but I might be wrong. We have defaulters, right? But at this point we register the same ones for legacy and the new group

origin/pkg/deploy/apis/apps/v1/register.go

Lines 19 to 22 in 7580da2

	LegacySchemeBuilder = runtime.NewSchemeBuilder(addLegacyKnownTypes, addConversionFuncs, RegisterDefaults)
	AddToSchemeInCoreGroup = LegacySchemeBuilder.AddToScheme
	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, RegisterDefaults)

Shouldn't I better plumb it there?

[smarterclayton]

The line I pasted is where the storage is customized one way or another. The strategy is what encapsulates the additional defaulting, so it's probably better that we keep the registration of the defaulting there. Creating net new types has a bunch of other impacts that we're probably not ready for (but which at some point will become necessary). When we do duplicate the types, we'll change how we register. Too big for the near term though.

…

On Tue, Aug 8, 2017 at 6:16 AM, Tomáš Nožička ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> my bad :( It was already like 9 PM on Friday for me and I was misguided by defaulters located under path deploy/apis/apps/v1/defaults.go. Should have tested creating resources in both API groups though... I am feeling a bit hesitant to do this at storage layer but I might be wrong. We have defaulters, right? But at this point we register the same ones for legacy and the new group https://github.com/openshift/ origin/blob/7580da21a2133f7d3e013b1bfe137db33110f4d3/pkg/deploy/apis/ apps/v1/register.go#L19-L22 Shouldn't I better plumb it there? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15642 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p1ryEttb5vKHq_lqyXgIfqj8pxVPks5sWDWJgaJpZM4OuDCi> .

[tnozicka]

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

[tnozicka]

@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

origin/pkg/deploy/controller/deploymentconfig/controller_ref_manager.go

Line 143 in e98a1a9

"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:

origin/test/extended/deployments/deployments.go

Line 1098 in e98a1a9

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_/

[tnozicka]

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?

[tnozicka]

@mfojtik @smarterclayton bump

[mfojtik]

@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?

[tnozicka]

I wanted to wrap the defaulting functions just for group API SchemeBuilder but that didn't work because they get called even for legacy resources (it calls defaulting functions twice for legacy resources) so I ended up modifying it on storage layer.

I am not excited to have defaulting scattered between 2 places but this is probably the only option here. And BuildConfigs do the same.

I will add an integration test tomorrow but this is mostly done.

[smarterclayton]

Split this into its own method for sanity. func AppsV1DeploymentConfigLayeredDefaults(*deployapi.DeploymentConfig) and add a good godoc comment.

[smarterclayton]

Looks good to me, go ahead and squash.

[tnozicka]

yum repo flake
/retest

[smarterclayton]

/retest

[tnozicka]

I will need to plumb in the new group client for tests and use that in

origin/test/extended/deployments/deployments.go

Lines 1093 to 1098 in e98a1a9

	g.By("deleting owned RCs when deleted", func() {
	// FIXME: Add delete option when we have new client available.
	// This is working fine now because of finalizers on RCs but when GC gets fixed
	// and we remove them this will probably break and will require setting deleteOptions
	// to achieve cascade delete
	err = oc.Client().DeploymentConfigs(namespace).Delete(dcName)

instead of the legacy client - that's why the extended_conformance_gce test is failing

[tnozicka]

• rebased to master
• plumbed in the group API into testing framework (only for AppsV1 now, but in a general and extensible way
• fixed the cascade delete test using the group API

[smarterclayton]

Looks perfect

/lgtm

[tnozicka]

Seems like some changes in code generation went into master from last time I run make update.
Updated files:

	modified:   pkg/user/generated/informers/externalversions/generic.go
	new file:   pkg/user/generated/informers/externalversions/user/v1/identity.go
	modified:   pkg/user/generated/informers/externalversions/user/v1/interface.go
	modified:   pkg/user/generated/informers/internalversion/generic.go
	new file:   pkg/user/generated/informers/internalversion/user/internalversion/identity.go
	modified:   pkg/user/generated/informers/internalversion/user/internalversion/interface.go
	modified:   pkg/user/generated/listers/user/internalversion/expansion_generated.go
	new file:   pkg/user/generated/listers/user/internalversion/identity.go
	modified:   pkg/user/generated/listers/user/internalversion/user.go
	modified:   pkg/user/generated/listers/user/v1/expansion_generated.go
	new file:   pkg/user/generated/listers/user/v1/identity.go

no other changes

[tnozicka]

/retest

[tnozicka]

Another change in code generation in master - rebased and added new generated files:

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

[mfojtik]

/lgtm

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kargakis, mfojtik, smarterclayton, tnozicka

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [mfojtik,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[openshift-merge-robot]

Automatic merge from submit-queue

[tnozicka]

@Kargakis not that I want to complain but submit queue probably shouldn't merge PRs with needs-api-review label