set default build prune limits for group api #14845

bparees · 2017-06-22T20:33:13Z

introduces behavior that will default a successful and failed build history limit for buildconfigs that are created via the new group api. legacy api behavior remains the same (no default limit, all builds are kept).

bparees · 2017-06-22T20:33:34Z

[testextended][extended:core(builds)]

bparees · 2017-06-22T20:41:06Z

@openshift/devex ptal
@smarterclayton @deads2k ptal also in case i've taken an insane approach.

bparees · 2017-06-22T20:41:34Z

[test]

gabemontero

I minor nit

gabemontero · 2017-06-22T21:08:03Z

pkg/build/registry/buildconfig/strategy.go

-var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator, false}
+
+// GroupStrategy is the default logic that applies when creating and updating BuildConfig objects.


comment - GroupStrategy

code - LegacyStrategy

thanks, fixed.

coreydaley · 2017-06-22T22:22:06Z

pkg/build/registry/buildconfig/strategy.go

-var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator, false}
+
+// LegacyStrategy is the default logic that applies when creating and updating BuildConfig objects.


Should this comment and the one for Strategy be more explanatory? Like, why would one be applied over the other? As they stand it sounds like they are both applied by default?

Also, s/applies/is applied/ ?

i'll expand the comment. applies is correct/intentional. "this is the rule that applies in this situation"

smarterclayton · 2017-06-22T23:31:07Z

For reference here's the other backcompat change view it on GitHub <#14818 (comment)>

…

