From 04c7484bfdfebf19b6cca2f04487ea113ab73f87 Mon Sep 17 00:00:00 2001 From: Ben Parees Date: Thu, 22 Jun 2017 16:31:48 -0400 Subject: [PATCH] set default build prune limits for group api --- pkg/build/api/types.go | 18 +++- pkg/build/registry/buildconfig/etcd/etcd.go | 6 +- pkg/build/registry/buildconfig/strategy.go | 74 +++++++++++++-- pkg/cmd/cli/cmd/exporter.go | 2 +- pkg/cmd/server/origin/legacy.go | 4 +- test/extended/builds/build_pruning.go | 37 +++++++- test/extended/testdata/bindata.go | 94 ++++++++++++++----- .../default-group-build-config.yaml | 16 ++++ .../default-legacy-build-config.yaml | 16 ++++ .../build-pruning/failed-build-config.yaml | 11 --- .../successful-build-config.yaml | 11 --- 11 files changed, 225 insertions(+), 64 deletions(-) create mode 100644 test/extended/testdata/build-pruning/default-group-build-config.yaml create mode 100644 test/extended/testdata/build-pruning/default-legacy-build-config.yaml diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 972015024f22..0f8a58874673 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -80,11 +80,23 @@ const ( BuildCancelledEventReason = "BuildCancelled" // BuildCancelledEventMessage is the message associated with the event registered when build is cancelled. BuildCancelledEventMessage = "Build %s/%s has been cancelled" + + // DefaultSuccessfulBuildsHistoryLimit is the default number of successful builds to retain + // if the buildconfig does not specify a value. This only applies to buildconfigs created + // via the new group api resource, not the legacy resource. + DefaultSuccessfulBuildsHistoryLimit = int32(5) + + // DefaultFailedBuildsHistoryLimit is the default number of failed builds to retain + // if the buildconfig does not specify a value. This only applies to buildconfigs created + // via the new group api resource, not the legacy resource. + DefaultFailedBuildsHistoryLimit = int32(5) ) -// WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the -// user on the build pod. -var WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"} +var ( + // WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the + // user on the build pod. + WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"} +) // +genclient=true diff --git a/pkg/build/registry/buildconfig/etcd/etcd.go b/pkg/build/registry/buildconfig/etcd/etcd.go index 29a0ea21fb51..8aac2e07f326 100644 --- a/pkg/build/registry/buildconfig/etcd/etcd.go +++ b/pkg/build/registry/buildconfig/etcd/etcd.go @@ -24,9 +24,9 @@ func NewREST(optsGetter restoptions.Getter) (*REST, error) { QualifiedResource: api.Resource("buildconfigs"), PredicateFunc: buildconfig.Matcher, - CreateStrategy: buildconfig.Strategy, - UpdateStrategy: buildconfig.Strategy, - DeleteStrategy: buildconfig.Strategy, + CreateStrategy: buildconfig.GroupStrategy, + UpdateStrategy: buildconfig.GroupStrategy, + DeleteStrategy: buildconfig.GroupStrategy, } options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: buildconfig.GetAttrs} diff --git a/pkg/build/registry/buildconfig/strategy.go b/pkg/build/registry/buildconfig/strategy.go index 9f2feeacd9c6..33799e5cec3b 100644 --- a/pkg/build/registry/buildconfig/strategy.go +++ b/pkg/build/registry/buildconfig/strategy.go @@ -9,6 +9,7 @@ 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" @@ -17,15 +18,28 @@ import ( "github.com/openshift/origin/pkg/build/api/validation" ) -// strategy implements behavior for BuildConfig objects +var ( + // GroupStrategy is the logic that applies when creating and updating BuildConfig objects + // in the Group api. + // This differs from the LegacyStrategy in that on create it will set a default build + // pruning limit value for both successful and failed builds. This is new behavior that + // can only be introduced to users consuming the new group based api. + GroupStrategy = groupStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}} + + // LegacyStrategy is the default logic that applies when creating BuildConfig objects. + // Specifically it will not set the default build pruning limit values because that was not + // part of the legacy api. + LegacyStrategy = legacyStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}} +) + +// strategy implements most of the behavior for BuildConfig objects +// It does not provide a PrepareForCreate implementation, that is expected +// to be implemented by the child implementation. type strategy struct { runtime.ObjectTyper names.NameGenerator } -// Strategy is the default logic that applies when creating and updating BuildConfig objects. -var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator} - func (strategy) NamespaceScoped() bool { return true } @@ -39,16 +53,17 @@ func (strategy) AllowUnconditionalUpdate() bool { return false } -// PrepareForCreate clears fields that are not allowed to be set by end users on creation. -func (strategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) { - bc := obj.(*api.BuildConfig) - dropUnknownTriggers(bc) -} - // Canonicalize normalizes the object after validation. func (strategy) Canonicalize(obj runtime.Object) { } +// PrepareForCreateCommon clears fields that are not allowed to be set by end users on creation. +// This is shared by the Group and Legacy strategies. +func (s strategy) PrepareForCreateCommon(ctx apirequest.Context, obj runtime.Object) { + bc := obj.(*api.BuildConfig) + dropUnknownTriggers(bc) +} + // PrepareForUpdate clears fields that are not allowed to be set by end users on update. func (strategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) { newBC := obj.(*api.BuildConfig) @@ -71,6 +86,45 @@ func (strategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) return validation.ValidateBuildConfigUpdate(obj.(*api.BuildConfig), old.(*api.BuildConfig)) } +// groupStrategy implements behavior for BuildConfig objects in the Group api +type groupStrategy struct { + strategy +} + +// PrepareForCreate delegates to the common strategy and sets default pruning limits +func (s groupStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) { + s.PrepareForCreateCommon(ctx, obj) + + bc := obj.(*api.BuildConfig) + if bc.Spec.SuccessfulBuildsHistoryLimit == nil { + v := api.DefaultSuccessfulBuildsHistoryLimit + bc.Spec.SuccessfulBuildsHistoryLimit = &v + } + if bc.Spec.FailedBuildsHistoryLimit == nil { + v := api.DefaultFailedBuildsHistoryLimit + bc.Spec.FailedBuildsHistoryLimit = &v + } +} + +// legacyStrategy implements behavior for BuildConfig 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.PrepareForCreateCommon(ctx, obj) + + // legacy buildconfig api does not apply default pruning values, to maintain + // backwards compatibility. + +} + +// DefaultGarbageCollectionPolicy for legacy buildconfigs will orphan dependents. +func (s legacyStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy { + return rest.OrphanDependents +} + // GetAttrs returns labels and fields of a given object for filtering purposes func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) { buildConfig, ok := obj.(*api.BuildConfig) diff --git a/pkg/cmd/cli/cmd/exporter.go b/pkg/cmd/cli/cmd/exporter.go index 9d3662982278..ca8a3f2f0bca 100644 --- a/pkg/cmd/cli/cmd/exporter.go +++ b/pkg/cmd/cli/cmd/exporter.go @@ -148,7 +148,7 @@ func (e *DefaultExporter) Export(obj runtime.Object, exact bool) error { return deployrest.Strategy.Export(ctx, obj, exact) case *buildapi.BuildConfig: - buildconfigrest.Strategy.PrepareForCreate(ctx, obj) + buildconfigrest.GroupStrategy.PrepareForCreate(ctx, obj) // TODO: should be handled by prepare for create t.Status.LastVersion = 0 for i := range t.Spec.Triggers { diff --git a/pkg/cmd/server/origin/legacy.go b/pkg/cmd/server/origin/legacy.go index e51878eb4edb..7216ea8ae833 100644 --- a/pkg/cmd/server/origin/legacy.go +++ b/pkg/cmd/server/origin/legacy.go @@ -5,6 +5,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/registry/rest" + buildconfig "github.com/openshift/origin/pkg/build/registry/buildconfig" buildconfigetcd "github.com/openshift/origin/pkg/build/registry/buildconfig/etcd" deploymentconfigetcd "github.com/openshift/origin/pkg/deploy/registry/deployconfig/etcd" ) @@ -197,7 +198,8 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[ case "buildConfigs": restStorage := s.(*buildconfigetcd.REST) store := *restStorage.Store - store.DeleteStrategy = orphanByDefault(store.DeleteStrategy) + store.DeleteStrategy = buildconfig.LegacyStrategy + store.CreateStrategy = buildconfig.LegacyStrategy legacyStorage[resource] = &buildconfigetcd.REST{Store: &store} case "deploymentConfigs": restStorage := s.(*deploymentconfigetcd.REST) diff --git a/test/extended/builds/build_pruning.go b/test/extended/builds/build_pruning.go index ccbee7e0060a..0d5e86f2c0ae 100644 --- a/test/extended/builds/build_pruning.go +++ b/test/extended/builds/build_pruning.go @@ -7,9 +7,10 @@ import ( g "github.com/onsi/ginkgo" o "github.com/onsi/gomega" - exutil "github.com/openshift/origin/test/extended/util" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + buildapi "github.com/openshift/origin/pkg/build/api" + exutil "github.com/openshift/origin/test/extended/util" ) var _ = g.Describe("[builds][pruning] prune builds based on settings in the buildconfig", func() { @@ -19,6 +20,8 @@ var _ = g.Describe("[builds][pruning] prune builds based on settings in the buil successfulBuildConfig = filepath.Join(buildPruningBaseDir, "successful-build-config.yaml") failedBuildConfig = filepath.Join(buildPruningBaseDir, "failed-build-config.yaml") erroredBuildConfig = filepath.Join(buildPruningBaseDir, "errored-build-config.yaml") + legacyBuildConfig = filepath.Join(buildPruningBaseDir, "default-legacy-build-config.yaml") + groupBuildConfig = filepath.Join(buildPruningBaseDir, "default-group-build-config.yaml") oc = exutil.NewCLI("build-pruning", exutil.KubeConfigPath()) ) @@ -149,4 +152,34 @@ var _ = g.Describe("[builds][pruning] prune builds based on settings in the buil }) + g.It("buildconfigs should have a default history limit set when created via the group api", func() { + + g.By("creating a build config with the group api") + err := oc.Run("create").Args("-f", groupBuildConfig).Execute() + o.Expect(err).NotTo(o.HaveOccurred()) + + buildConfig, err := oc.Client().BuildConfigs(oc.Namespace()).Get("myphp", metav1.GetOptions{}) + if err != nil { + fmt.Fprintf(g.GinkgoWriter, "%v", err) + } + o.Expect(buildConfig.Spec.SuccessfulBuildsHistoryLimit).NotTo(o.BeNil(), "the buildconfig should have the default successful history limit set") + o.Expect(buildConfig.Spec.FailedBuildsHistoryLimit).NotTo(o.BeNil(), "the buildconfig should have the default failed history limit set") + o.Expect(*buildConfig.Spec.SuccessfulBuildsHistoryLimit).To(o.Equal(buildapi.DefaultSuccessfulBuildsHistoryLimit), "the buildconfig should have the default successful history limit set") + o.Expect(*buildConfig.Spec.FailedBuildsHistoryLimit).To(o.Equal(buildapi.DefaultFailedBuildsHistoryLimit), "the buildconfig should have the default failed history limit set") + }) + + g.It("buildconfigs should not have a default history limit set when created via the legacy api", func() { + + g.By("creating a build config with the legacy api") + err := oc.Run("create").Args("-f", legacyBuildConfig).Execute() + o.Expect(err).NotTo(o.HaveOccurred()) + + buildConfig, err := oc.Client().BuildConfigs(oc.Namespace()).Get("myphp", metav1.GetOptions{}) + if err != nil { + fmt.Fprintf(g.GinkgoWriter, "%v", err) + } + o.Expect(buildConfig.Spec.SuccessfulBuildsHistoryLimit).To(o.BeNil(), "the buildconfig should not have the default successful history limit set") + o.Expect(buildConfig.Spec.FailedBuildsHistoryLimit).To(o.BeNil(), "the buildconfig should not have the default failed history limit set") + }) + }) diff --git a/test/extended/testdata/bindata.go b/test/extended/testdata/bindata.go index 78635216b028..eb3e723aa47f 100644 --- a/test/extended/testdata/bindata.go +++ b/test/extended/testdata/bindata.go @@ -12,6 +12,8 @@ // test/extended/testdata/build-extended/bc-scripts-in-the-image.yaml // test/extended/testdata/build-extended/jvm-runner-with-scripts.yaml // test/extended/testdata/build-extended/jvm-runner.yaml +// test/extended/testdata/build-pruning/default-group-build-config.yaml +// test/extended/testdata/build-pruning/default-legacy-build-config.yaml // test/extended/testdata/build-pruning/errored-build-config.yaml // test/extended/testdata/build-pruning/failed-build-config.yaml // test/extended/testdata/build-pruning/imagestream.yaml @@ -683,6 +685,72 @@ func testExtendedTestdataBuildExtendedJvmRunnerYaml() (*asset, error) { return a, nil } +var _testExtendedTestdataBuildPruningDefaultGroupBuildConfigYaml = []byte(`apiVersion: build.openshift.io/v1 +kind: BuildConfig +metadata: + name: myphp +spec: + source: + type: Git + git: + uri: 'https://github.com/openshift/cakephp-ex.git' + strategy: + type: Source + sourceStrategy: + from: + kind: ImageStreamTag + namespace: openshift + name: 'php:7.0' +`) + +func testExtendedTestdataBuildPruningDefaultGroupBuildConfigYamlBytes() ([]byte, error) { + return _testExtendedTestdataBuildPruningDefaultGroupBuildConfigYaml, nil +} + +func testExtendedTestdataBuildPruningDefaultGroupBuildConfigYaml() (*asset, error) { + bytes, err := testExtendedTestdataBuildPruningDefaultGroupBuildConfigYamlBytes() + if err != nil { + return nil, err + } + + info := bindataFileInfo{name: "test/extended/testdata/build-pruning/default-group-build-config.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + a := &asset{bytes: bytes, info: info} + return a, nil +} + +var _testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYaml = []byte(`apiVersion: v1 +kind: BuildConfig +metadata: + name: myphp +spec: + source: + type: Git + git: + uri: 'https://github.com/openshift/cakephp-ex.git' + strategy: + type: Source + sourceStrategy: + from: + kind: ImageStreamTag + namespace: openshift + name: 'php:7.0' +`) + +func testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYamlBytes() ([]byte, error) { + return _testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYaml, nil +} + +func testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYaml() (*asset, error) { + bytes, err := testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYamlBytes() + if err != nil { + return nil, err + } + + info := bindataFileInfo{name: "test/extended/testdata/build-pruning/default-legacy-build-config.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + a := &asset{bytes: bytes, info: info} + return a, nil +} + var _testExtendedTestdataBuildPruningErroredBuildConfigYaml = []byte(`apiVersion: v1 kind: BuildConfig metadata: @@ -733,13 +801,10 @@ metadata: openshift.io/generated-by: OpenShiftWebConsole spec: failedBuildsHistoryLimit: 2 - triggers: {} - runPolicy: Serial source: type: Git git: uri: 'https://github.com/openshift/non-working-example.git' - ref: master strategy: type: Source sourceStrategy: @@ -747,14 +812,6 @@ spec: kind: ImageStreamTag namespace: openshift name: 'php:7.0' - output: - to: - kind: ImageStreamTag - name: 'myphp:latest' - resources: {} - postCommit: {} - nodeSelector: null -status: `) func testExtendedTestdataBuildPruningFailedBuildConfigYamlBytes() ([]byte, error) { @@ -803,13 +860,10 @@ metadata: openshift.io/generated-by: OpenShiftWebConsole spec: successfulBuildsHistoryLimit: 2 - triggers: {} - runPolicy: Serial source: type: Git git: uri: 'https://github.com/openshift/cakephp-ex.git' - ref: master strategy: type: Source sourceStrategy: @@ -817,14 +871,6 @@ spec: kind: ImageStreamTag namespace: openshift name: 'php:7.0' - output: - to: - kind: ImageStreamTag - name: 'myphp:latest' - resources: {} - postCommit: {} - nodeSelector: null -status: `) func testExtendedTestdataBuildPruningSuccessfulBuildConfigYamlBytes() ([]byte, error) { @@ -20176,6 +20222,8 @@ var _bindata = map[string]func() (*asset, error){ "test/extended/testdata/build-extended/bc-scripts-in-the-image.yaml": testExtendedTestdataBuildExtendedBcScriptsInTheImageYaml, "test/extended/testdata/build-extended/jvm-runner-with-scripts.yaml": testExtendedTestdataBuildExtendedJvmRunnerWithScriptsYaml, "test/extended/testdata/build-extended/jvm-runner.yaml": testExtendedTestdataBuildExtendedJvmRunnerYaml, + "test/extended/testdata/build-pruning/default-group-build-config.yaml": testExtendedTestdataBuildPruningDefaultGroupBuildConfigYaml, + "test/extended/testdata/build-pruning/default-legacy-build-config.yaml": testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYaml, "test/extended/testdata/build-pruning/errored-build-config.yaml": testExtendedTestdataBuildPruningErroredBuildConfigYaml, "test/extended/testdata/build-pruning/failed-build-config.yaml": testExtendedTestdataBuildPruningFailedBuildConfigYaml, "test/extended/testdata/build-pruning/imagestream.yaml": testExtendedTestdataBuildPruningImagestreamYaml, @@ -20489,6 +20537,8 @@ var _bintree = &bintree{nil, map[string]*bintree{ "jvm-runner.yaml": &bintree{testExtendedTestdataBuildExtendedJvmRunnerYaml, map[string]*bintree{}}, }}, "build-pruning": &bintree{nil, map[string]*bintree{ + "default-group-build-config.yaml": &bintree{testExtendedTestdataBuildPruningDefaultGroupBuildConfigYaml, map[string]*bintree{}}, + "default-legacy-build-config.yaml": &bintree{testExtendedTestdataBuildPruningDefaultLegacyBuildConfigYaml, map[string]*bintree{}}, "errored-build-config.yaml": &bintree{testExtendedTestdataBuildPruningErroredBuildConfigYaml, map[string]*bintree{}}, "failed-build-config.yaml": &bintree{testExtendedTestdataBuildPruningFailedBuildConfigYaml, map[string]*bintree{}}, "imagestream.yaml": &bintree{testExtendedTestdataBuildPruningImagestreamYaml, map[string]*bintree{}}, diff --git a/test/extended/testdata/build-pruning/default-group-build-config.yaml b/test/extended/testdata/build-pruning/default-group-build-config.yaml new file mode 100644 index 000000000000..d4d0dbc12034 --- /dev/null +++ b/test/extended/testdata/build-pruning/default-group-build-config.yaml @@ -0,0 +1,16 @@ +apiVersion: build.openshift.io/v1 +kind: BuildConfig +metadata: + name: myphp +spec: + source: + type: Git + git: + uri: 'https://github.com/openshift/cakephp-ex.git' + strategy: + type: Source + sourceStrategy: + from: + kind: ImageStreamTag + namespace: openshift + name: 'php:7.0' diff --git a/test/extended/testdata/build-pruning/default-legacy-build-config.yaml b/test/extended/testdata/build-pruning/default-legacy-build-config.yaml new file mode 100644 index 000000000000..78a3d92f843b --- /dev/null +++ b/test/extended/testdata/build-pruning/default-legacy-build-config.yaml @@ -0,0 +1,16 @@ +apiVersion: v1 +kind: BuildConfig +metadata: + name: myphp +spec: + source: + type: Git + git: + uri: 'https://github.com/openshift/cakephp-ex.git' + strategy: + type: Source + sourceStrategy: + from: + kind: ImageStreamTag + namespace: openshift + name: 'php:7.0' diff --git a/test/extended/testdata/build-pruning/failed-build-config.yaml b/test/extended/testdata/build-pruning/failed-build-config.yaml index ece44b8da6af..2496c9854c69 100644 --- a/test/extended/testdata/build-pruning/failed-build-config.yaml +++ b/test/extended/testdata/build-pruning/failed-build-config.yaml @@ -8,13 +8,10 @@ metadata: openshift.io/generated-by: OpenShiftWebConsole spec: failedBuildsHistoryLimit: 2 - triggers: {} - runPolicy: Serial source: type: Git git: uri: 'https://github.com/openshift/non-working-example.git' - ref: master strategy: type: Source sourceStrategy: @@ -22,11 +19,3 @@ spec: kind: ImageStreamTag namespace: openshift name: 'php:7.0' - output: - to: - kind: ImageStreamTag - name: 'myphp:latest' - resources: {} - postCommit: {} - nodeSelector: null -status: diff --git a/test/extended/testdata/build-pruning/successful-build-config.yaml b/test/extended/testdata/build-pruning/successful-build-config.yaml index 14419bd62d50..85f593259433 100644 --- a/test/extended/testdata/build-pruning/successful-build-config.yaml +++ b/test/extended/testdata/build-pruning/successful-build-config.yaml @@ -8,13 +8,10 @@ metadata: openshift.io/generated-by: OpenShiftWebConsole spec: successfulBuildsHistoryLimit: 2 - triggers: {} - runPolicy: Serial source: type: Git git: uri: 'https://github.com/openshift/cakephp-ex.git' - ref: master strategy: type: Source sourceStrategy: @@ -22,11 +19,3 @@ spec: kind: ImageStreamTag namespace: openshift name: 'php:7.0' - output: - to: - kind: ImageStreamTag - name: 'myphp:latest' - resources: {} - postCommit: {} - nodeSelector: null -status: