Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After upgrade to 7.14.0 http_port keeps changing and api does not work #8007

Open
heldersepu opened this issue Apr 27, 2024 · 27 comments · May be fixed by #8022
Open

After upgrade to 7.14.0 http_port keeps changing and api does not work #8007

heldersepu opened this issue Apr 27, 2024 · 27 comments · May be fixed by #8022

Comments

@heldersepu
Copy link
Contributor

heldersepu commented Apr 27, 2024

Describe the bug
I expected my settings to be maintained but some how the http_port changed

I had:

[api]
http_enabled = True
http_port = 52194

after upgrade

[api]
    http_enabled = True
    http_port = 32883
  • OS: [e.g. Ubuntu 22.04]
  • Tribler's version [e.g. 7.14.0]

I just tested running from source:

image

here is the output of the triblerd.conf after opening an closing a couple of times

heldersepu@bat:~$ cat ~/.Tribler/7.14/triblerd.conf
[api]
    http_enabled = True
    http_port = 42585
    key = *****

heldersepu@bat:~$ cat ~/.Tribler/7.14/triblerd.conf
[api]
    http_enabled = True
    http_port = 46817
    key = *****

@heldersepu
Copy link
Contributor Author

Actually it looks like the port is now changing every time that tribler closes

@heldersepu
Copy link
Contributor Author

Screenshot from 2024-04-27 19-11-10

@synctext
Copy link
Member

Wow, thank you for finding this weird bug!

Btw could you possibly test if the experimental version on our front-page has for you the same weird bug?

@heldersepu
Copy link
Contributor Author

@synctext sorry I do not see any link to the experimental version

@synctext
Copy link
Member

Very helpful!
Btw Website Tribler.org has bleeding edge downloads on front-page.

@heldersepu
Copy link
Contributor Author

heldersepu commented Apr 28, 2024

