Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

[KOGITO-1771] Checking build status before triggering a new build #323

Merged
merged 4 commits into from Apr 28, 2020
Merged

[KOGITO-1771] Checking build status before triggering a new build #323

merged 4 commits into from Apr 28, 2020

Conversation

ricardozanini
Copy link
Member

@ricardozanini ricardozanini commented Apr 24, 2020 •
edited

Signed-off-by: Ricardo Zanini zanini@redhat.com

See:
https://issues.redhat.com/browse/KOGITO-1771

Small refactoring introducing the build package. It's basically the same thing, but we will follow this up on KOGITO-1999.

I think this solved the issue, at least on my side, never happened again.

Many thanks for submiting your Pull Request ❤️!

Please make sure that your PR meets the following requirements:

  • You have read the contributors guide
  • Pull Request title is properly formatted: [KOGITO-XYZ] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
  • Your feature/bug fix has a unit test that verifies it
  • You've tested the new feature/bug fix in an actual OpenShift cluster

@ricardozanini
[KOGITO-1771] Checking build status before triggering a new build
8cccac4 
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
@ricardozanini ricardozanini added this to the v0.11.0 milestone Apr 24, 2020
@ricardozanini ricardozanini added the needs review 🔍 Pull Request that needs reviewers label Apr 24, 2020
@MarianMacik MarianMacik self-requested a review April 27, 2020 10:23
MarianMacik
MarianMacik reviewed Apr 27, 2020
View reviewed changes
pkg/util/maps.go Show resolved Hide resolved
pkg/util/maps.go Outdated Show resolved Hide resolved
pkg/util/maps.go Show resolved Hide resolved
@sutaakar
Copy link
Contributor

sutaakar commented Apr 27, 2020 •
edited

List of tests being done:

  • S2I build, update image - passed

  • runtime build, update image - blocked by https://issues.redhat.com/browse/KOGITO-2017

  • binary build, update image - blocked by https://issues.redhat.com/browse/KOGITO-2017

  • S2I build, update source - passed

  • binary build, reupload binary - passed

  • S2I build, update source while other build running - changes are not reflected and old build is finished, no new build triggered passed (original build finished first, then new one was triggered)

  • binary build, update source while other build running - passed

@ricardozanini
Copy link
Member Author

Guys, this is a workaround to fix this bug, I'll address some of your comments, but like described everything will be rewritten soon: https://issues.redhat.com/browse/KOGITO-1999

@sutaakar
Copy link
Contributor

Ok, will count with that.
Just need to make sure that there are no regressions during transition period, before KOGITO-1999 is fully implemented.

@ricardozanini
Copy link
Member Author

Ok, will count with that.
Just need to make sure that there are no regressions during transition period, before KOGITO-1999 is fully implemented.

Agreed. I was not talking about your tests, that make a lot of sense to me. :)

@ricardozanini
Copy link
Member Author

@MarianMacik can you take another look? I'll review the bug again based on your last comments on the JIRA:
https://issues.redhat.com/browse/KOGITO-1771?focusedCommentId=14061404&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14061404

@MarianMacik
Copy link
Member

@ricardozanini I looked at the updates, looks good, just found that the if can be indeed even nicer :) Other than that looks good to me so far 😉

@ricardozanini
Copy link
Member Author

@sutaakar by seeing in your tests, it looks like you haven't faced the same issues as @MarianMacik . Are you guys using the same env? Looks like the problem does not happen on CRC.

@sutaakar
Copy link
Contributor

Yes, we should be using same environment (OCP 4.4, 3 master nodes)
I haven't seen that issue yet.

@ricardozanini
Copy link
Member Author

Yes, we should be using same environment (OCP 4.4, 3 master nodes)
I haven't seen that issue yet.

@MarianMacik by looking at your logs, maybe your operator version wasn't updated.

@MarianMacik
Copy link
Member

MarianMacik commented Apr 27, 2020 •
edited

The operator was built straight from this PR. I will try to update images version as well as I use 0.9.0.

@ricardozanini BTW bear in mind it only happens to me on clusters with 3 master nodes. So on CRC it won't happen at all. I tried 4.3 and 4.4 with one and 3 master nodes and it only did for me on 3 master nodes.

by looking at your logs, maybe your operator version wasn't updated.

My logs contain log messages added in this PR so I must have used the right version.

@ricardozanini
Copy link
Member Author

I was just wondering why @sutaakar hasn’t seen it yet. I’ll take a look after my coffee.

@MarianMacik
Copy link
Member

@ricardozanini Yes, good question. Maybe I run more tests with examples? I run Quarkus native/non-native + Spring Boot, all of that with and without persistence times 4 as I test 40k, 80k, 160k and 320k. And with 40k and 80k the issue doesn't manifest as the test finishes before the second build is finished. But with 160k and 320k the build finishes in the middle of the test and a pod is restarted and thus failing my test run. So I think that's why.

@ricardozanini
Copy link
Member Author

It's a race condition problem.
OCP is starting the first build by the ImageChange event:

Build trigger cause:	Image change
Image ID:		image-registry.openshift-image-registry.svc:5000/mmacik-perf-jenkins-cucumber-565n/kogito-quarkus-ubi8-s2i@sha256:1eb22ea8b16af416179789775e3b6c7ba31c6ceb3193acd113d2f3f4333183bf
Image Name/Kind:	kogito-quarkus-ubi8-s2i:0.9.1 / ImageStreamTag

Our code is not catching this build because it's created after ours probably. In the logs, I can see that we are only triggering once. Let's try to reconcile after the BC is created, then we will have time to catch all objects.

MarianMacik
MarianMacik approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@MarianMacik MarianMacik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and issue is not reproducible anymore. Thanks for the fix & discussion!

radtriste
radtriste approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@radtriste radtriste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me
minor comment

pkg/controller/kogitoapp/build/trigger.go
return t.buildConfig != nil
}

func (t *trigger) OnBuildConfig(buildConfig *buildv1.BuildConfig) Trigger {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that one used ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, but I've decided to let it there since we will use in the KogitoBuild CR. Just to not forget this interface.

@ricardozanini ricardozanini added ready 🚀 PR is ready to be merged and removed needs review 🔍 Pull Request that needs reviewers labels Apr 28, 2020
@ricardozanini ricardozanini merged commit a63b2ae into apache:master Apr 28, 2020
@ricardozanini ricardozanini deleted the kogito-1771 branch April 28, 2020 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@spolti spolti spolti approved these changes

@sutaakar sutaakar sutaakar approved these changes

@MarianMacik MarianMacik MarianMacik approved these changes

@radtriste radtriste radtriste approved these changes

@Sgitario Sgitario Awaiting requested review from Sgitario

Assignees
No one assigned
Labels
ready 🚀 PR is ready to be merged
Projects
None yet
Milestone
v0.11.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ricardozanini @sutaakar @MarianMacik @spolti @radtriste