Add Client.create_repository() [RHELDST-22483] #218

rbikar · 2024-03-13T14:18:13Z

Client is now capable of creating a repository on pulp
server. The repository is initialized with proper Importer in order
to enable sync/upload of content to new repositories.

Another notable changes:

409 status is not retried in requests as it is used as indicator of
existing repository.
Added create_repository() method to FakeClient as well.
Small changes in the repository.yaml schema as some of the fields
don't have to be necessarily initialized when creating repo but can be
updated later.
Added Importer pulp object mapping class and related subclasses
that encapsulate minimal required fields for this object with valid
defaults.
added "importers": True to search options for repos
fixed converter of notes.signatures on Repository - empty string is now
converted to empty list, not to list with empty string

codecov · 2024-03-13T14:48:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (dfe6dd8) to head (fd915a0).

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         3162      3225   +63     
=========================================
+ Hits          3162      3225   +63

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pubtools/pulplib/_impl/client/client.py

rohanpm · 2024-03-14T05:28:46Z

pubtools/pulplib/_impl/model/repository/base.py

+ """
+ Type id of the importer.
+ """
+ config = pulp_attrib(default=None, type=dict, pulp_field="config")


Could this please use frozendict (converter=frozendict_or_none_converter) so the object is immutable?

rohanpm · 2024-03-14T05:31:46Z

pubtools/pulplib/_impl/model/repository/file.py

@@ -13,6 +13,16 @@
 LOG = logging.getLogger("pubtools.pulplib")


+@attr.s(kw_only=True, frozen=True)
+class IsoImporter(Importer):


Could we please name this FileImporter instead, to match FileRepository?

It's the same reasoning as for FileRepository itself. Yes, technically in Pulp's code and type IDs it refers to it as "iso", but that's a mistake and it's actually a generic file handler. We don't need to repeat the mistake in our own APIs, and by avoiding that we'll be better off if we ever want to port this code to Pulp3.

rohanpm · 2024-03-14T05:33:48Z

tests/repository/test_yum_repository.py

+
+def test_create_repository(client, requests_mocker):
+ repo = YumRepository(id="yum_repo")
+ repo.__dict__["_client"] = client


I don't think this line should be here. It's a hack done from within some tests to forcibly connect a repo object with a client, but the real users of the library aren't supposed to do that, and in this case it's important to test that client.create_repository(repo) actually works without doing this hack first.

I believe if you delete this line the test should still pass and it'd be more accurately covering what the user code should look like.

rohanpm · 2024-03-14T05:34:00Z

tests/repository/test_yum_repository.py

+
+def test_create_repository_already_exists(client, requests_mocker, caplog):
+ repo = YumRepository(id="yum_repo")
+ repo.__dict__["_client"] = client


Same here, we shouldn't need this hack.

rohanpm · 2024-03-14T05:34:08Z

tests/repository/test_yum_repository.py

+
+def test_create_repository_raises_exception(client, requests_mocker):
+ repo = YumRepository(id="yum_repo")
+ repo.__dict__["_client"] = client


And here, we shouldn't need this hack.

rbikar · 2024-03-14T16:23:25Z

I have addressed suggestions but for some reason the coverage checks started to time out.

rbikar · 2024-03-15T10:23:16Z

@rohanpm
I've tried to play with the tests and timeout they hit, I was able to simulate it also on local env. but I'm still still unsure why the tests sometimes hang there and sometimes they do not. It seems to hang on waiter.acquire()
I believe that that code with chaining futures in the create_repository() is OK and I think that there might be some problem in pytest that leads to deadlock. Do you have any idea why this happens?

...
  File "/home/runner/work/pubtools-pulplib/pubtools-pulplib/.tox/cov/lib/python3.8/site-packages/more_executors/_impl/map.py", line 72, in _delegate_resolved
    result = self._map_fn(result)
  File "/home/runner/work/pubtools-pulplib/pubtools-pulplib/pubtools/pulplib/_impl/client/client.py", line 1100, in get_and_check_repo
    repo_on_server.result() == repo
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/concurrent/futures/_base.py", line 439, in result
    self._condition.wait(timeout)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/threading.py", line 302, in wait
    waiter.acquire()

rohanpm · 2024-03-18T01:10:14Z

@rohanpm I've tried to play with the tests and timeout they hit, I was able to simulate it also on local env. but I'm still still unsure why the tests sometimes hang there and sometimes they do not. It seems to hang on waiter.acquire() I believe that that code with chaining futures in the create_repository() is OK and I think that there might be some problem in pytest that leads to deadlock. Do you have any idea why this happens?

I believe there is a bug in the change actually. I was able to reproduce it too, but I couldn't reproduce it after a change like below:

diff --git a/pubtools/pulplib/_impl/client/client.py b/pubtools/pulplib/_impl/client/client.py
index 8a0bc27..5e24a85 100644
--- a/pubtools/pulplib/_impl/client/client.py
+++ b/pubtools/pulplib/_impl/client/client.py
@@ -1093,11 +1093,10 @@ class Client(object):
 
             raise exception
 