some more logs that might help:

 2024-04-28 13:41:59,598 - INFO - RESTComponent(174) - RESTComponent: waiting for KnowledgeComponent�[0m
 2024-04-28 13:41:59,598 - INFO - RESTComponent(174) - RESTComponent: waiting for TunnelsComponent�[0m
 2024-04-28 13:41:59,599 - INFO - RESTComponent(174) - RESTComponent: waiting for TorrentCheckerComponent�[0m
 2024-04-28 13:41:59,599 - INFO - RESTComponent(174) - RESTComponent: waiting for DatabaseComponent�[0m
 2024-04-28 13:41:59,601 - INFO - RESTComponent(50) - Adding: "/settings"...�[0m
 2024-04-28 13:41:59,602 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,602 - INFO - RESTComponent(50) - Adding: "/shutdown"...�[0m
 2024-04-28 13:41:59,603 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,603 - INFO - RESTComponent(50) - Adding: "/debug"...�[0m
 2024-04-28 13:41:59,605 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,605 - INFO - RESTComponent(50) - Adding: "/downloads"...�[0m
 2024-04-28 13:41:59,621 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,621 - INFO - RESTComponent(50) - Adding: "/createtorrent"...�[0m
 2024-04-28 13:41:59,622 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,622 - INFO - RESTComponent(50) - Adding: "/statistics"...�[0m
 2024-04-28 13:41:59,623 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,623 - INFO - RESTComponent(50) - Adding: "/libtorrent"...�[0m
 2024-04-28 13:41:59,624 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,625 - INFO - RESTComponent(50) - Adding: "/torrentinfo"...�[0m
 2024-04-28 13:41:59,626 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,627 - INFO - RESTComponent(50) - Adding: "/metadata"...�[0m
 2024-04-28 13:41:59,630 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,630 - INFO - RESTComponent(50) - Adding: "/search"...�[0m
 2024-04-28 13:41:59,631 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,631 - INFO - RESTComponent(50) - Adding: "/knowledge"...�[0m
 2024-04-28 13:41:59,635 - INFO - RESTComponent(58) - OK�[0m
 2024-04-28 13:41:59,725 - INFO - RESTManager(115) - Starting RESTManager...�[0m
 2024-04-28 13:41:59,727 - INFO - RESTManager(126) - Set security scheme and apply to all endpoints�[0m
 2024-04-28 13:41:59,727 - INFO - RESTManager(133) - Remove head�[0m
 2024-04-28 13:42:00,116 - INFO - RESTManager(140) - Http enabled�[0m
 2024-04-28 13:42:00,116 - INFO - RESTManager(154) - Starting HTTP REST API server on port 0...�[0m

I'm looking at the code for the Starting HTTP REST API server on port 0

@heldersepu
Copy link
Contributor Author

heldersepu commented Apr 28, 2024

Just to test I changed a couple of things:

https://github.com/Tribler/tribler/blob/main/src/tribler/core/components/restapi/rest/settings.py#L10-L11
hardcoded that to:

    http_enabled: bool = True
    http_port: int = 43195

it was still showing the Starting HTTP REST API server on port 0

then I hardcoded it here:
https://github.com/Tribler/tribler/blob/main/src/tribler/core/components/restapi/rest/rest_manager.py#L151
after that it seem to api is replying as it should on the hardcoded port


I think this is where the issue was introduced:
74337b2#diff-f4a4e039117c56099dbd14a77f0d9d3f8a65e165f556a9a9dbf8e3256ed8ce9bR147-R150

# The -1 value is assigned for backward compatibility reasons when the port is not specified.
# When RESTManager actually uses the value, it converts -1 to zero.
# It is possible that later we can directly store zero to config.api.http_port, but I prefer to be safe now.
config.api.http_port = api_port or -1

So we are overwriting the config port, why even have that in the config then?

@heldersepu
Copy link
Contributor Author

@kozlovsky maybe you can help with this ...

@kozlovsky
Copy link
Collaborator

kozlovsky commented Apr 29, 2024

Hi @heldersepu!
Can you please tell me your scenario and why you want a dedicated HTTP port?

If you want to access Tribler REST API, you should specify it as a CORE_API_PORT environment variable. It is specified in the "Tribler REST API" documentation:

Alternatively, requests can be made using Swagger UI by starting Tribler
and opening http://localhost:<port>/docs in a browser.
The port can be specified by setting up "CORE_API_PORT" environment variable.

The config option "http_port" is currently not intended for direct use. Historically, some fields of Tribler config, like "http_port," were used to communicate between GUI and Core Tribler processes: The GUI process set the values of these options and expected The Core process to read them. The original logic was that Tribler finds a free port and sticks to it by saving the found port value to settings, so after the relaunch, it uses the same port value. Unfortunately, it does not always work because another application can occupy this port, so when Tribler starts the next time, it cannot use the port value specified in the settings. Usually, for Tribler, it is not necessary to use a specific port value, and we do not want Tribler to stop with an error if the previously used port is unavailable, so, for a long time, the specified http_port value considered just as a recommendation that can be ignored by the Tribler Core process.

@heldersepu
Copy link
Contributor Author

heldersepu commented Apr 29, 2024

@kozlovsky Have you tried the api lately? As is it is unusable...
For me the rest api just does not work on 7.14, i'm assuming that the port is the issue because when I hard code it works fine

The suggestion to use CORE_API_PORT environment variable, I see no reason for that, why can we not read the port from the tribbler config and use that?


for example I expected this to work:

#!/bin/bash
## Script to test the api

IFS=$'\n'

key=$(cat ~/.Tribler/7.14/triblerd.conf | grep "key =")
key="${key/"key = "/""}"

port=$(cat ~/.Tribler/7.14/triblerd.conf | grep "port =")
port="${port/"port = "/""}"

curl -H "X-Api-Key: $key" -s http://localhost:$port/downloads

that was working fine on 7.10, no longer working on 7.14

@heldersepu
Copy link
Contributor Author

heldersepu commented Apr 29, 2024

... and for the record i'm all for Tribler not using a specific port value, yes if another application can occupy this port, we are in troubles, same applies to the CORE_API_PORT environment variable.

But whatever port is used it must be "published" in the config and as far as I can see that is not happening, all I see in the logs is:
Starting HTTP REST API server on port 0

@heldersepu heldersepu changed the title After upgrade to 7.14.0 http_port changed After upgrade to 7.14.0 http_port keeps changing and api does not work May 3, 2024
@heldersepu
Copy link
Contributor Author

Let me add some more context:

The reason I started using the Rest API is because queueing is not an option in Tribler, so the only way I found to enforce a max simultaneous download is with a small script to check how many in progress and stop & start items to keep a max...

Maybe I should work on incorporating that queue in the core instead of the workaround via api

@kozlovsky
Copy link
Collaborator

Hi @heldersepu, I apologize for the delayed response.
The topic is a bit complicated and includes several independent sub-issues.

I hope to answer in a few hours.

@heldersepu
Copy link
Contributor Author

@kozlovsky absolutely no rush

@egbertbouman
Copy link
Member

egbertbouman commented May 8, 2024

Seems like Tribler writes a newly acquired port number to disk only on shutdown (when it's no longer relevant). A reasonable fix seems to be writing to disk just after the API server has started. That's just a matter of calling config.write().

As far as queueing is concerned, I completely agree that we need it. Right now if you have 100 downloads running, it will actually start all 100 swarms. It's much better to let libtorrent deal with starting/stopping swarms.

In the past we avoided auto-management, because of the GUI work required. Our anonymous downloading stack also complicates matters. In the future, once we have a webui (#7736), queueing will become a priority.

@heldersepu
Copy link
Contributor Author

once we have a webui (#7736), queueing will become a priority.

... but we already have a webui (swagger-ui) 🤕
I guess that should come with "for advanced users only" disclaimer

@heldersepu
Copy link
Contributor Author

Seems like Tribler writes a newly acquired port number to disk only on shutdown (when it's no longer relevant). A reasonable fix seems to be writing to disk just after the API server has started. That's just a matter of calling config.write().

I just confirmed this...
added a self.logger.info(settings['api']['http_port'])
and the port in the logs is different than what we have in the triblerd.conf

now to hunt for where to add that config.write()

@heldersepu
Copy link
Contributor Author

heldersepu commented May 8, 2024

I think I smashed this bug take a look at that PR...
In the class RESTManager we had:

if self.config.http_port != api_port:
    self.config.http_port = api_port

and no writing the config after that, in part because it was not possible; That config was not a real config(TriblerConfig) but of type APISettings, just bad naming convention used in that variable that will confuse anyone, at least it confused me for a while; Now we have the "proper" config and writing after the port is changed

@egbertbouman
Copy link
Member

I don't really think that's a bug, it's just how we pass parts of the main config to the components. Considering you can't write those to disk, I would just call config.write() after RESTManager.start.

@heldersepu
Copy link
Contributor Author

heldersepu commented May 8, 2024

we disagree there ...

I firmly believe this is a bug! 🐛
Knowing that there is a TriblerConfig, we just called a parameter of type APISettings simply config, 🤯 ... I mean seriously!?! one could argue we should minify all the code, if we do not care about naming variables something close to what they are, that will save a lot of characters on the code. Code golf anyone? ⛳

Now joking aside, there is no need for two params:
RESTManager(config=config.api, root_endpoint=self.root_endpoint, state_dir=config.state_dir)
when the same can be accomplished with one:
RESTManager(config=config, root_endpoint=self.root_endpoint)
and that way we pass a real config to the rest manager

Also inside the class RESTManager we only invoke the write only when is necessary.

@kozlovsky
Copy link
Collaborator

Hi @heldersepu!

I understand your need to obtain the actual port value. However, I'm afraid I have to disagree that the Tribler core process should save the current port value to the triblerd.conf file for the following reasons:

  1. An application configuration file should contain non-default option values that the application must use. If the config file contains the port value, then the application should use this port value only and report an error if another process occupies the port.

  2. An application should not dynamically update its config file by writing some volatile values. If the application randomly chooses a free port and then dynamically updates its config file with the new port value, it looks inside out. Ideally, an application should not update its configuration file at all, except in two cases:

    • An initial creation of the configuration file;
    • Some options were updated by a user in the UI and need to be saved.
  3. If another process or script wants to obtain the application's current parameters, it should not take them from the configuration file:

    • Options in the configuration file can be missed if they have default values, so reading a config file is not a reliable way to obtain any option currently used by an application:
    • Config files are not designed as a tool for inter-process communication.
    • If an application updates its config file after launch and another process reads it, race conditions are possible because the file is updated not immediately after the application starts but with some delay.

Currently, the Tribler Core process indeed writes a port value to the config file. But this behavior looks like a bug we should fix, not the one we should use for inter-process communication.

Specifically, the following looks like an incorrect behavior:

  1. Tribler Core ignores the http_port and key values specified in the config file;
  2. Tribler Core updates the http_port and key values in the config file with new values that are ignored on the next run.

As I understand your current opinion, you say Tribler Core updates the config file too late (on process exit), and should do it earlier. I believe it shouldn't update the config file at all.


So, I think we can do the following to fix Tribler's current incorrect behavior.

  1. In the next Tribler version (7.15 or 8.0), the upgrade process should remove the http_port option value from the config file because currently (in Tribler 7.11 and above), it contains not a value set by a user but a randomly selected port value.
  2. If a user again assigns value to the http_port config option after that, Tribler should use this port and not randomly select a free port.
  3. We should remove all config file updates from the Tribker Core code except its initial creation and SettingsEndpoint.

But now, what should we do with your original problem? How should we make the current port/key values available to other applications?

The simplest thing we can do is write them into a file, but it should not be a config file. We can create a second file, triblerd.info, that contains all actual information about the current Tribler process. Then, we can add a new helper method, config.write_info(), and use it instead of config.write() whenever we want to update the information about the current process.

This approach has the following benefits:

  1. We stop writing volatile values to the config file and stop using the config file for inter-process communication.
  2. We can write all current option values (including the default ones) to the new file triblerd.info, so it should be easier to use.

Before we implement it and release a new Tribler version with these changes, it is totally possible to use environment variables CORE_API_PORT and CORE_API_KEY to specify exact port/key values and use them for REST API. It should work for all recent Tribler versions, including Tribler 7.10 and 7.14. I understand that you prefer to read values from a file instead of setting values through environment variables; I'm just pointing out that this option works quite well right now.


Regarding your PR #8022, I think we can use it as a base for these refactoring; I just believe we should write to a different file like triblerd.info instead of triblerd.conf.

@heldersepu
Copy link
Contributor Author

heldersepu commented May 10, 2024

@kozlovsky I feel you are over engineering a solution here ...

Tribler already had a way to "set" a port via the conf file, but one way is not enough so CORE_API_PORT environment variable to the rescue... following that same pattern ... one file is not enough let's add a second, yeee.

At this point I honestly could care less how you do it or how long it takes to release it, I was ready to roll back to v7.10 ... but I spent the time and got my own fork working & will run from there for the foreseeable future


One more thing you might want to add to this bug, if I use CORE_API_PORT then close tribler and open again I get an error:
RuntimeError: Tribler configuration conflicts with the current OS state: REST API port 52196 already in use
I never got that with v7.10

@kozlovsky
Copy link
Collaborator

TLDR: Tribler should not write random temporary values to its own configuration file.


If I use CORE_API_PORT then close tribler and open again I get an error:
RuntimeError: Tribler configuration conflicts with the current OS state: REST API port 52196 already in use

For me, it works correctly:

[tribler-core PID:29468] 2024-05-10 22:51:06,503 - INFO - RESTManager(156) - Starting HTTP REST API server on port 33333...
...
[tribler-core PID:29468] 2024-05-10 22:51:06,654 - INFO - RESTManager(168) - HTTP REST API server started on port 33333
[tribler-core PID:29468] 2024-05-10 22:51:06,654 - INFO - RESTManager(149) - Swagger docs: http://127.0.0.1:33333/docs
[tribler-core PID:29468] 2024-05-10 22:51:06,655 - INFO - RESTManager(150) - Swagger JSON: http://127.0.0.1:33333/docs/swagger.json

By the way, your previous remark also does not correspond with what I see:

But whatever port is used it must be "published" in the config and as far as I can see that is not happening, all I see in the logs is: Starting HTTP REST API server on port 0

This is what I see in logs when CORE_API_PORT is not specified:

[tribler-core PID:9604] 2024-05-10 22:57:47,570 - INFO - RESTManager(156) - Starting HTTP REST API server on port 0...
...
[tribler-core PID:9604] 2024-05-10 22:57:47,705 - INFO - RESTManager(168) - HTTP REST API server started on port 65452
[tribler-core PID:9604] 2024-05-10 22:57:47,705 - INFO - RESTManager(149) - Swagger docs: http://127.0.0.1:65452/docs
[tribler-core PID:9604] 2024-05-10 22:57:47,706 - INFO - RESTManager(150) - Swagger JSON: http://127.0.0.1:65452/docs/swagger.json

@heldersepu
Copy link
Contributor Author

heldersepu commented May 10, 2024

TLDR: Tribler should not write random temporary values to its own configuration file.

But right now that is exactly what v7.14 is doing and at the wrong time

@heldersepu
Copy link
Contributor Author

For me, it works correctly

the Tribler configuration conflicts with the current OS state is happening all the time for me ... I'm on Ubuntu and i do
export CORE_API_PORT=52196; ~/Documents/GitHub/tribler/src/tribler.sh
If I wait a few minutes from the time I close to reopen it does work seems the port stays open for some reason

@kozlovsky
Copy link
Collaborator

Thanks for the information; that's interesting...

@heldersepu
Copy link
Contributor Author

heldersepu commented May 10, 2024

By the way, your previous remark also does not correspond with what I see:

You are showing exactly the same line I did:
Starting HTTP REST API server on port 0
In my troubleshooting I stopped there, never saw others
I guess it was not really starting anything at that port just leftover junk messages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants