Skip to content
New issue

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

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

Already on GitHub? # to your account

Replace ComputeExternalAddress by param Server.ExternalWebUrl and rename Server.Port and Server.Address #378

Merged
merged 10 commits into from
Nov 22, 2023

Conversation

haoming29
Copy link
Contributor

@haoming29 haoming29 commented Nov 15, 2023

Closes #372

The ComputeExternalAddress() seems to be less useful if we simply assigned Server.Hostname+Server.Port as the default return value of the function, so I replace the function by directly setting the default value of Server.ExternalAddress, and, of course, maintaining the url type.

Later while I making the changes to the codebase, I figured that those three config names (Server.Port, Server.Address, Server.ExternalAddress) are very broad and can cause confusion when people try to use them. So I decided to rename them to better reflect what they meant:

  • Server.Port -> Server.WebPort: In our codebase, Server.Port is used to setup web engine and serves as the port for various web related services (Prometheus, IssuerURL, etc). So I figured it's better to call it Server.WebPort to be dinstinct with XrootD.Port
  • Server.Address -> Server.WebHost: Server.Address is only used when we setup web engine so it's better call it WebHost
  • Server.ExternalAddress -> Server.ExternalWebUrl: per description, this value is an URL that can represent the server's web service for external users, so it makes sense to name it as what it's intended to do.

This is a potential breaking change, but given the fact that we haven't started distributing our server instances, it shouldn't hurt too much if we put it in the release note.

Testing Instructions

Run your servers and go to origin and the web ui can load as expected. Note your console output for all the servers and there shouldn't be any errors. Now change the Server.ExternalAddress to https://${Server.Hostname}:${Server.Port} where the ${} is the value of the config names, and re-run all servers and they should still work as expected.

@haoming29 haoming29 added bug Something isn't working enhancement New feature or request labels Nov 15, 2023
@haoming29 haoming29 force-pushed the imprv-compute-external-add branch from bec35f4 to 0be7d1b Compare November 15, 2023 21:04
@haoming29 haoming29 requested review from jhiemstrawisc and turetske and removed request for jhiemstrawisc November 15, 2023 21:08
@haoming29 haoming29 added this to the v7.3.0 milestone Nov 16, 2023
@turetske
Copy link
Collaborator

@haoming29 Do we want to clarify Origin.Url as well? Because I think that's actually the web url and not the url that xrootd listens to. For example, I have Origin.Url set to <hostname>:8444 which is the web url I go to, but the url that we use to push and pull from the origin is <hostname>:8443. I just think it might need some clarification in the docs/parameters.yaml description.

@haoming29
Copy link
Contributor Author

haoming29 commented Nov 20, 2023

@haoming29 Do we want to clarify Origin.Url as well? Because I think that's actually the web url and not the url that xrootd listens to. For example, I have Origin.Url set to <hostname>:8444 which is the web url I go to, but the url that we use to push and pull from the origin is <hostname>:8443. I just think it might need some clarification in the docs/parameters.yaml description.

That's even more interesting because I thought Origin.Url is the url to xrootd. We used Origin.Url in the adverse here. And we set that value in here where the port is the port that xrootd listen to.

I think the reason why changing Origin.Url didn't change the port that xrootd listens to is because we didn't reverse parse the port number in Origin.Url to Xoortd.Port which is what's being used in xrootd config to specify the port in here.

I would suggest that I will clarify the intent for Origin.Url in parameter.yaml without changing the name of if for now (or if we want to rename it as Origin.XrootdUrl I'm also happy to do so), but I will add to the description to clarify the intent for this parameter. I will add another ticket to fix for the inconsistent values with Origin.Url being set.

@turetske
Copy link
Collaborator

@haoming29 Do we want to clarify Origin.Url as well? Because I think that's actually the web url and not the url that xrootd listens to. For example, I have Origin.Url set to <hostname>:8444 which is the web url I go to, but the url that we use to push and pull from the origin is <hostname>:8443. I just think it might need some clarification in the docs/parameters.yaml description.

That's even more interesting because I thought Origin.Url is the url to xrootd. We used Origin.Url in the adverse here. And we set that value in here where the port is the port that xrootd listen to.

I think the reason why changing Origin.Url didn't change the port that xrootd listens to is because we didn't reverse parse the port number in Origin.Url to Xoortd.Port which is what's being used in xrootd config to specify the port in here.

I would suggest that I will clarify the intent for Origin.Url in parameter.yaml without changing the name of if for now (or if we want to rename it as Origin.XrootdUrl I'm also happy to do so), but I will add to the description to clarify the intent for this parameter. I will add another ticket to fix for the inconsistent values with Origin.Url being set.

Then we should probably add something for the weburl? Which needs to be different than Origin.Url as we can't be using the same port for both the website and xrootd. Though this may be part of that additional ticket you mentioned.

@haoming29
Copy link
Contributor Author

haoming29 commented Nov 22, 2023

@turetske I've added clarification to parameters.yaml but I skipped adding similar stuff to Xrootd.Port and Server.Port assuming people should be aware of the distinctiveness of the two if they want to set both to other values. Let me know what you think

Copy link
Collaborator

@turetske turetske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@turetske turetske merged commit 5f9f5c9 into PelicanPlatform:main Nov 22, 2023
@haoming29 haoming29 deleted the imprv-compute-external-add branch November 29, 2023 15:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config.ComputeExternalAddress() returns inconsistent values
2 participants