Add parent BuildConfig to Build OwnerReferences #12961

oatmealraisin · 2017-02-14T23:12:12Z

This is a prelim step to enabling garbage collection instead of reaper code.

@openshift/devex ptal?

oatmealraisin · 2017-02-14T23:13:23Z

bparees · 2017-02-14T23:42:54Z

pkg/build/generator/generator.go

+	// deleted when the BuildConfig is deleted
+	// TODO: is there a utility function for generating an owner reference from an object?
+	newBuild.OwnerReferences = append(newBuild.OwnerReferences, kapi.OwnerReference{
+		APIVersion: "v1",          // BuildConfig.APIVersion is not populated


we need to sort out the right way to resolve this.... we've got an internal buildconfig object, we (apparently?) need to know the api version of the actual buildconfig, or maybe just the api version of the server we're running in, to set this correctly?

@deads2k help
@jim-minter you've got a similar block in your template PR

@bparees find me on aos.

I'm very sorry I missed this, but was there a decision on what to do about this?

not exactly, but we did learn that "v1" is not the right value :)

i think the net was there isn't a good answer today because the value is supposed to be the name of the api group for builds, but builds aren't in an api group today. I think @deads2k implied that emptystring, or "core" or some other value could be used for things that aren't in an apigroup, but i'm not sure if that's true today, or something that he's hoping to make happen in the GC implementation.

i think the net was there isn't a good answer today because the value is supposed to be the name of the api group for builds, but builds aren't in an api group today. I think @deads2k implied that emptystring, or "core" or some other value could be used for things that aren't in an apigroup, but i'm not sure if that's true today, or something that he's hoping to make happen in the GC implementation.

No value you put in here will work today. Your best bet is build.openshift.io/v1 (which obviously has migration problems), but that won't work without other API groups enabled. I really wouldn't much worry about it at the moment.

@deads2k is there any intention to enable GC to work w/ non-apigroup resources in the 3.6 timeframe?

I don't see why we would want to do that. Every API should be group-ified. That should allow us to use the "stock" GC controller instead of doing the surgery required on the dynamic client to make it understand /oapi.

ok. I still don't really understand why ObjectRefs don't need apigroup values, but OwnerRefs do.

ok. I still don't really understand why ObjectRefs don't need apigroup values, but OwnerRefs do.

ObjectRefs are going to need them too, but changing it now is likely to break API consumers.

@deads2k that's got some interesting migration implications......

@deads2k that's got some interesting migration implications......

release 1: make new available, tolerate both

release 2: stop writing old, write only new, migrate old data

release 3: stop reading old and remove code cruft

We could stretch it out if we really wanted to I guess, but I think that's enough.

bparees · 2017-02-14T23:44:10Z

test/extended/builds/start.go

+	g.Describe("setting OwnerReferences", func() {
+		g.It("should set parent BuildConfig as OwnerReference", func() {
+			g.By("starting the build from buildconfig")
+			br, err := exutil.StartBuildAndWait(oc, "sample-build")


no need to wait for the build to finish, it'll be created immediately so you can just grab it and check.

Given that there's no need for the build to run, and factoring in my suggestions above, can't this all be done in existing unit tests in generator_test.go? Search there for references to generateBuildFromConfig and generateBuildFromBuild.

yeah that's probably true that this need not be an extended test.

I've moved everything to the unit tests, along with other changes.

bparees · 2017-02-14T23:44:43Z

test/extended/builds/start.go

+			br.AssertSuccess()
+			if len(br.Build.ObjectMeta.OwnerReferences) == 0 {
+				o.Expect(fmt.Errorf("Created build has no OwnerReferences")).NotTo(o.HaveOccurred())
+			}


you should make sure the reference references the right thing (proper type, name of the buildconfig matches what you'd expect)

I've moved everything to unit tests, but this is implemented

bparees · 2017-02-14T23:44:56Z

test/extended/builds/start.go

+			br.AssertSuccess()
+			if len(br.Build.ObjectMeta.OwnerReferences) == 0 {
+				o.Expect(fmt.Errorf("Created build has no OwnerReferences")).NotTo(o.HaveOccurred())
+			}


all the same comments here.

I've moved everything to unit tests, but this is implemented

jim-minter · 2017-02-15T09:57:22Z

pkg/build/generator/generator.go

+	// Add the original Build's OwnerReferences to the new Builds. This will
+	// make sure that the new Build is deleted when the original BuildConfig
+	// is deleted.
+	if len(build.OwnerReferences) > 0 {


I think this would be better as a one-liner (I don't think the if statement is necessary) in generateBuildFromBuild, where newBuild is being instantiated.

Just to confirm, the logic here is: OpenShift will always add OwnerReferences to builds instantiated from buildconfigs; if I happen to remove the OwnerReference manually and then clone that build, the resulting build will also not have an OwnerReference?

i'm not sure i'd classify it as intentional logic but yes that's the behavior and it's certainly worth noting. I'm inclined to say it's acceptable behavior... us re-attaching the reference by looking up the associated buildconfig, if anything, seems to be contrary to what the user probably intended if they removed the ownerreference.

jim-minter · 2017-02-15T10:02:52Z

pkg/build/generator/generator.go

+	// Add the BuildConfig to the Build's OwnerReference, so that it will be
+	// deleted when the BuildConfig is deleted
+	// TODO: is there a utility function for generating an owner reference from an object?
+	newBuild.OwnerReferences = append(newBuild.OwnerReferences, kapi.OwnerReference{


I think this is better going straight into func (g *BuildGenerator) generateBuildFromConfig.

The change was so simple, I didn't consider generateBuildFromConfig/Build. Thanks for pointing that out!

oatmealraisin · 2017-02-15T23:41:26Z

@openshift/devex Ready for another go around, ptal?

jim-minter · 2017-02-16T09:22:56Z

pkg/build/generator/generator.go

@@ -410,6 +410,14 @@ func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.
 		ObjectMeta: kapi.ObjectMeta{
 			Name:   buildName,
 			Labels: bcCopy.Labels,
+			OwnerReferences: []kapi.OwnerReference{
+				kapi.OwnerReference{


You don't need to repeat kapi.OwnerReference - { should do here.

jim-minter · 2017-02-16T09:23:07Z

pkg/build/generator/generator_test.go

@@ -1103,6 +1107,14 @@ func TestGenerateBuildFromBuild(t *testing.T) {
 				buildapi.BuildJenkinsBuildURIAnnotation:   "baz",
 				buildapi.BuildPodNameAnnotation:           "ruby-sample-build-1-build",
 			},
+			OwnerReferences: []kapi.OwnerReference{
+				kapi.OwnerReference{


You don't need to repeat kapi.OwnerReference - { should do here.

jim-minter · 2017-02-16T09:26:53Z

pkg/build/generator/generator_test.go

@@ -1137,6 +1149,9 @@ func TestGenerateBuildFromBuild(t *testing.T) {
 	if _, ok := newBuild.ObjectMeta.Annotations[buildapi.BuildPodNameAnnotation]; ok {
 		t.Errorf("%s annotation exists, expected it not to", buildapi.BuildPodNameAnnotation)
 	}
+	if len(newBuild.OwnerReferences) == 0 || newBuild.OwnerReferences[0].Name != build.OwnerReferences[0].Name || newBuild.OwnerReferences[0].Kind != build.OwnerReferences[0].Kind {


This statement currently has no effect. How about:

if !reflect.DeepEqual(build.ObjectMeta.OwnerReferences, newBuild.ObjectMeta.OwnerReferences) { t.Errorf("Build OwnerReferences does not match the original Build OwnerReferences") }

jim-minter · 2017-02-16T09:28:30Z

pkg/build/generator/generator_test.go

@@ -841,6 +842,9 @@ func TestGenerateBuildFromConfig(t *testing.T) {
 	if build.Annotations[buildapi.BuildNumberAnnotation] != "13" {
 		t.Errorf("Build number annotation value %s does not match expected value 13", build.Annotations[buildapi.BuildNumberAnnotation])
 	}
+	if len(build.OwnerReferences) == 0 { //|| build.OwnerReferences[0].Kind != "BuildConfig" || build.OwnerReferences[0].Name != bc.Name {


I think testing all the fields is probably a good plan.

oatmealraisin · 2017-02-16T22:21:12Z

@jim-minter Implemented your requests.

jim-minter · 2017-02-17T09:16:57Z

lgtm - squash & then good to go.

bparees · 2017-02-17T19:19:11Z

lgtm, will merge after 3.5 branch is created.

oatmealraisin · 2017-02-17T19:49:46Z

Squashed

bparees · 2017-02-21T18:30:07Z

[merge]

openshift-bot · 2017-02-21T18:37:29Z

Evaluated for origin merge up to 8c0f0b0

openshift-bot · 2017-02-21T18:37:29Z

[Test]ing while waiting on the merge queue

openshift-bot · 2017-02-21T18:45:37Z

Evaluated for origin test up to 8c0f0b0

openshift-bot · 2017-02-21T20:16:45Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/397/) (Base Commit: 2d20080)

openshift-bot · 2017-02-21T21:53:49Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/405/) (Base Commit: 20682f2) (Image: devenv-rhel7_5948)

bparees reviewed Feb 14, 2017

View reviewed changes

bparees self-assigned this Feb 14, 2017

jim-minter reviewed Feb 15, 2017

View reviewed changes

jim-minter reviewed Feb 16, 2017

View reviewed changes

bparees added this to the 1.6.0 milestone Feb 17, 2017

Add parent BuildConfig to Build OwnerReferences

8c0f0b0

openshift-bot merged commit 90aaad3 into openshift:master Feb 21, 2017

Add parent BuildConfig to Build OwnerReferences #12961

Add parent BuildConfig to Build OwnerReferences #12961

Conversation

oatmealraisin commented Feb 14, 2017

oatmealraisin commented Feb 14, 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

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

oatmealraisin commented Feb 15, 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

oatmealraisin commented Feb 16, 2017

jim-minter commented Feb 17, 2017

bparees commented Feb 17, 2017

oatmealraisin commented Feb 17, 2017

bparees commented Feb 21, 2017

openshift-bot commented Feb 21, 2017

openshift-bot commented Feb 21, 2017

openshift-bot commented Feb 21, 2017

openshift-bot commented Feb 21, 2017

openshift-bot commented Feb 21, 2017 • edited Loading

openshift-bot commented Feb 21, 2017 •

edited

Loading