-        def get_and_check_repo(_):
-            repo_on_server = self.get_repository(repo_id)
+        def check_repo(repo_on_server):
             try:
                 assert (
-                    repo_on_server.result() == repo
+                    repo_on_server == repo
                 ), "Repo exists on server with unexpected values"
             except AssertionError:
                 if importer:
@@ -1120,7 +1119,7 @@ class Client(object):
                 )
                 raise
 
-            return repo_on_server
+            return f_return(repo_on_server)
 
         LOG.debug("Creating repository %s", repo_id)
         out = self._request_executor.submit(
@@ -1128,6 +1127,9 @@ class Client(object):
         )
 
         out = f_map(out, error_fn=log_existing_repo)
-        out = f_flat_map(out, get_and_check_repo)
+        # After supposedly creating the repo, get it...
+        out = f_flat_map(out, lambda _: self.get_repository(repo_id))
+        # ...and check it matches what's needed
+        out = f_flat_map(out, check_repo)
 
         return f_proxy(out)

...i.e. changing it into a fully non-blocking style rather than a mixture of blocking and non-blocking. Do you want to give that a try?

I think the problem with the other approach is:

when you call repo_on_server.result(), this needs to block on completion of the get_repository call.
get_repository needs to go through the RetryExecutor's thread.
but: that thread might be the one currently in the middle of invoking get_and_check_repo. If so: deadlock!

It can happen randomly, or not, depending on how fast each future manages to complete. Because when you do out = f_flat_map(out, get_and_check_repo), if the future has already completed by that time, then get_and_check_repo will be immediately called in the current thread (no deadlock), but if it hasn't completed yet then get_and_check_repo will later be called from the RetryExecutor's thread (deadlock).

`Client` is now capable of creating a repository on pulp server. The repository is initialized with proper `Importer` in order to enable sync/upload of content to new repositories. Another notable changes: * 409 status is not retried in requests as it is used as indicator of existing repository. * Added create_repository() method to FakeClient as well. * Small changes in the repository.yaml schema as some of the fields don't have to be necessarily initialized when creating repo but can be updated later. * Added `Importer` pulp object mapping class and related subclasses that encapsulate minimal required fields for this object with valid defaults. * added `"importers": True` to search options for repos * fixed converter of `notes.signatures` on `Repository` - empty string is now converted to empty list, not to list with empty string

rbikar force-pushed the create-repo branch 3 times, most recently from 7a977c3 to 7171e16 Compare March 13, 2024 14:45

rbikar force-pushed the create-repo branch 4 times, most recently from 1a27b69 to 4125b70 Compare March 13, 2024 15:34

rbikar marked this pull request as ready for review March 13, 2024 15:34

rbikar requested review from rohanpm and rajulkumar as code owners March 13, 2024 15:34

rohanpm requested changes Mar 14, 2024

View reviewed changes

rbikar force-pushed the create-repo branch from 10982d5 to 87f2367 Compare March 14, 2024 16:07

rbikar requested a review from rohanpm March 14, 2024 16:22

rohanpm previously approved these changes Mar 14, 2024

View reviewed changes

rbikar dismissed rohanpm’s stale review via 31314a1 March 15, 2024 08:45

rbikar force-pushed the create-repo branch from 31314a1 to e2c0efe Compare March 15, 2024 08:52

rbikar force-pushed the create-repo branch from e2c0efe to fd915a0 Compare March 18, 2024 08:41

rohanpm approved these changes Mar 18, 2024

View reviewed changes

rbikar merged commit a38c822 into release-engineering:master Mar 19, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Client.create_repository() [RHELDST-22483] #218

Add Client.create_repository() [RHELDST-22483] #218

rbikar commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

rohanpm Mar 14, 2024

rohanpm Mar 14, 2024

rohanpm Mar 14, 2024

rohanpm Mar 14, 2024

rohanpm Mar 14, 2024

rbikar commented Mar 14, 2024

rbikar commented Mar 15, 2024

rohanpm commented Mar 18, 2024

Add Client.create_repository() [RHELDST-22483] #218

Add Client.create_repository() [RHELDST-22483] #218

Conversation

rbikar commented Mar 13, 2024 • edited Loading

codecov bot commented Mar 13, 2024 • edited Loading

Codecov Report

rohanpm Mar 14, 2024

Choose a reason for hiding this comment

rohanpm Mar 14, 2024

Choose a reason for hiding this comment

rohanpm Mar 14, 2024

Choose a reason for hiding this comment

rohanpm Mar 14, 2024

Choose a reason for hiding this comment

rohanpm Mar 14, 2024

Choose a reason for hiding this comment

rbikar commented Mar 14, 2024

rbikar commented Mar 15, 2024

rohanpm commented Mar 18, 2024

rbikar commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading