Treat 403s from Flickr as dead links #1201

sarayourfriend · 2023-04-14T05:20:23Z

Fixes

Requested Olga specifically as she's looked into this before. Requested Staci the most knowledgeable of us all about Flickr as a provider.

Description

Adds a way of transparently handling status check differences per-provider. The default behaviour remains the same, but this allows us to assign different sets of "unknown" and "live" statuses per-provider.

I had to remove the memory limit from ES for myself as it was not possible to use an environment variable to disable it. I changed the approach to use compose file overrides instead and conditionally include the overrides file. The default is to include it, but setting OPENVERSE_ENABLE_DC_OVERRIDES in your environment to anything other than "true" will disable it.

Testing Instructions

Can't really test it locally as you'd have to force some Flickr links to 403. Check the new unit tests and make sure they cover things comprehensively. Make some regular requests to your local API for good measure.

To test the changes to the docker-compose, run just dc version and confirm that the echoed command just is running includes -f docker-compose.overrides.yml. Now set OPENVERSE_ENABLE_DC_OVERRIDES="false" into your environment and run just dc version again and confirm that the overrides docker compose file is not included in the command.

Checklist

My pull request has a descriptive title (not a vague title like
Update index.md).
My pull request targets the default branch of the repository (main) or
a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible
errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

api/catalog/api/utils/check_dead_links/provider_status_mappings.py

krysal · 2023-04-14T14:17:12Z

docker-compose.overrides.yml

+services:
+  es:
+    # Memory limit for ES, as it tends to be a memory hoarder
+    mem_limit: 4294967296


This option is removed in newer docker compose file versions.¹

Specifying the value in high-level units is also possible if you still want to keep it.

Suggested change

mem_limit: 4294967296

mem_limit: 4gb

Footnotes

Compose file reference > Legacy versions > About versions and upgrading ↩

I'd be happy to remove it, but I assume some people rely on this to prevent ES from sucking up their RAM? We're still on compose file version 2.4 for now:

openverse/docker-compose.yml

Line 1 in 2ecb187

version: "2.4"

When we update the compose format, maybe then we can remove it? I don't want to cause a disruption for others with this PR in the mean time.

@krysal there was an open issue with higher units not working, so we had to resort to specifying the value as a plain number.

Personally I have no intention of bumping the Docker Compose config up to v3, none of the features we need are absent in v2 and none of the features added to v3 are useful to us.

@sarayourfriend I'd like to get @AetherUnbound's input on this as another Linux user but for macOS folks using Docker Desktop, the memory limit for the system as a whole can be configured from the settings UI.

Can we make this "opt-in" rather than "opt-out"? Impossible to run ES locally for me without removing this

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

dhruvkb · 2023-04-18T04:57:05Z

docker-compose.overrides.yml

+services:
+  es:
+    # Memory limit for ES, as it tends to be a memory hoarder
+    mem_limit: 4294967296


@krysal there was an open issue with higher units not working, so we had to resort to specifying the value as a plain number.

Personally I have no intention of bumping the Docker Compose config up to v3, none of the features we need are absent in v2 and none of the features added to v3 are useful to us.

@sarayourfriend I'd like to get @AetherUnbound's input on this as another Linux user but for macOS folks using Docker Desktop, the memory limit for the system as a whole can be configured from the settings UI.

dhruvkb · 2023-04-18T04:59:41Z

justfile

@@ -8,6 +8,8 @@ IS_PROD := env_var_or_default("PROD", env_var_or_default("IS_PROD", ""))
 # `PROD_ENV` can be "ingestion_server" or "catalog"
 PROD_ENV := env_var_or_default("PROD_ENV", "")
 IS_CI := env_var_or_default("CI", "")
+DC_USER := env_var_or_default("DC_USER", "opener")
+ENABLE_DC_OVERRIDES := env_var_or_default("OPENVERSE_ENABLE_DC_OVERRIDES", "true")


I'm not in favour of splitting the Docker Compose config into a regular and an override file.

If we can remove the memory limit entirely, then I'd also prefer not to have an overrides file, but I can't get any mem_limit value to work on my local, even set as high as 32 GB. Docker always kills the container.

FWIW, can you share why you dislike an overrides file?

It's just means you have to read in two places to read to mentally piece together a full config. I'm not vetoing it, just expressing displeasure that we had to use this approach.

@dhruvkb

@krysal there was an open issue with higher units not working, so we had to resort to specifying the value as a plain number.

Ah, that is sad to hear. It's really frustrating when these differences of OS happens, but related to that and the next quote:

Personally I have no intention of bumping the Docker Compose config up to v3, none of the features we need are absent in v2 and none of the features added to v3 are useful to us.

