Modular support

[mmathesius]

Some cleanup of the code to fix all old flake8 complaints, then work in progress for modular support.

[mmathesius]

I've currently gotten as far as getting several of the functions to deal with modules, then extracting and parsing modulemd data from koji to get the list of component RPMs with associated info for each. I need advice on how to proceed with syncing the modular components.

[contyk]

I'd drop the %(stream)s here; if anything, it could be %(ref)s which we also discussed but since we're looking for the modulemd-provided ref in the entire source repo, I'd leave it blank like in the defaults/rpms section.

[mmathesius]

Thanks! I'll make that revision, and also correct the description of the format strings below to make it clear that %(name) and %(ref) can only be used in the module sub-component defaults.

[mmathesius]

There's a few small isolated details that need to be looked at yet (commented with TODO), but I think this is now ready enough for a good solid review.

[mmathesius]

I think I got the tag syncing corrected now.

@contyk I'm taking away the "WIP" in the PR title now. Please review.

[contyk]

I gave this a decent read, except for the tests where I just trust you :)

My only comment would be on the ImportError exceptions which are, I believe, new in 3.9; older versions of Python raise ValueError on failed imports and the try blocks would therefore not catch these. Since we currently run Distrobaker on RHEL, I think this should be changed to catch both types of exception.

Otherwise I'd say we're good to merge this. Maybe @hanzz may have some comments, too, though.

[mmathesius]

I gave this a decent read, except for the tests where I just trust you :)

Thanks for the review!

My only comment would be on the ImportError exceptions which are, I believe, new in 3.9; older versions of Python raise ValueError on failed imports and the try blocks would therefore not catch these. Since we currently run Distrobaker on RHEL, I think this should be changed to catch both types of exception.

After a little research into that, it appears that the ValueError exception may have happened in certain cases for relative imports only, so it wouldn't have been a problem. Nevertheless, I just pushed an update so it uses Exception for consistency with nearly every other exception case in distrobaker.

I also pushed a small additional commit to eliminate unnecessary parameters being passed to clone_destination_repo() and fetch_upstream_repo(). And one more small commit that pre-configures the logger when running tests to make it really easy to enable debugging log output.

[mmathesius]

Two additional commits were just pushed. The first commit fixes a small pre-existing bug that the mandatory control.build option was being ignored. The second commit adds some basic unit tests for the repo manipulation functions.

[mmathesius]

Two additional commits were just pushed. The first commit corrects a race condition in the previously added unit tests for the repo manipulation functions. The second commit corrects a bug with an incorrectly formed scmurl being used when submitting a module build to the MBS. (eg., the name of the git repo was being specified as module:stream repo rather than just module)

[mmathesius]

@contyk @hanzz Do you have any objections to merging this--so OSCI can start their refactoring work?

CC: @bookwar

[contyk]

No objections from me. I think we should merge this so that OSCI can start refactoring to make it suit their needs.

[mmathesius]

One additional commit was just pushed to add logic to skip the MBS generated *-devel modules. Otherwise errors are thrown because there is no corresponding SCM modulename-devel repo.

[jankaluza]

FYI, I will start work on adding this to our distrobuildsync and distrogitsync so we can deploy this internally and do some possible tests. I will also opensource distrobuildsync and distrogitsync after that - let's discuss the best way how to do that separately.

[jankaluza]

@mmathesius , reading the code more deeply while backporting it to distrogitsync, I have some extra questions.

It seems the synced (downstream) module uses the original (upstream) modulemd file. If upstream module contains a component built from the stream-foo branch (just for example), then also the downstream module will be built from whatever was the latest HEAD of upstream stream-foo branch.

This means that the sync at first syncs the stream-foo branch so its downstream HEAD points to the same HEAD or ref as used by the upstream stream-foo branch. Once this is done, the downstream module is built and because the downstream HEAD is the same as upstream HEAD, also the component in the downstream module is the same. So far so good.

I think this is somehow fragile. Imagine two modules using the component from the very same stream-foo branch. The first module is submitted for a build and is being built. Note that module build can take days in case of issues which the maintainer decides to fix by resubmit of existing failed module build. In the meantime, there are some commits in the stream-foo branch and the second module is built from these commits. This results in internal HEAD of stream-foo pointing to newer version than the one used in the first module build. If this module build succeeds, the downstream rebuild will use newer stream-foo HEAD than the upstream one.

The same situation can happen if module build uses component from c9s branch and there are some commits done to this branch during the module build.

Another example is if first module uses particular commit hash for component (does the current PR supports it?) and second uses the branch name.

This also makes the module build sync hard to replicate in case of any downtime of any service. Imagine the internal MBS or Brew is down and we cannot sync the first module. If this happens and there are some commits done to stream-foo in the meantime and the second module is built, we cannot simply run distrobaker for first module, because the stream-foo HEAD is already to new for the first module build.

I understand it is not easy or maybe even not possible to sync modules in some better way, but at the same time I think this proposed way is too fragile.

I believe RFE for MBS to allow specifying particular commit hashes of components included in a modulemd file which would then be used instead of the original ref would be way to go here. That way, we could just keep stream-foo always in 1:1 sync with c9s and while submitting the module build to MBS, we would instruct downstream MBS to use the very same commit hashes as upstream MBS and there would be no chance that some strange race condition between builds would mess things up.

If you are not against this approach, I would do a PR against MBS to support this.

[mmathesius]

@hanzz Your approach sounds reasonable to me.
@contyk Do you have any thoughts on this?

[jankaluza]

@mmathesius https://pagure.io/fm-orchestrator/pull-request/1725