Add builds/status and buildconfigs/status subresource #10061

csrwng · 2016-07-27T18:48:45Z

Introduces 2 new subresources for builds and buildconfigs

builds/status
buildconfigs/status

For builds, any modifications to the build.Status struct should now be done through the builds/status subresource, with the exception of the build.Status.Cancelled flag. That flag can still be modified through the regular builds resource. Modifications to the build.Spec can only be done through the builds resource.

For buildConfigs, modifications to the buildconfig.Status struct are now only allowed through the buildconfigs/status subresource. Modifications to the buildconfig.Spec struct are only allowed through the buildconfigs resource with the exception of changes to the buildconfig.Spec.Triggers[x].ImageChange.LastImageID field. That field can be modified through the buildconfigs/status subresource.

The intent of these changes is to separate the use cases for regular updates vs status updates. In general, only the system (controllers, API server) should be allowed to change the status of an object. And only users should be allowed to change the Spec of the object. In the case of builds, a build should generally be immutable from a regular user's perspective. The exception to this is the cancel operation.

The build controller service account no longer needs to have create permission on the builds/source, builds/docker, builds/custom subresources because it only needs to do builds/status updates.

Fixes #6886

csrwng · 2016-07-27T18:49:01Z

[test]

csrwng · 2016-07-27T18:49:28Z

@bparees ptal

csrwng · 2016-07-27T18:51:23Z

@openshift/api-review

liggitt · 2016-07-27T18:52:16Z

pkg/build/api/validation/validation.go