I assume that newer versions have better compatibility between different OS, specially for working with docker compose v2 which we are promoting to go for, so I don't think we should stuck in v2, but that is a tasks por another issue for sure.

dhruvkb · 2023-04-18T04:59:48Z

docker-compose.yml

-    # Memory limit for ES, as it tends to be a memory hoarder
-    # Set this value to an empty string to remove the limit
-    # https://docs.docker.com/compose/compose-file/compose-file-v2/#cpu-and-other-resources
-    mem_limit: ${ES_MEM_LIMIT:-4294967296} # 4 GiB in bytes


If this problem only affects your setup (which is likely because it's not been brought up before) the better option would be for you to set the ES_MEM_LIMIT on your machine to a very high number.

dhruvkb

Approved Docker Compose changes because nothing else works. Tried ES_MEM_LIMIT set to both a higher value and a blank string.

stacimc

The dead link handling changes LGTM. The discussion around the docker-compose changes makes sense to me.

sarayourfriend requested review from obulat and stacimc April 14, 2023 05:20

sarayourfriend requested a review from a team as a code owner April 14, 2023 05:20

openverse-bot added this to Needs review in Openverse PRs Apr 14, 2023

sarayourfriend changed the title ~~Fix/flickr 403 dead link~~ Treat 403s from Flickr as dead links Apr 14, 2023

github-actions bot added the 🧱 stack: api Related to the Django API label Apr 14, 2023

krysal reviewed Apr 14, 2023

View reviewed changes

sarayourfriend added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🕹 aspect: interface Concerns end-users' experience with the software labels Apr 16, 2023

sarayourfriend changed the base branch from main_old to main April 16, 2023 23:55

sarayourfriend requested a review from a team as a code owner April 16, 2023 23:55

sarayourfriend added 3 commits April 17, 2023 09:55

Remove Elasticsearch mem limit

2ecb187

Can we make this "opt-in" rather than "opt-out"? Impossible to run ES locally for me without removing this

Add per-provider status checks in dead link validation

bc898e5

Add default overrides and unambiguous way of disabling them

69cbfdc

sarayourfriend force-pushed the fix/flickr-403-dead-link branch from 19ec605 to 69cbfdc Compare April 16, 2023 23:55

Use more descriptive module docstring

94b2a65

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

sarayourfriend requested a review from krysal April 16, 2023 23:57

dhruvkb reviewed Apr 18, 2023

View reviewed changes

dhruvkb approved these changes Apr 18, 2023

View reviewed changes

stacimc approved these changes Apr 18, 2023

View reviewed changes

Openverse PRs automation moved this from Needs review to Reviewer approved Apr 18, 2023

sarayourfriend merged commit 6cdfb59 into main Apr 18, 2023
38 checks passed

Openverse PRs automation moved this from Reviewer approved to Merged! Apr 18, 2023

sarayourfriend deleted the fix/flickr-403-dead-link branch April 18, 2023 22:19

krysal mentioned this pull request Jun 16, 2023

Treat a 403 response code from Europeana as dead link #2417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat 403s from Flickr as dead links #1201

Treat 403s from Flickr as dead links #1201

sarayourfriend commented Apr 14, 2023 •

edited

krysal Apr 14, 2023

sarayourfriend Apr 16, 2023 •

edited

dhruvkb Apr 18, 2023

dhruvkb Apr 18, 2023

dhruvkb Apr 18, 2023

sarayourfriend Apr 18, 2023

dhruvkb Apr 18, 2023 •

edited

krysal Apr 18, 2023

dhruvkb Apr 18, 2023

dhruvkb left a comment

stacimc left a comment

Treat 403s from Flickr as dead links #1201

Treat 403s from Flickr as dead links #1201

Conversation

sarayourfriend commented Apr 14, 2023 • edited

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

krysal Apr 14, 2023

Choose a reason for hiding this comment

Footnotes

sarayourfriend Apr 16, 2023 • edited

Choose a reason for hiding this comment

dhruvkb Apr 18, 2023

Choose a reason for hiding this comment

dhruvkb Apr 18, 2023

Choose a reason for hiding this comment

dhruvkb Apr 18, 2023

Choose a reason for hiding this comment

sarayourfriend Apr 18, 2023

Choose a reason for hiding this comment

dhruvkb Apr 18, 2023 • edited

Choose a reason for hiding this comment

krysal Apr 18, 2023

Choose a reason for hiding this comment

dhruvkb Apr 18, 2023

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

stacimc left a comment

Choose a reason for hiding this comment

sarayourfriend commented Apr 14, 2023 •

edited

sarayourfriend Apr 16, 2023 •

edited

dhruvkb Apr 18, 2023 •

edited