Warning produced during sync of package-extract result

[fridex]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior

No warning produced. Is it something that has implications on our side?

[pacospace]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior

No warning produced. Is it something that has implications on our side?

I had a look and it seems like the issue is because the software_environment_id in HasSymbol table is a primary key and in general we can link has_symbol table to SoftwareEnvironment and ExternalSoftwareEnvironment, we should make software_environment_id nullable=True and not primary key.

[pacospace]

When do we differentiate between package extract is_external or not? if the image analyzed comes from a user or from us?

[pacospace]

cc @fridex

[fridex]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior No warning produced. Is it something that has implications on our side?

I had a look and it seems like the issue is because the software_environment_id in HasSymbol table is a primary key and in general we can link has_symbol table to SoftwareEnvironment and ExternalSoftwareEnvironment, we should make software_environment_id nullable=True and not primary key.

Sounds good. :+1:

When do we differentiate between package extract is_external or not? if the image analyzed comes from a user or from us?

I think it was originally designed this way (IIRC). As of now, I don't see why we should differentiate this fact. We run python applications similarly to others, so we might eventually discard this flag over time.

[pacospace]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior No warning produced. Is it something that has implications on our side?

I had a look and it seems like the issue is because the software_environment_id in HasSymbol table is a primary key and in general we can link has_symbol table to SoftwareEnvironment and ExternalSoftwareEnvironment, we should make software_environment_id nullable=True and not primary key.

Sounds good.

When do we differentiate between package extract is_external or not? if the image analyzed comes from a user or from us?

I think it was originally designed this way (IIRC). As of now, I don't see why we should differentiate this fact. We run python applications similarly to others, so we might eventually discard this flag over time.

I saw that there is only one query using HasSymbol table but linked to SoftwareEnvironment only: https://github.com/thoth-station/storages/blob/cae7a9ec77569bc5e714397eab73e7f1dca78461/thoth/storages/graph/postgres.py#L5427

which component uses this query?

Because in this case is_external set might be important and we should generalize that query also. WDYT?

Moreover, I noticed that in management API we use is_external=False by default https://github.com/thoth-station/management-api/blob/8cb8ed9169d9ccdb293cf0520b9500b0b4aa71da/thoth/management_api/api_v1.py#L276

, while in investigator we use the method in common https://github.com/thoth-station/investigator/blob/0be77c664535c074752579541551cf246e3bfa04/thoth/investigator/package_extract_trigger/investigate_package_extract_trigger.py#L45

and by default we set is_external=True: https://github.com/thoth-station/common/blob/8fb9467dc47c72ab7a4f0c7c3d8a4289dd4fff05/thoth/common/openshift.py#L990

Depending on which components use the query https://github.com/thoth-station/storages/blob/cae7a9ec77569bc5e714397eab73e7f1dca78461/thoth/storages/graph/postgres.py#L5427 and from where we are scheduling package extract we should make sure we set that flag correctly.

[fridex]

Now I recall parameter semantics. Its purpose was to have marked container image analyses that are coming from external users and should not be used during recommendations specific to the runtime environment. e.g. s2i-ubi-8-py38 is not an external container image, but our adviser image is an external container image build.

[pacospace]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior No warning produced. Is it something that has implications on our side?

I had a look and it seems like the issue is because the software_environment_id in HasSymbol table is a primary key and in general we can link has_symbol table to SoftwareEnvironment and ExternalSoftwareEnvironment, we should make software_environment_id nullable=True and not primary key.

Sounds good.

When do we differentiate between package extract is_external or not? if the image analyzed comes from a user or from us?

I think it was originally designed this way (IIRC). As of now, I don't see why we should differentiate this fact. We run python applications similarly to others, so we might eventually discard this flag over time.

I saw that there is only one query using HasSymbol table but linked to SoftwareEnvironment only:

https://github.com/thoth-station/storages/blob/cae7a9ec77569bc5e714397eab73e7f1dca78461/thoth/storages/graph/postgres.py#L5427

