Pulp

There are several models that are created using bulk_create() where duplicates should not result in failure to create the batch. The use cases for these models is as follows:

1. The batch contains duplicates.
2. Race condition whereby separate workers are attempting to create the same thing.

In both cases, the resulting IntegretyError is logged and/or reported to the user and the entire operation fails. The desired result is for the bulk_create to ignore the failure and create the batch.

Due to the race condition in (2), the proposed solution is to fall back to using Model.save() in a nested transaction committing the entire batch in a single overall transaction. When model.save() raises an IntegrityError, it would be fetched using the natural key and yielded instead. The bulk_create() method can be overridden in a Manager subclass for provide this behavior. This manager may be assigned to affected models.

Basic algorithm (transactions omitted for clarity but they will be critical).

1. return super().bulk_create().
2. catch IntegrityError
3. foreach model in the batch: save() and yield.
4. catch IntegrityError
5. get() the model from the DB and yield instead.

For a general solution, it would help if the pulp models would include a property or method that returned a models.Q object used to query the DB (step:5). Perhaps: getq() or just a property named q(). This would also simplify the query stages.

Identified Models:

• Content
• Artifact
• ContentArtifact
• RemoteArtifact

There is code in the pipeline that relies on bulk_create() yielding the same model instances as passed in. This needs to be accounted for. Also, I believe there is work related to flattening the Content tables that has the same expectation.

The pattern above preserves the queue order, so that won't be a problem for the docker use case.

Just to outline the problem, I have a use case (docker sync) that needs the queue order to stay the same. The reason for this is that Docker has many-to-many relationships between content units. Currently, the implementation is to put this relationship in DeclarativeContent.extra_data, and the many-to-many is saved after the stage that saves the units. However, once Docker can use the new implementation of this issue, the many-to-many's can be handled in the QueryAndSave "post_save" hook. Even in this case, it is necessary to preserve the order.

In case that's not clear, here's an example.
2 Content units, A and B, that are related.
In a previous stage, A is created (but not saved), and creates B as a side effect. A.extra_data stores that it will eventually have a relationship with B. The output order for this stage is (B, A).
In the relationship stage (or this stage), B must come through first to guarantee that B has been saved before we try to create a relationship (which will happen immediately after saving A).

Note: The only core stage that is not "Q order safe" is the ArtifactDownload stage. (However in Docker, this is still safe because the "fast lane" preserves order of units in the fast lane, and that's enough in our case.)

@jortel this plan sounds pretty good. The only possible issue could be if there is any extra data attached to the in-memory model being saved which would be lost when the model is queried and replaced. Our usage of data attached to models as assigned attributes is not a good practice, but I remember we were doing it in a few places in the stages API to handle bulk_create with foreign keys between the models. Do we do this currently and is this done on the single content branch dalley is working on?

Also +1 to making a property that returns a Q object for that instance. Can we call it q_obj so it would be like: Artifact.q_obj Also can we add it into Artifact, Content, RemoteArtifact, and ContentArtifact using a mixin class that defines q_obj or $otheridea?

bmbouter wrote:

@jortel this plan sounds pretty good.

The only possible issue could be if there is any extra data attached to the in-memory model being saved which would be lost when the model is queried and replaced. Our usage of data attached to models as assigned attributes is not a good practice, but I remember we were doing it in a few places in the stages API to handle bulk_create with foreign keys between the models. Do we do this currently and is this done on the single content branch dalley is working on?

Yes, I noted this in my comment as well.

Also +1 to making a property that returns a Q object for that instance. Can we call it q_obj so it would be like: Artifact.q_obj

A model.Q is an object. What is the purpose for the _obj suffix?

Also can we add it into Artifact, Content, RemoteArtifact, and ContentArtifact using a mixin class that defines q_obj or $otheridea?

jortel@redhat.com wrote:

bmbouter wrote:

@jortel this plan sounds pretty good.

The only possible issue could be if there is any extra data attached to the in-memory model being saved which would be lost when the model is queried and replaced. Our usage of data attached to models as assigned attributes is not a good practice, but I remember we were doing it in a few places in the stages API to handle bulk_create with foreign keys between the models. Do we do this currently and is this done on the single content branch dalley is working on?

Yes, I noted this in my comment as well.

Oh right I do see that mentioned there. So what's the plan for those code areas? If we could identify them we could see if another implementation of that is feasible. In fact that could be done first to undo the reliance on that need as it's own thing instead of at the same time as this piece of work.

Also +1 to making a property that returns a Q object for that instance. Can we call it q_obj so it would be like: Artifact.q_obj

A model.Q is an object. What is the purpose for the _obj suffix?

Django's documentation writes the Model.Q as "the Q object" https://docs.djangoproject.com/en/2.1/topics/db/queries/#complex-lookups-with-q-objects so this piggybacks off that name. My concern with just q or Q is that since the stages also use queues in lots of places I was hoping to avoid something that could be interpreted as related to a Queue object from asyncio.

Also can we add it into Artifact, Content, RemoteArtifact, and ContentArtifact using a mixin class that defines q_obj or $otheridea?

bmbouter wrote:

The only possible issue could be if there is any extra data attached to the in-memory model being saved which would be lost when the model is queried and replaced.

I'm don't think this is a problem.

In rough psuedocode:

dc = DeclarativeContent()
If dc.content is not unique:
    dc.content = SomeContent.objects.get(pk=dc.content.pk)

Therefore, the dc data is preserved. If 2 dupes go through a single pipeline, each is contained by it's own dc.

amacdona@redhat.com wrote:

bmbouter wrote:

The only possible issue could be if there is any extra data attached to the in-memory model being saved which would be lost when the model is queried and replaced.

I'm don't think this is a problem.

I'm not explaining myself well. I don't mean the extra_data attribute on the DeclarativeContent unit, I mean an extra attribute assigned to the model itself. Something like:

content = dc.content
content.new_attribute = 'i need this later'
for content in MyContent.bulk_create(new_attribute)
    print(content.new_attribute)  # Does this raise an AttributeError?

In rough psuedocode:

[...]

Therefore, the dc data is preserved. If 2 dupes go through a single pipeline, each is contained by it's own dc.

During IRC convo we confirmed that currently in core, and in @dalley's upcoming single-content branch PR there is no reliance on this 'attribute attachment' implementation so I think core is good w/ the change.

I did have an idea to share, which is that instead of replacing the in-memory object we could take the db attributes we just fetched and assign them to the in-memory one and continue to use that.

I'll point out that instance attachment approach works with bulk_create so if we're replacing bulk_create on some of our models with our implementation, it would be good if we could not deviate from django's behavior.

What's the benefit to overriding bulk_create instead of just defining a new method?

I reproduced this with pulp_file. Here were the steps:

mkdir test && cd test
echo "test" > test1
echo "test" > test2
for file in `ls test*`; do echo $file,`sha256sum $file | awk '{ print $1 }'`,`stat -L -c '%s' $file`; done > PULP_MANIFEST
http :8000/pulp/api/v3/repositories/ name=test
http :8000/pulp/api/v3/remotes/file/ name=test url="file://`pwd`/PULP_MANIFEST"
http :8000/pulp/api/v3/remotes/file/1/sync/ repository=/pulp/api/v3/repositories/1/

daviddavis wrote:

What's the benefit to overriding bulk_create instead of just defining a new method?

Since there is always a race condition when multiple workers are syncing content, it seems like the plugin writer would always want to prevent an IntegrityError from failing the sync. Providing only one method for bulk creating would prevent the plugin writer from using the wrong method that doesn't account for the IntegrityError.

https://github.com/pulp/pulp/pull/3744