On Thu, Jun 22, 2017 at 7:23 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/testextended SUCCESS ( https://ci.openshift.redhat.com/jenkins/job/test_pull_ request_origin_extended/700/) (Base Commit: 4423ff5 <4423ff5>) (PR Branch Commit: cf46f52 <cf46f52>) (Extended Tests: core(builds)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14845 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p-BPUsi0Wa4xqh6tLIOL9guyeZThks5sGvd2gaJpZM4OCz7a> .

bparees · 2017-06-23T01:59:13Z

For reference here's the other backcompat change view it on GitHub

looks at least superficially similar to what i've done here. That one might be a bit more architectually pure but do you have any concerns w/ how i've done it here?

jim-minter · 2017-06-23T10:09:02Z

pkg/build/api/types.go

+	// 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)


could be consts?

no because i need a pointer value at assignment time and you can't take an address of a const.

(so they could be consts, but then we'd have to do a secondary assignment at the point of use)

it's a nit, but isn't that a good argument for them to be consts?

you mean to avoid someone inadvertently changing them after something else has a pointer to them?

deads2k · 2017-06-23T15:14:20Z

pkg/build/registry/buildconfig/strategy.go

+// 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.
+var LegacyStrategy = strategy{kapi.Scheme, names.SimpleNameGenerator, true}


Nice. I like having different strategies.

deads2k · 2017-06-23T15:14:38Z

pkg/build/registry/buildconfig/strategy.go

+
+	// legacy buildconfig api does not apply default pruning values, to maintain
+	// backwards compatibility.
+	if !s.legacy {


Any chance of actually having different types and losing this if block?

you mean two separate impls? (with one presumably embedding the other and then overriding this specific logic?) I can..

bparees · 2017-06-26T16:54:30Z

@jim-minter made it a constant
@deads2k introduced a full strategy hierarchy

ptal

deads2k · 2017-06-26T17:36:05Z

pkg/build/registry/buildconfig/strategy.go

 // 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) {


This could just be PrepareForCreate and the impl are just specific when they call, right?

if i didn't want to share the common behavior, yes. but I didn't want to write the common behavior in both impls. Am I missing a composition pattern?

deads2k · 2017-06-26T17:37:40Z

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)


Shouldn't this vary based on how you requested the buildconfig?

maybe. can i determine that by looking at the obj.TypeMeta?

leave this as legacy. The new endpoint will have it, the old one won't, and you don't want to twiddle it.

deads2k · 2017-06-26T17:38:33Z

pkg/cmd/server/origin/legacy.go

@@ -198,6 +199,7 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 					restStorage := s.(*buildconfigetcd.REST)
 					store := *restStorage.Store
 					store.DeleteStrategy = orphanByDefault(store.DeleteStrategy)


This ought to actually be updated to just set it properly in the legacy strategy, right? Followup is ok.

yeah i guess that makes sense. i'll just do it here.

deads2k · 2017-06-26T17:39:39Z

test/extended/builds/build_pruning.go

@@ -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() {


Seems like you might want this as an integration test to stop any merge from succeeding if it breaks this.

we run these tests nightly and check the results, i can live w/ it for now at least.

Why not add them to conformance, they should be cheap?

Or are they not cheap?

they're "relatively" cheap. they all involve running builds which is never less than a minute or so, even though we're not pushing anywhere.

anyway i did end up adding a unit test, so at least if someone breaks the strategy we'll know. if they stop using the strategy, we'll find out when the nightly run fails.

(sorry, these tests in particular do not run builds. i guess i can add just the new ones to conformance)

openshift-bot · 2017-06-27T12:47:37Z

Evaluated for origin merge up to d039a9b

smarterclayton · 2017-06-27T19:32:28Z

[test]

bparees · 2017-06-27T21:38:56Z

@smarterclayton @stevekuznetsov ummm?

[INFO] Starting server
chcon: invalid context: /var/lib/openshift/openshift.local.volumes	system_u:object_r:openshift_var_lib_t:s0
[ERROR] PID 23536: test/extended/setup.sh:88: `${sudo} chcon "${label}" ${VOLUME_DIR}` exited with status 1.
[INFO] 		Stack Trace: 
[INFO] 		  1: test/extended/setup.sh:88: `${sudo} chcon "${label}" ${VOLUME_DIR}`
[INFO] 		  2: test/extended/core.sh:8: os::test::extended::setup
[INFO]   Exiting with code 1.
/data/src/github.com/openshift/origin/hack/lib/log/system.sh: line 31: 26206 Terminated              sar -A -o "${binary_logfile}" 1 86400 > /dev/null 2> "${stderr_logfile}"
[INFO] [CLEANUP] Beginning cleanup routines...
[INFO] [CLEANUP] Dumping cluster events to _output/scripts/core/artifacts/events.txt
error: A server URL must be specified
[INFO] [CLEANUP] Dumping etcd contents to _output/scripts/core/artifacts/etcd
[WARNING] No compiled `etcdhelper` binary was found. Attempting to build one using:
[WARNING]   $ hack/build-go.sh tools/etcdhelper
++ Building go targets for linux/amd64: tools/etcdhelper
/data/src/github.com/openshift/origin/hack/build-go.sh took 65 seconds
ERROR: unable to create client config: open /tmp/openshift/core/openshift.local.config/master/master.etcd-client.crt: no such file or directory
[INFO] [CLEANUP] Dumping container logs to _output/scripts/core/logs/containers
[INFO] [CLEANUP] Truncating log files over 200M
[INFO] [CLEANUP] Stopping docker containers
[INFO] [CLEANUP] Removing docker containers
[INFO] [CLEANUP] Killing child processes
[INFO] [CLEANUP] Pruning etcd data directory
[ERROR] test/extended/core.sh exited with code 1 after 00h 02m 03s
make: *** [test-extended] Error 1
++ export status=FAILURE
++ status=FAILURE
+ set +o xtrace
########## FINISHED STAGE: FAILURE: RUN EXTENDED TESTS [00h 02m 03s] ##########

https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/771/consoleFull

docker should be able to handle all of this correctly without the two problems in this patch. It hard codes svirt_sandbox_file_t, which is actually policy specific, even if basically everyone uses our reference policy. It means that things under that directory could (if they can break out of their mount namespace) see this directory. docker doesn't require this anymore. So remove this custom hack.

stevekuznetsov · 2017-06-28T13:24:24Z

@eparis looks like your changes in 88e4c71 may be causing the above

bparees · 2017-06-28T13:28:42Z

/me shakes tiny fist @eparis

stevekuznetsov · 2017-06-28T14:56:22Z

xref: #14937

openshift-bot · 2017-06-28T15:07:55Z

Evaluated for origin testextended up to 17a2e33

openshift-bot · 2017-06-28T15:08:04Z

Evaluated for origin test up to 17a2e33

openshift-bot · 2017-06-28T17:26:47Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2760/) (Base Commit: 3ff2db7) (PR Branch Commit: 17a2e33)

openshift-bot · 2017-06-28T17:47:40Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/777/) (Base Commit: 3ff2db7) (PR Branch Commit: 17a2e33) (Extended Tests: core(builds))

jorgemoralespou · 2017-07-14T10:05:51Z

@bparees should documentation be updated that the "build history" graph will no longer be seeing while using this new endpoint, as by default will never reach the amount of builds to show that graph?

Is it worth keeping that graph in the page?

bparees · 2017-07-14T11:51:16Z

@jorgemoralespou that's probably a better question for @jwforres or @spadgett but it's a good point.

jwforres · 2017-07-14T12:31:51Z

@bparees what are the new defaults?

jorgemoralespou · 2017-07-14T12:38:08Z

1 success and 1 failed I think.

jorgemoralespou · 2017-07-14T12:43:07Z

Although looking at the code it seems to be 5 each. I seem to have misunderstood first comment as it defaulted to 1. So disregard the comment.

jwforres · 2017-07-14T12:44:53Z

On Fri, Jul 14, 2017 at 8:43 AM, Jorge Morales Pou ***@***.*** > wrote: Although looking at the code it seems to be 5 each.

5 would show up in the history

…

I'll let @bparees <https://github.com/bparees> answer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14845 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7f3wcxi9-zA_DhIw93tZW2RMmwzkks5sN2JigaJpZM4OCz7a> .

bparees · 2017-07-14T13:27:34Z

it's 5. @jorgemoralespou where did you see 1?

jorgemoralespou · 2017-07-14T14:58:47Z

@bparees lost in translation. Read a for 1 in your first comment.

introduces behavior that will default a successful and failed build history limit

spadgett · 2017-07-14T15:04:10Z

We show the chart if there are 4 builds.

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/buildTrendsChart.js#L15-L16

bparees · 2017-07-14T15:11:56Z

There. Are. FOUR. Builds.

jorgemoralespou · 2017-07-14T16:14:09Z

@bparees I'm liking you more every day. I know is not reciprocal though. We'll get there ONE (1) day.

bparees force-pushed the default_pruning branch 2 times, most recently from 0389e9f to cf46f52 Compare June 22, 2017 20:40

openshift deleted a comment from openshift-bot Jun 22, 2017

gabemontero suggested changes Jun 22, 2017

View reviewed changes

bparees force-pushed the default_pruning branch from cf46f52 to c60ad43 Compare June 22, 2017 22:10

coreydaley suggested changes Jun 22, 2017

View reviewed changes

bparees force-pushed the default_pruning branch from c60ad43 to fa442ae Compare June 23, 2017 02:01

jim-minter reviewed Jun 23, 2017

View reviewed changes

deads2k reviewed Jun 23, 2017

View reviewed changes

openshift deleted a comment from openshift-bot Jun 25, 2017

bparees force-pushed the default_pruning branch from fa442ae to 083e672 Compare June 26, 2017 16:55

gabemontero approved these changes Jun 26, 2017

View reviewed changes

bparees force-pushed the default_pruning branch from 083e672 to 1408004 Compare June 26, 2017 17:09

deads2k reviewed Jun 26, 2017

View reviewed changes

bparees force-pushed the default_pruning branch from 1408004 to 04c7484 Compare June 26, 2017 18:16

set default build prune limits for group api

17a2e33

bparees force-pushed the default_pruning branch from d039a9b to 17a2e33 Compare June 27, 2017 19:37

openshift deleted a comment from openshift-bot Jun 27, 2017

openshift deleted a comment from openshift-bot Jun 28, 2017

openshift deleted a comment from eparis Jun 28, 2017

smarterclayton merged commit 583c3bd into openshift:master Jun 28, 2017

bparees deleted the default_pruning branch June 30, 2017 21:04

		@@ -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() {

set default build prune limits for group api #14845

set default build prune limits for group api #14845

Conversation

bparees commented Jun 22, 2017 • edited Loading

bparees commented Jun 22, 2017

bparees commented Jun 22, 2017

bparees commented Jun 22, 2017

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 22, 2017 via email

bparees commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

bparees commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jun 27, 2017

smarterclayton commented Jun 27, 2017

bparees commented Jun 27, 2017

stevekuznetsov commented Jun 28, 2017

bparees commented Jun 28, 2017

stevekuznetsov commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

jorgemoralespou commented Jul 14, 2017

bparees commented Jul 14, 2017

jwforres commented Jul 14, 2017

jorgemoralespou commented Jul 14, 2017 via email • edited Loading

jorgemoralespou commented Jul 14, 2017 • edited Loading

jwforres commented Jul 14, 2017 via email

bparees commented Jul 14, 2017

jorgemoralespou commented Jul 14, 2017

spadgett commented Jul 14, 2017

bparees commented Jul 14, 2017

jorgemoralespou commented Jul 14, 2017 via email

bparees commented Jun 22, 2017 •

edited

Loading

bparees Jun 23, 2017 •

edited

Loading

jorgemoralespou commented Jul 14, 2017 via email •

edited

Loading

jorgemoralespou commented Jul 14, 2017 •

edited

Loading