which component uses this query?

Found in adviser: https://github.com/thoth-station/adviser/blob/fc708aeda70db7a6132f67d13c513d0041a65410/thoth/adviser/sieves/abi_compat.py#L57

Because in this case is_external set might be important and we should generalize that query also. WDYT?

Moreover, I noticed that in management API we use is_external=False by default https://github.com/thoth-station/management-api/blob/8cb8ed9169d9ccdb293cf0520b9500b0b4aa71da/thoth/management_api/api_v1.py#L276

, while in investigator we use the method in common https://github.com/thoth-station/investigator/blob/0be77c664535c074752579541551cf246e3bfa04/thoth/investigator/package_extract_trigger/investigate_package_extract_trigger.py#L45

and by default we set is_external=True: https://github.com/thoth-station/common/blob/8fb9467dc47c72ab7a4f0c7c3d8a4289dd4fff05/thoth/common/openshift.py#L990

Depending on which components use the query

https://github.com/thoth-station/storages/blob/cae7a9ec77569bc5e714397eab73e7f1dca78461/thoth/storages/graph/postgres.py#L5427

and from where we are scheduling package extract we should make sure we set that flag correctly.

[pacospace]

Now I recall parameter semantics. Its purpose was to have marked container image analyses that are coming from external users and should not be used during recommendations specific to the runtime environment. e.g. s2i-ubi-8-py38 is not an external container image, but our adviser image is an external container image build.

who submit the package extract for internal image and for external image analysis? Is it automated?

[fridex]

Now I recall parameter semantics. Its purpose was to have marked container image analyses that are coming from external users and should not be used during recommendations specific to the runtime environment. e.g. s2i-ubi-8-py38 is not an external container image, but our adviser image is an external container image build.

who submit the package extract for internal image and for external image analysis? Is it automated?

The internal one is submitted via management-api (by admins) as this action controls which container images are taken into account during recommendations targeting container images (runtime env)

The external one can be submitted from user-api, by any user of Thoth.

[pacospace]

Now I recall parameter semantics. Its purpose was to have marked container image analyses that are coming from external users and should not be used during recommendations specific to the runtime environment. e.g. s2i-ubi-8-py38 is not an external container image, but our adviser image is an external container image build.

who submit the package extract for internal image and for external image analysis? Is it automated?

The internal one is submitted via management-api (by admins) as this action controls which container images are taken into account during recommendations targeting container images (runtime env)

The external one can be submitted from user-api, by any user of Thoth.

Ok thanks, then we are all good! Thanks for the explanation! I ll open PR to remove the constraint on the primary key :)

[pacospace]

Describe the bug

