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

Purge topology origin servers when there is a duplicated server from Pelican #1142

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

haoming29
Copy link
Contributor

@haoming29 haoming29 commented Apr 19, 2024

Closes #1104

The core idea of this PR is as following:

  • For all serverAd-s and their namespaces, we store them as-is, no matter they are from Topology or Pelican. We mark the ones from Topology with a new field FromTopology
  • When director receives an object request, it will go over the serverAds and when doing match-making (related code is in getAdsForPath in director/cache_ads.go:
    • if origin candidate list is empty, set the matched origin server to the list, no matter from topology or not
    • if origin list is not empty, and the LAST item and the next matched origin ad are from the same source, simply add the next matched origin to the list
    • If they are different, then
      • If the next matched one is from topology, then ignore it (meaning existing candidates are all from Pelican)
      • If the next matched one is from Pelican, then remove the existing candidates and put the Pelican one into the list
    • For cache servers, as long as they matched the namespace, set them to the cache candidate list

What this PR hasn't covered yet, is the list of namespaces and their servers the director sends to the cache for generating scitokens/authfile. We should do the same check ands filter out the duplicated topology server. Not sure if I should include this aspect in this PR or make it a separate one.

This PR also make sure we stable-sort the serverAd first in getAdsForPath and always generate the stable result (the current code gives random result depending on serverAds ordering of its Items() which is random ordering).

@haoming29 haoming29 added this to the v7.8.0 milestone Apr 19, 2024
@haoming29 haoming29 added critical High priority for next release director Issue relating to the director component labels Apr 19, 2024
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

I think I already know the answer to this question, but I just want to confirm -- if an old origin/namespace advertises to this new director with an old server/namespace ad that doesn't have FromTopology, will the director automatically unmarshal the response and set that value to false? If that's the case, I think we're okay to modify the struct without worrying about hurting backwards compatibility.

@haoming29
Copy link
Contributor Author

I think I already know the answer to this question, but I just want to confirm -- if an old origin/namespace advertises to this new director with an old server/namespace ad that doesn't have FromTopology, will the director automatically unmarshal the response and set that value to false? If that's the case, I think we're okay to modify the struct without worrying about hurting backwards compatibility.

The server advertisement should be fine, as we manually set the value (or let it be default value for Pelican server: director/director.go line 612), for namespace ad, I'm pretty sure if the incoming namespaceAd does not have FromTopology, the field is initialized to false by encoding/json, so there shouldn't be a compatibility issue.

@haoming29 haoming29 requested a review from jhiemstrawisc April 23, 2024 17:18
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

LGTM. Do we need/want to patch this one into 7.7?

@haoming29
Copy link
Contributor Author

LGTM. Do we need/want to patch this one into 7.7?

What would be a good question for @bbockelm

@haoming29 haoming29 merged commit 50f0cdf into PelicanPlatform:main Apr 23, 2024
19 checks passed
@haoming29 haoming29 deleted the fix-dir-topo branch April 23, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical High priority for next release director Issue relating to the director component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove topology serverAd if there's a Pelican advertisement with the same server name
2 participants