PR#1381: Add a check for non-ready builds in requires resolver - fm-orchestrator

fm-orchestrator

#1381 Add a check for non-ready builds in requires resolver

Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.

vmaljulin/fm-orchestrator FACTORY-2212 into master

Add a check for non-ready builds (siblings) in requires resolver

Valerij Maljulin • 4 years ago

c4a95d0

module_build_service/models.py

file modified

+2 -1

		`@@ -438,7 +438,8 @@`
		`allowing to set additional filter for results.`
		`"""`
		`return ModuleBuild._get_last_builds_in_stream_query(`
		`- db_session, name, stream, **kwargs).first()`
		`+ db_session, name, stream, **kwargs`
		`+ ).first()`

		`@staticmethod`
		`def get_build_from_nsvc(db_session, name, stream, version, context, **kwargs):`

module_build_service/resolver/DBResolver.py

file modified

+14 -2

		`@@ -388,14 +388,26 @@`

		`if module_version is None or module_context is None:`
		`build = models.ModuleBuild.get_last_build_in_stream(`
		`- self.db_session, module_name, module_stream)`
		`+ self.db_session, module_name, module_stream`
		`+ )`
		`else:`
		`build = models.ModuleBuild.get_build_from_nsvc(`
		`- self.db_session, module_name, module_stream, module_version, module_context)`
		`+ self.db_session, module_name, module_stream, module_version, module_context`
		`+ )`

		`if not build:`
		`raise UnprocessableEntity("The module {} was not found".format(nsvc))`

		`+ for sibling_id in build.siblings(self.db_session):`
		`+ sibling_build = models.ModuleBuild.get_by_id(self.db_session, sibling_id)`
		`+ if sibling_build.state not in (`
		`+ models.BUILD_STATES["ready"], models.BUILD_STATES["failed"]`
		`+ ):`
		`+ raise UnprocessableEntity('Buildrequire {}-{}-{} is in "{}" state'.format(`
		`+ sibling_build.name, sibling_build.stream, sibling_build.version,`
		`+ models.INVERSE_BUILD_STATES[sibling_build.state]`
		`+ ))`
		`+`
		`commit_hash = None`
		`mmd = build.mmd()`
		`mbs_xmd = mmd.get_xmd().get("mbs", {})`

tests/test_resolver/test_db.py

file modified

+36

		`@@ -30,6 +30,7 @@`
		`from module_build_service import models, utils, Modulemd`
		`from module_build_service.utils import import_mmd, mmd_to_str, load_mmd`
		`from module_build_service.models import ModuleBuild`
		`+ from module_build_service.errors import UnprocessableEntity`
		`import tests`


		`@@ -188,6 +189,41 @@`
		`}`
		`}`

		`+ def test_resolve_requires_exception(self, db_session):`
		`+ build = models.ModuleBuild.get_by_id(db_session, 2)`
		`+ resolver = mbs_resolver.GenericResolver.create(db_session, tests.conf, backend="db")`
		`+ with pytest.raises(UnprocessableEntity):`
		`+ resolver.resolve_requires(`
		`+ [":".join(["abcdefghi", build.stream, build.version, build.context])]`
		`+ )`
		`+`
		`+ def test_resolve_requires_siblings(self, db_session):`
		`+ tests.clean_database()`
		`+ resolver = mbs_resolver.GenericResolver.create(db_session, tests.conf, backend="db")`
		`+ mmd = load_mmd(tests.read_staged_data("formatted_testmodule"))`
		`+ for i in range(3):`
		`+ build = tests.module_build_from_modulemd(mmd_to_str(mmd))`
		`+ build.build_context = "f6e2aeec7576196241b9afa0b6b22acf2b6873d" + str(i)`
		`+ build.runtime_context = "bbc84c7b817ab3dd54916c0bcd6c6bdf512f7f9c" + str(i)`
		`+ build.state = models.BUILD_STATES["ready"]`
		`+ db_session.add(build)`
		`+ db_session.commit()`
		`+`
		`+ build_one = ModuleBuild.get_by_id(db_session, 2)`
		`+ nsvc = ":".join([build_one.name, build_one.stream, build_one.version, build_one.context])`
		`+ result = resolver.resolve_requires([nsvc])`
		`+ assert result == {`
		`+ "testmodule": {`
		`+ "stream": build_one.stream,`
		`+ "version": build_one.version,`
		`+ "context": build_one.context,`
		`+ "ref": "65a7721ee4eff44d2a63fb8f3a8da6e944ab7f4d",`
		`+ "koji_tag": None`
		`+ }`
		`+ }`
		`+`
		`+ db_session.commit()`
		`+`
		`def test_resolve_profiles(self, db_session):`
		`"""`
		`Tests that the profiles get resolved recursively`

vmaljulin commented 4 years ago

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

mikeb commented 4 years ago

Build #351 failed (commit: 5b049d8f825e97ab2f19f2d014820003f968d203).
Rebase or make new commits to rebuild.

pretty please pagure-ci rebuild

4 years ago

cqi commented on line 11 of module_build_service/resolver/DBResolver.py 4 years ago

Is models.ModuleBuild.get_last_build_in_stream(self.db_session, module_name, module_stream, state=BUILD_STATES["ready"]) enough for this purpose?

vmaljulin commented 4 years ago

Is models.ModuleBuild.get_last_build_in_stream(self.db_session, module_name, module_stream, state=BUILD_STATES["ready"]) enough for this purpose?

What did you mean?

cqi commented 4 years ago

@vmaljulin

Scratch my previous comment, the original _get_last_builds_in_stream_query only queries builds in ready state. However, the thought behind that comment is still valid here. To ensure to raise error The module {} is not in ready state, I think we just need to check the return value from get_last_build_in_stream which simply returns first object from query result returned from _get_last_builds_in_stream_query? E.g.

if not get_last_build_in_stream(...):
    raise UnprocessableEntity("The module {} is not in ready state".format(nsvc))

cqi commented 4 years ago

Instead of focusing on get_last_build_in_stream, argument state should be passed to get_build_from_nsvc in my opinion.

Meanwhile, if neither get_last_build_in_stream nor get_build_from_nsvc returns a module build , it means no module build is in ready state yet, and the subsequent if not build: handles that already. But, I think error message The module {} was not found should be updated like Module {} in ready state was not found..

jkaluza commented 4 years ago

Hm, should this PR fix following situation?

1) User A submits module foo to MBS. MBS does module stream expansion and finds out that this module foo can be built against multiple platform modules (for example). Therefore it starts multiple module builds like foo:1:1:c1 and foo:1:1:c2.
2) The foo:1:1:c1 finishes and is in ready state while foo:1:1:c2 is still building and is in build state.
3) Now, the user B submits bar module which buildrequires foo:1. MBS tries to find out the latest version of foo:1 in ready state and finds out foo:1:1:c1, which is correct. But it should also find out foo:1:1:c2, because that's also the latest version. The issue is it is not in ready state yet, because it is still building.

If that's the case you are trying to solve, then the current code is not enough, because resolve_requires is only called after the module stream expansion of module B in utils/mse.py and the input for this call is always complete NSVC, so this part of code you are changing is imho not even used outside of the tests.

I know @mprahl in that jira ticket advised changing this part of code, but I think he was not right :(.

I think what should be done here is to change get_buildrequired_modulemds and get_module_modulemds to check all the module builds defined by build.siblings for each build returned by this method. If some of the siblings are in the build, wait or init state, the exception should be raised.

cqi commented 4 years ago

I think what should be done here is to change get_buildrequired_modulemds and get_module_modulemds to check all the module builds defined by build.siblings for each build returned by this method. If some of the siblings are in the build, wait or init state, the exception should be raised.

@jkaluza I think these two function calls should be in the code path from

mmds_for_resolving = get_mmds_required_by_module_recursively(
    db_session, current_mmd, default_streams, raise_if_stream_ambigous)

inside generate_expanded_mmds, right?

If yes, module builds haven't stored into database yet, seems build.siblings does not work at that moment.

jkaluza commented 4 years ago

The build I mentioned is the ModuleBuild buildrequired by a build which is just being submitted (and handled in generate_expanded_mmds), so it will exist.

rebased onto 32427b4984cb32cd5a117dfaad0285cb8ca104d0

4 years ago

rebased onto b48b98ff9e1b40282683d97b05a19c4e189bf35d

4 years ago

rebased onto 1c96dd712e4a16e0943452691c3e9ff2e0538ab0

4 years ago

vmaljulin commented 4 years ago

Remade

pretty please pagure-ci rebuild

4 years ago

rebased onto f9323a42487eb8c6c079b16bff0144a1c1753c77

4 years ago

mprahl commented on line 6 of module_build_service/models.py 4 years ago

Could you please explain why you made this change? I don't see where this is called without both name and stream.

mprahl commented on line 22 of module_build_service/resolver/DBResolver.py 4 years ago

Could you add a more descriptive error so the user knows which module build is still building?

mprahl commented on line 17 of module_build_service/resolver/DBResolver.py 4 years ago

This check would also need to happen in the other resolver plugin.

rebased onto f2c62eb16f050c09bd10cb9f5209aaa33cbcbd7b

4 years ago

rebased onto c4a95d0

4 years ago

Pull-Request has been merged by vmaljulin

4 years ago

Metadata

Assignee

None

Tags

No Tags

fm-orchestrator

Source Code

#1381 Add a check for non-ready builds in requires resolver Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin. vmaljulin/fm-orchestrator FACTORY-2212 into master

Metadata

Flags

Changes Summary 3

#1381 Add a check for non-ready builds in requires resolver

Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.

vmaljulin/fm-orchestrator FACTORY-2212 into master