2021-01-12T20:49:09.569560916Z {"name": "thoth.storages.graph.models_base", "levelname": "WARNING", "module": "models_base", "lineno": 56, "funcname": "get_or_create", "created
": 1610484549.568839, "asctime": "2021-01-12 20:49:09,568", "msecs": 568.8390731811523, "relative_created": 611646.9593048096, "process": 1, "message": "Integrity error on crea
ting a new record; this can be due to concurrent writes to database, recovering (attributes: {'sha256': '1ea5d03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}): (p
sycopg2.errors.UniqueViolation) duplicate key value violates unique constraint \"python_file_digest_sha256_key\"\nDETAIL:  Key (sha256)=(1ea5d03b8249852a9d33b778431a11d6e4e17b7
cdc04a0030df7eb81509a60fd) already exists.\n\n[SQL: INSERT INTO python_file_digest (sha256) VALUES (%(sha256)s) RETURNING python_file_digest.id]\n[parameters: {'sha256': '1ea5d
03b8249852a9d33b778431a11d6e4e17b7cdc04a0030df7eb81509a60fd'}]\n(Background on this error at: http://sqlalche.me/e/13/gkpj)"}
2021-01-12T20:54:39.414572803Z /opt/app-root/lib64/python3.8/site-packages/sqlalchemy/sql/crud.py:801: SAWarning: Column 'has_symbol.software_environment_id' is marked as a mem
ber of the primary key for table 'has_symbol', but has no Python-side or server-side default generator indicated, nor does it indicate 'autoincrement=True' or 'nullable=True',
and no explicit value is passed.  Primary key columns typically may not store NULL. Note that as of SQLAlchemy 1.1, 'autoincrement=True' must be indicated explicitly for compos
ite (e.g. multicolumn) primary keys if AUTO_INCREMENT/SERIAL/IDENTITY behavior is expected for one of the columns in the primary key. CREATE TABLE statements are impacted by th
is change as well on most backends.

To Reproduce Steps to reproduce the behavior:

1. Try to analyze quay.io/thoth-station/adviser:v0.21.1
2. After succesful package-extract, check graph-sync logs
3. See the warning

Expected behavior No warning produced. Is it something that has implications on our side?

I had a look and it seems like the issue is because the software_environment_id in HasSymbol table is a primary key and in general we can link has_symbol table to SoftwareEnvironment and ExternalSoftwareEnvironment, we should make software_environment_id nullable=True and not primary key.

Sounds good.

It appears that it is already nullable actually:

We removed the constraint with: https://github.com/thoth-station/storages/blob/master/thoth/storages/data/alembic/versions/54d50b0eeb33_allow_nullable_references_for_software_.py

So trying to generate a new alembic migration removing the primary key from the has_symbol table for software_environment_id produces empty migration.

When do we differentiate between package extract is_external or not? if the image analyzed comes from a user or from us?

I think it was originally designed this way (IIRC). As of now, I don't see why we should differentiate this fact. We run python applications similarly to others, so we might eventually discard this flag over time.

[fridex]

Thanks for digging into this. This was observed in test environment ocp4, so maybe we need a database migration update.

[pacospace]

Thanks for digging into this.

removing primary_key=True simply from HasSymbol class seems to remove that warning, which in any case is not present in the database because of that previous migration that invalidated the primary key which by default cannot be nullable.

This was observed in test environment ocp4, so maybe we need a database migration update.

test has same alembic version as stage: e7ebfc165a96

[pacospace]

Thanks for digging into this.

removing primary_key=True simply from HasSymbol class seems to remove that warning, which in any case is not present in the database because of that previous migration that invalidated the primary key which by default cannot be nullable.

This was observed in test environment ocp4, so maybe we need a database migration update.

test has same alembic version as stage: e7ebfc165a96

But there is issue in test, maybe due to copy of data from stage?

There are equal records copied three times indeed in the tables. the database schema in test need to be recreated I think.

[fridex]

Oh, I think this is bad. Alembic expects just one entry. Can we have an alert on this if this is true?

[pacospace]

Oh, I think this is bad. Alembic expects just one entry. Can we have an alert on this if this is true?

This should not happen, syncing to the database with same ids should not be possible when the id is set as unique.

That also explains the issue mentioned in this issue description probably if the syncing was performed in test environment.

I think this happened also in the past with test environment when we migrated data from stage to test, maybe the procedure doing that need to be modified, especially if we need to migrate between other environments. cc @harshad16

[harshad16]

Yes, it is really bad. This happens when the database is not dropped before restoring the database with migrated db, which will get to make the migration process smoother, so such an issue doesn't occur.

[sesheta]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[fridex]

/remove-lifecycle stale

[pacospace]

do we have to do something more here?

[fridex]

Looks like https://github.com/thoth-station/metrics-exporter/issues/573 is in place. If not, feel free to reopen.

/close

[sesheta]

@fridex: Closing this issue.

In response to [this](https://github.com/thoth-station/storages/issues/2169#issuecomment-830054465): >Looks like https://github.com/thoth-station/metrics-exporter/issues/573 is in place. If not, feel free to reopen. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.