+func ValidateBuildStatusUpdate(build *buildapi.Build, older *buildapi.Build) field.ErrorList {
+	allErrs := field.ErrorList{}
+	// Ensure that the cancelled flag is not changed in a status update
+	if older.Status.Cancelled != build.Status.Cancelled {


why do we have a field in status that can't be changed by updating status?

because we have made bad choices in life.

It shouldn't have been put in status in the first place.

Cancelled is user input. It should be reflected into status, but they are two separate fields.

Users set spec cancelled.

Build controller reacts with status cancelled.

Not the same field.

Also, cancelled is one way.

smarterclayton · 2016-07-27T20:00:45Z

pkg/build/api/validation/validation.go

+
+func validateBuildConfigTriggerStatusUpdate(trigger buildapi.BuildTriggerPolicy, older buildapi.BuildTriggerPolicy, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if trigger.Type != older.Type {


Please use a deep reflection and exclude the fields you want to copy (see upstream for examples) instead of doing this. This is extremely fragile - we want a "whitelist" approach for "is this allowed to change, not blacklist.

Makes sense, fixed

deads2k · 2016-07-28T12:07:40Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -117,12 +117,7 @@ func init() {
 				// BuildController.BuildUpdater (OSClientBuildClient)
 				{
 					Verbs:     sets.NewString("update"),
-					Resources: sets.NewString("builds"),


You can't do this here. Doing this will break old controllers running against new masters, right?

Yes it will, I didn't know that was a thing :)
What's the use case ?

csrwng · 2016-07-28T16:54:26Z

@deads2k updated policy and update strategies to allow the previous type of updates (and TODOs to get rid of them when appropriate)

csrwng · 2016-07-28T18:18:37Z

flake #9775
[test]

csrwng · 2016-07-29T16:09:20Z

Added commit to move the Cancelled flag to the Build.Spec in the internal Build type based on @bparees suggestion.

csrwng · 2016-07-29T16:09:32Z

[test]

csrwng · 2016-07-29T17:34:17Z

flake #9775
[test]

csrwng · 2016-07-29T21:33:35Z

Flake #9959 on
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/4240/

csrwng · 2016-07-29T21:38:18Z

[test]

stevekuznetsov · 2016-07-29T22:58:49Z

@csrwng I guess this could have been clearer -- the job that failed that we were looking for was technically the super-job, not the integration sub-job. When the new feature gets rolled out again I'll send out a clearer e-mail.

csrwng · 2016-07-30T15:26:52Z

Flake #10080
Flake #9959
[test]

csrwng · 2016-07-30T16:50:24Z

flake #9959
[test]

stevekuznetsov · 2016-08-01T19:22:20Z

re[test]

bparees · 2016-08-01T19:51:29Z

pkg/build/client/clients.go

@@ -63,6 +36,12 @@ func (c OSClientBuildClient) Update(namespace string, build *buildapi.Build) err
 	return e
 }

+// Update updates build status using the OpenShift client.


UpdateStatus

smarterclayton · 2016-08-02T19:02:23Z

pkg/build/registry/buildconfig/strategy.go

+	newBC.Spec = oldBC.Spec
+	// Allow changes to triggers for LastImageID. If anything else in triggers is
+	// changed, validation of the update will fail.
+	newBC.Spec.Triggers = newTriggers


This is wrong.

This is because we're keeping status inside the trigger spec (LastTriggeredImageID). I'm all in favor of moving it to the status field. Thoughts?

It's not backwards compatible unfortunately to change it.

Couldn't we do it via conversion? keep it in the same place in v1 and eventually expose it in the right place in v2?

bparees · 2016-08-02T19:06:08Z

this PR needs to be on @pweil- 's radar as it includes significant api changes and we still want it for 3.3 hopefully.

csrwng · 2016-08-02T20:48:18Z

@bparees fixed conversion, thx to @deads2k
I needed a // +genconversion=false comment on each field that should not be converted.

csrwng · 2016-08-02T22:47:06Z

#9856
[test]

openshift-bot · 2016-08-02T22:47:30Z

Evaluated for origin test up to acffaff

openshift-bot · 2016-08-03T00:07:50Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7421/)

smarterclayton · 2016-10-20T22:20:32Z

What's the status of this?

csrwng · 2016-10-24T13:43:15Z

@smarterclayton the problem with this pull was that we wanted to convince ourselves that making a change like this would not introduce backward compatibility problems. I have since introduced extended tests for builds that test compatibility with a previous version -> #10707. Right now it's running build integration tests on the following combinations:
prev controller + prev api + current client
prev controller + current api + current client
current controller + current api + current client

At this point, I'm not sure if the existing set of tests is sufficient or we need more tests. However, I do think that doing this split of spec v. status is the right thing to do.

smarterclayton · 2016-10-25T18:15:09Z

Ok, can discuss in 1.5.

On Mon, Oct 24, 2016 at 9:43 AM, Cesar Wong [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton the problem with this
pull was that we wanted to convince ourselves that making a change like
this would not introduce backward compatibility problems. I have since
introduced extended tests for builds that test compatibility with a
previous version -> #10707
#10707. Right now it's running
build integration tests on the following combinations:
prev controller + prev api + current client
prev controller + current api + current client
current controller + current api + current client

At this point, I'm not sure if the existing set of tests is sufficient or
we need more tests. However, I do think that doing this split of spec v.
status is the right thing to do.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10061 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_py4vJwlsx6fkEev1fTYS98lkvgAhks5q3LX1gaJpZM4JWgVD
.

smarterclayton · 2016-11-18T18:47:45Z

Still has outstanding questions.

openshift-bot · 2017-01-27T01:05:31Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

csrwng · 2017-02-06T15:49:16Z

Closing for now. Will revisit in the next API revision.
Trello: https://trello.com/c/2vkORAHW/1139-add-a-status-sub-resource-to-builds-and-buildconfigs-builds

csrwng added the needs-api-review label Jul 27, 2016

liggitt reviewed Jul 27, 2016
View reviewed changes

csrwng force-pushed the build_status_subresource branch from 5886133 to 0897179 Compare July 27, 2016 19:13

smarterclayton reviewed Jul 27, 2016
View reviewed changes

csrwng force-pushed the build_status_subresource branch from 0897179 to 77f9ee3 Compare July 27, 2016 23:38

deads2k reviewed Jul 28, 2016
View reviewed changes

csrwng force-pushed the build_status_subresource branch from 77f9ee3 to 61f1bbd Compare July 28, 2016 16:52

csrwng force-pushed the build_status_subresource branch from 61f1bbd to 0d40e06 Compare July 29, 2016 16:07

bparees reviewed Aug 1, 2016
View reviewed changes

csrwng force-pushed the build_status_subresource branch from 0d40e06 to f5dc258 Compare August 2, 2016 14:15

smarterclayton reviewed Aug 2, 2016
View reviewed changes

csrwng added 2 commits August 2, 2016 16:40

Add builds/status and buildconfigs/status subresource

8a4546b

Move Build Cancelled flag from Status to Spec

acffaff

csrwng force-pushed the build_status_subresource branch from f5dc258 to acffaff Compare August 2, 2016 20:47

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2016

csrwng mentioned this pull request Sep 6, 2016

Update build status to indicate build failure reason #10817

Merged

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 1, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 11, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 2, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 15, 2016

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2017

csrwng closed this Feb 6, 2017

Add builds/status and buildconfigs/status subresource #10061

Add builds/status and buildconfigs/status subresource #10061

Conversation

csrwng commented Jul 27, 2016

csrwng commented Jul 27, 2016

csrwng commented Jul 27, 2016

csrwng commented Jul 27, 2016

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

csrwng commented Jul 28, 2016

csrwng commented Jul 28, 2016

csrwng commented Jul 29, 2016

csrwng commented Jul 29, 2016

csrwng commented Jul 29, 2016

csrwng commented Jul 29, 2016 • edited Loading

csrwng commented Jul 29, 2016

stevekuznetsov commented Jul 29, 2016

csrwng commented Jul 30, 2016

csrwng commented Jul 30, 2016

stevekuznetsov commented Aug 1, 2016

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 commented Aug 2, 2016

csrwng commented Aug 2, 2016

csrwng commented Aug 2, 2016

openshift-bot commented Aug 2, 2016

openshift-bot commented Aug 3, 2016

smarterclayton commented Oct 20, 2016

csrwng commented Oct 24, 2016

smarterclayton commented Oct 25, 2016

smarterclayton commented Nov 18, 2016

openshift-bot commented Jan 27, 2017

csrwng commented Feb 6, 2017

csrwng commented Jul 29, 2016 •

edited

Loading