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

fix MDBF-414 #156

Merged
merged 1 commit into from May 31, 2022
Merged

fix MDBF-414 #156

merged 1 commit into from May 31, 2022

Conversation

fauust
Copy link
Contributor

@fauust fauust commented May 18, 2022

@grooverdan how could I test my changes without merging, do you want me to add the same logic as for install/upgrade script (see: https://github.com/MariaDB/mariadb.org-tools/blob/master/buildbot.mariadb.org/scripts/bash_lib.sh#L28).

One suggested approach is that we know that https://buildbot.mariadb.org/#/builders/311/builds/9150 does not clean the environment. So I could compare 2 run (one before the patch and the one after).

@fauust
Copy link
Contributor Author

fauust commented May 18, 2022

And now https://buildbot.mariadb.org/#/builders/311/builds/9150 passes :) I just rebuild https://buildbot.mariadb.org/#/builders/311/builds/8838. Not sure that I understand exactly what the problem is, I need to understand better this script.

Copy link
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

I haven't looked at bash_lib.sh yet.

The general principle with this script is that it will get invoked for each architecture, and when the magic number of expected architectures is reached in line L235 (on the original), the manifest is pushed.

Between runs it needs to maintain its incomplete state. An important consequence of this is that the end of the script needs to clear the trap EXIT handler so we aren't purging out a manifest before its completed by subsequent build runs.

We are relying a lot of the L247 onwards to clear out partial manifests and their images that are incomplete because the build wasn't triggered for arch X/Y/Z. A failure bb trigger could preemptively clearly these. This would leave the case of a retrigger happy user or bb restarts that are handled.

There could also be logical that a re-triggered build replaces a image in the manifest of the same arch rather than adding to it otherwise we get like 10.9-mdev-20119-misc 3x arm64 builds. But overall this is a nice to have.

@fauust
Copy link
Contributor Author

fauust commented May 18, 2022

I haven't looked at bash_lib.sh yet.

The idea is that instead of using positional arguments or having to call the script with all needed arguments (or settings env vars), you only need to provide the build URL from buildbot and it will get all env variables directly from the BB API. This could even be applied for any invocation of the script (via BB or manually) but as discussed with @vladbogo it's going to generate more calls on BB API and it's probably less scalable. And if we find a way to template or create generic function for build/test calls from BB then the benefit of this is lesser in simplifying BB configuration files.

Finally the bash_lib.sh provides logging options that permits more homogeneity (and easier maintenance if we decide to log things differently in the future).

@grooverdan
Copy link
Member

Need to clear the trap EXIT at the end and then I'm happy with this as a major improvement.

@fauust
Copy link
Contributor Author

fauust commented May 23, 2022

Need to clear the trap EXIT at the end and then I'm happy with this as a major improvement.

Not sure what you mean...

@grooverdan
Copy link
Member

Without clearing the trap EXIT handler at the end of this script, it will execute the handler on successful exit. This is undesireable as the build of quay.io manifests relies on the state (i.e. incomplete manifest and images) to exist in future runs for other build architecutes which push when the right number of architecture images are in the manifest and ready to be pushed.

Make sure that we clean properly containers.
See: https://jira.mariadb.org/browse/MDBF-414
@fauust fauust changed the title WIP: fix MDBF-414 fix MDBF-414 May 30, 2022
@fauust
Copy link
Contributor Author

fauust commented May 30, 2022

Done.

@grooverdan grooverdan merged commit a4cacaa into MariaDB:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants