If a CG calls init_build(), then later calls init_build() with the same build data, will this cause the second call to fail? (That's my hoped-for behaviour...)

If a CG calls init_build(), then later calls init_build() with the same build data, will this cause the second call to fail? (That's my hoped-for behaviour...)

Not now - it will return same build id as previous call. Anyway, I can add it.

I need to look at this more, but I have concerns.

The sanity check is weakened in recycle_build. While there are new sanity checks added elsewhere, but it still worries me.

The handling of strict in new_build seems backwards.

I'm not sure about using build.extra for the reservation data. Maybe it's ok, but it feels off.

Does that init_build() method belong in CG_Importer? It doesn't use self.

I've rewritten a part of it to not use recycle_build at all, fixed strict issue and put init_build into cg_init_build completely.

I'm not sure about using build.extra. Would it be cleaner to introduce new build state aka RESERVED?

Would it be cleaner to introduce new build state aka RESERVED?

It's not so much an extra state. The BUILDING state already pretty much means this.

The main thing here is ensuring that the entity that does the final import matches the entity that performed the reservation. With the old way, we had:

• a dedicated field in the build table (task_id) to indicate the entity
• a pretty good way to validate the entity (check host, check that host has that task id open)
• strong constraints ensuring that only one host can have a given task open
• an additional state progression sanity check

So here, we need some way to record and validate the entity. This is tricky since all we really have is the content generator info, and we don't know how the CG may be structured (e.g. could the CG have parallel processes performing builds and talking to Koji about it?).

The build.extra field is there and can hold any kind of data, so it's certainly possible to put some reservation data there. It just feels a little off to me. This data is not about the build result, it's technical process stuff.

If we don't use build.extra, I guess we're talking a schema change. Either a new field for build or a new table.

Apart from that, we don't have as strong an entity identifier to work with. A CG id is much less specific than a task id. Should we allow the CG to specify some sort of reservation key value? Or just return one to them? Is that too paranoid?

Upside down: Reservation token seems right to me. I was also wondering, how I should enforce, that same entity which reserved value is using it later. I would create random value to store with build and return to CG. I wouldn't allow CG to set up its own - it could lead to crappy implementation with fixed tokens or predictable ones. It is better for us to be handling it.

In such case, it could be valuable to store this token in extra data forever for audit reasons. Maybe, there is no real use case to track it after build was imported. But what I can imagine is that CG is able to prove, that it was this source CG which imported final data (than just showing build ID).

In such light, separate db field makes sense {'reserved_by_cg: True, 'token': 'xyz'}. On the other hand, it is a field which will not be used in most of the cases. We still can have it as a subfield of build.extra. For separation there could be something like similar json-field build.reservation (or somewhat more generic name for technical/process data) or only INT/VARCHAR build.reservation_token (I'm leaning to this one, we can swap it later for something more generic if such situation appears).

I think a separate varchar field makes the most sense.

If this value will be used to prove the CG has the reservation, then the api should not report it through the normal calls (e.g. getBuild). This makes me wonder if it shouldn't be in a separate table.

We probably also want to record which cg did the reserving. That info would be helpful to display normally.

I've added tokens via separated table.

• Where would you think, CG should be displayed?
• I'm not happy with putting token to CG metadata['build'] from same reason as abusing build.extra. Do you think, that it is ok, or should I move it to separate key (metadata['reservation']) or move completely out of metadata and just change CGImport(metadata, directory, token=None) signature?

CREATE TABLE build_reservations (
       build_id INTEGER NOT NULL REFERENCES build(id),
       user_id INTEGER NOT NULL REFERENCES users(id),
       token VARCHAR(64),
       PRIMARY KEY (build_id)

I wonder if user_id is the right value to include here. Should it be cg_id instead?

It's possible (or at least allowed) for multiple users to act as the same CG. If we were in the future to have kojid use the CG interface to import builds as a "koji" CG, then the user making the reservation could be different from the user performing the final import.

I'm not happy with putting token to CG metadata['build'] from same reason as abusing build.extra.

I agree. Seems like the wrong place

Do you think, that it is ok, or should I move it to separate key (metadata['reservation']) or move completely out of metadata and just change CGImport(metadata, directory, token=None) signature?

To me the metadata seems like the wrong place entirely, given the transient nature of the token. Adding a new arg seems a little wonky too, but I think I might prefer this route. I'm open to argument though. Perhaps I am missing something.

Where would you think, CG should be displayed?

In the buildinfo display, but only when the build is in the BUILDING state. Granted, we should be removing the reservation entry when the build leaves this state.

This makes me ask the question though: should the cg_id be in the build table instead of the reservation table? This is maybe a can of worms since technically the build metadata can span multiple CGs (a fact I've never liked). OR, could we just leave cg_id out and rely purely on the token for the reservation?

'binascii' imported but unused

• I've moved token from metadata to an option.
• display - When build is in BUILDING state, owner is cg, only after import it is rewritten to true owner. That was my idea, that we've stored CG in table to know, which cg imported it and which token it was using. So, that was the reason to not delete it. On the other hand, not sure, if it would be ever used.

If cg_id is in reservation table or build table doesn't make much difference in case of more CGs. Reservation table also allows only one CG.

If we don't want to store token forever, I would move cg_id to build table.

Changes according to discussion yesterday. I did one more thing - extended getBuild to return cg info for builds in BUILDING state to be able to show it in web ui/CLI

Possible additional commit with moving cg_id from build_reservations table to build table.
https://pagure.io/fork/tkopecek/koji/c/2b02cb229e09c3edc18e0ce2c68189a2fd03c005?branch=issue1463a

Possible additional commit

Looks ok overall.

There's a stray debug print statement in left in the hub code

With the cg_id out of the reservation table, all that data is transient. I think we can clear it when when the import finalizes.

https://github.com/mikem23/koji-playground/commits/issue1463a

:thumbsup:

Commit bf394684 fixes this pull-request

Merged with a couple fixes related to epoch

@mikem - It seems, that it is not merged properly - I can't see change in buildinfo.chtml in current master.