Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing builder on Image does not trigger Build if last Build failed #1198

Open
kieron-dev opened this issue Apr 26, 2023 · 13 comments
Open

Comments

@kieron-dev
Copy link
Contributor

kieron-dev commented Apr 26, 2023

Suppose I have two Builders - one with a nodejs buildpack only, and other with a java buildpack only.

If I create an Image referencing some nodejs source code, but mistakenly using the java Builder, a Build will be created that will fail at the detection phase.

If I then modify the Image to point to the correct nodejs buildpack, I would expect a new Build to be generated and for it to succeed. However no new Build is created. I don't believe this is the correct behaviour.

This line might be the problem: https://github.com/pivotal/kpack/blob/9081a1861d6551e86110653f80259c9a17c2b28f/pkg/reconciler/image/build_required.go#L108

@kieron-dev
Copy link
Contributor Author

I've noticed that a build also is not created when the latest build was successful and then the only modification to the image is to change the builder ref. This also feels like a build should be created.

@tomkennedy513
Copy link
Collaborator

I've noticed that a build also is not created when the latest build was successful and then the only modification to the image is to change the builder ref. This also feels like a build should be created.

Does the new builder have a different version of any of the buildpacks used in the latest successful build or a different stack?

@kieron-dev
Copy link
Contributor Author

Does the new builder have a different version of any of the buildpacks used in the latest successful build or a different stack?

Ah - it's the same buildpack. That would be why then. (The main point of this issue is still valid though).

@tomkennedy513
Copy link
Collaborator

I think the problem you are running into is not really a bug, but more of a limitation in the the way kpack processes changes that result in new builds being created. If the last build failed, kpack doesn't know if the buildpacks changed by changing the builder reference. To get the behavior you are looking for, I think kpack would need to introduce a new change reason that would look specifically for a change in the builderRef as opposed to the stack or buildpacks inside a builder changing

@tomkennedy513
Copy link
Collaborator

I think a better workaround that would fix this issue for you in the short term would be to add some sort of environment variable to the image with a time stamp or cf push info every time you update the image which will always cause a new build to be generated

@kieron-dev
Copy link
Contributor Author

We don't really want to make non-changes to force builds. At the moment kpack restages or rebuilds images when stacks or buildpacks change in the background, then all that is required is a cf restage for korifi apps to pick up the latest build. If we forced a change to the env, we would duplicate all those background builds.

Is it not a good thing if changing a buildpack always causes a rebuild? It's possible for the reconciler to store previous values in image annotations or somewhere in the image status so you can determine whether a buildpack has changed without relying on a previously successful build.

@tomkennedy513
Copy link
Collaborator

Is it not a good thing if changing a buildpack always causes a rebuild? It's possible for the reconciler to store previous values in image annotations or somewhere in the image status so you can determine whether a buildpack has changed without relying on a previously successful build.

The main problem here is that we don't want to trigger a rebuild on any buildpack change in a builder, only on buildpack changes for the buildpacks that were actually used by the previous build. If we triggered rebuilds on every buildpack change, we would end up with a ton of unnecessary rebuilds on the cluster

@kieron-dev
Copy link
Contributor Author

I only mean reacting to a change in the builder reference on an image. Isn't that desirable?

@tomkennedy513
Copy link
Collaborator

I only mean reacting to a change in the builder reference on an image. Isn't that desirable?

Yes, I think that would be our end goal. We could add that logic to the existing change config build reason or potentially create a new reason. Depending on prioritization, I'm not sure when we'd implement this feature though. That is my main reason for suggesting that you annotate the image every time its updated as a workaround.

@tomkennedy513
Copy link
Collaborator

We don't really want to make non-changes to force builds.

Can you describe a little what you mean by this

@kieron-dev
Copy link
Contributor Author

We don't really want to make non-changes to force builds.

Can you describe a little what you mean by this

I think you were suggesting setting a build env var to a unique value to trigger a kpack build when the sources haven't changed, but the user has selected a new buildpack.

We could do this every time we get a buildworkload in korifi (which is the result of a cf push or restage). However, there are some cases, like restaging to pick up a new stack, where the latest build on the image is already the build we need, and we wouldn't want to waste time performing the build again.

However, we do check that a kpack build gets created for the current generation of a kpack image associated with an app. When it doesn't, we set a failure message on the buildworkload. But I suppose, we could set a build env var on the image at that point to recover from that situation, rather than telling the user to update the sources and re-push as we do at the moment.

@tomkennedy513
Copy link
Collaborator

I was suggesting adding some data to the annotations that was derived from the the input from the user (ie. the buildpacks they requested) because that would cause a new build to occur in the event that they changed their buildpack.

kieron-dev pushed a commit to cloudfoundry/korifi that referenced this issue May 10, 2023
Kpack would not trigger a build if the builder in the kpack image
changes and latest build had failed (e.g. push with buildpack A that
fails and then try to push with buildpack B) - see
buildpacks-community/kpack#1198

In order to work around this kpack limitation we annotate the latest
build with `image.kpack.io/additionalBuildNeeded` annotation that forces
kpack into creating a new build thus picking up the builder specified in
the kpack image.

Co-authored-by: Kieron Browne <kbrowne@vmware.com>
kieron-dev pushed a commit to cloudfoundry/korifi that referenced this issue May 11, 2023
Kpack would not trigger a build if the builder in the kpack image
changes and latest build had failed (e.g. push with buildpack A that
fails and then try to push with buildpack B) - see
buildpacks-community/kpack#1198

In order to work around this kpack limitation we annotate the latest
build with `image.kpack.io/additionalBuildNeeded` annotation that forces
kpack into creating a new build thus picking up the builder specified in
the kpack image.

Co-authored-by: Kieron Browne <kbrowne@vmware.com>
kieron-dev pushed a commit to cloudfoundry/korifi that referenced this issue May 11, 2023
Kpack would not trigger a build if the builder in the kpack image
changes and latest build had failed (e.g. push with buildpack A that
fails and then try to push with buildpack B) - see
buildpacks-community/kpack#1198

In order to work around this kpack limitation we annotate the latest
build with `image.kpack.io/additionalBuildNeeded` annotation that forces
kpack into creating a new build thus picking up the builder specified in
the kpack image.

Co-authored-by: Kieron Browne <kbrowne@vmware.com>
@kieron-dev
Copy link
Contributor Author

Thanks for you help. We've implemented the workaround now (in this commit) where if our change to the kpack image results in a new generation, but not a new build, then we annotation the latest build with the BuildNeededAnnotation, triggering a new build on the image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants