From 41fe45f6056429fc1e2ab7c9f0de8c31be433358 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Tue, 24 Sep 2024 15:15:11 +0000 Subject: [PATCH] Use capabilities structs in various types of server ads This moves us away from using something like `ad.Writes` in favor of `ads.Caps.Writes` by using our capabilities struct across the board where appropriate. The work in this PR has been spread out over 3 releases to minimize potential impact to backwards compatibility issues, with the 7.11 release targeted for final breaking changes. --- director/advertise.go | 17 ++++------------- director/advertise_test.go | 33 ++++++++++++++------------------- director/director.go | 26 +++++++++++--------------- origin/advertise.go | 1 - server_structs/director.go | 11 ++--------- server_structs/director_test.go | 1 - 6 files changed, 31 insertions(+), 58 deletions(-) diff --git a/director/advertise.go b/director/advertise.go index 2bf532b51..804eb8a83 100644 --- a/director/advertise.go +++ b/director/advertise.go @@ -49,10 +49,6 @@ func consolidateDupServerAd(newAd, existingAd server_structs.ServerAd) server_st consolidatedAd.Caps.Writes = existingAd.Caps.Writes || newAd.Caps.Writes consolidatedAd.Caps.Listings = existingAd.Caps.Listings || newAd.Caps.Listings - consolidatedAd.DirectReads = existingAd.DirectReads || newAd.DirectReads - consolidatedAd.Writes = existingAd.Writes || newAd.Writes - consolidatedAd.Listings = existingAd.Listings || newAd.Listings - return consolidatedAd } @@ -67,15 +63,11 @@ func parseServerAdFromTopology(server server_structs.TopoServer, serverType serv // Explicitly set these to false for caches, because these caps don't really translate in that case if serverAd.Type == server_structs.CacheType.String() { serverAd.Caps = server_structs.Capabilities{} - serverAd.Writes = false - serverAd.Listings = false - serverAd.DirectReads = false + serverAd.Caps.Writes = false + serverAd.Caps.Listings = false + serverAd.Caps.DirectReads = false + serverAd.Caps.PublicReads = true } else { - // Until we consolidate ServerAd capabilities with NamespaceAdV2 capabilities, we'll keep setting the top-level - // ServerAd capabilities. Eventually we should replace with the actual caps struct. - serverAd.Writes = caps.Writes - serverAd.Listings = caps.Listings - serverAd.DirectReads = caps.DirectReads serverAd.Caps = caps } @@ -250,7 +242,6 @@ func AdvertiseOSDF(ctx context.Context) error { } nsAd := server_structs.NamespaceAdV2{ Path: ns.Path, - PublicRead: caps.PublicReads, Caps: caps, Generation: []server_structs.TokenGen{tGen}, Issuer: tokenIssuers, diff --git a/director/advertise_test.go b/director/advertise_test.go index 5a2e05c3a..737bc5965 100644 --- a/director/advertise_test.go +++ b/director/advertise_test.go @@ -47,20 +47,20 @@ var ( func TestConsolidateDupServerAd(t *testing.T) { t.Run("union-capabilities", func(t *testing.T) { - existingAd := server_structs.ServerAd{Writes: false} - newAd := server_structs.ServerAd{Writes: true} + existingAd := server_structs.ServerAd{Caps: server_structs.Capabilities{Writes: false}} + newAd := server_structs.ServerAd{Caps: server_structs.Capabilities{Writes: true}} get := consolidateDupServerAd(newAd, existingAd) - assert.True(t, get.Writes) + assert.True(t, get.Caps.Writes) - existingAd = server_structs.ServerAd{DirectReads: false} - newAd = server_structs.ServerAd{DirectReads: true} + existingAd = server_structs.ServerAd{Caps: server_structs.Capabilities{DirectReads: false}} + newAd = server_structs.ServerAd{Caps: server_structs.Capabilities{DirectReads: true}} get = consolidateDupServerAd(newAd, existingAd) - assert.True(t, get.DirectReads) + assert.True(t, get.Caps.DirectReads) - existingAd = server_structs.ServerAd{Listings: false} - newAd = server_structs.ServerAd{Listings: true} + existingAd = server_structs.ServerAd{Caps: server_structs.Capabilities{Listings: false}} + newAd = server_structs.ServerAd{Caps: server_structs.Capabilities{Listings: true}} get = consolidateDupServerAd(newAd, existingAd) - assert.True(t, get.Listings) + assert.True(t, get.Caps.Listings) // All false existingAd = server_structs.ServerAd{Caps: server_structs.Capabilities{}} @@ -144,22 +144,19 @@ func TestParseServerAdFromTopology(t *testing.T) { Writes: true, Listings: true, DirectReads: true, + PublicReads: true, } ad := parseServerAdFromTopology(server, server_structs.OriginType, caps) - assert.True(t, ad.Writes) assert.True(t, ad.Caps.Writes) - assert.True(t, ad.Listings) assert.True(t, ad.Caps.Listings) - assert.True(t, ad.DirectReads) assert.True(t, ad.Caps.DirectReads) + assert.True(t, ad.Caps.PublicReads) ad = parseServerAdFromTopology(server, server_structs.CacheType, caps) - assert.False(t, ad.Writes) assert.False(t, ad.Caps.Writes) - assert.False(t, ad.Listings) assert.False(t, ad.Caps.Listings) - assert.False(t, ad.DirectReads) assert.False(t, ad.Caps.DirectReads) + assert.True(t, ad.Caps.PublicReads) }) t.Run("test-invalid-url", func(t *testing.T) { @@ -233,9 +230,7 @@ func TestAdvertiseOSDF(t *testing.T) { assert.Equal(t, "https://cache2.com", cAds[0].URL.String()) // Check that various capabilities have survived until this point. Because these are from topology, // origin and namespace caps should be the same - assert.True(t, oAds[0].Writes) assert.True(t, oAds[0].Caps.Writes) - assert.True(t, oAds[0].Listings) assert.True(t, oAds[0].Caps.Listings) assert.False(t, oAds[0].Caps.PublicReads) assert.True(t, nsAd.Caps.Writes) @@ -273,8 +268,8 @@ func TestAdvertiseOSDF(t *testing.T) { assert.Equal(t, server_structs.OriginType.String(), foundAd.Type) assert.Len(t, foundAd.NamespaceAds, 3) // This origin has at least one namespace enables the following capacity - assert.True(t, foundAd.DirectReads) - assert.True(t, foundAd.Writes) + assert.True(t, foundAd.Caps.DirectReads) + assert.True(t, foundAd.Caps.Writes) assert.True(t, foundAd.Caps.PublicReads) }) diff --git a/director/director.go b/director/director.go index ea43d24f9..b907bcb12 100644 --- a/director/director.go +++ b/director/director.go @@ -451,7 +451,7 @@ func redirectToCache(ginCtx *gin.Context) { if len(cacheAds) == 0 { for _, originAd := range originAdsWObject { // Find the first origin that enables direct reads as the fallback - if originAd.DirectReads { + if originAd.Caps.DirectReads { cacheAds = append(cacheAds, originAd) break } @@ -594,7 +594,7 @@ func redirectToOrigin(ginCtx *gin.Context) { // Query Origins and check if the object exists q = NewObjectStat() qr := q.Query(context.Background(), reqPath, server_structs.OriginType, 1, 3, - withOriginAds(originAds), WithToken(reqParams.Get("authz")), withAuth(!namespaceAd.PublicRead)) + withOriginAds(originAds), WithToken(reqParams.Get("authz")), withAuth(!namespaceAd.Caps.PublicReads)) log.Debugf("Stat result for %s: %s", reqPath, qr.String()) // For a successful response, we got a list of object URLs. @@ -729,8 +729,8 @@ func redirectToOrigin(ginCtx *gin.Context) { // If the namespace or the origin does not allow directory listings, then we should not advertise a collections-url. // This is because the configuration of the origin/namespace should override the inclusion of "dirlisthost" for that origin. // Listings is true by default so if it is ever set to false we should accept that config over the dirlisthost. - if namespaceAd.Caps.Listings && len(availableAds) > 0 && availableAds[0].Listings { - if !namespaceAd.PublicRead && availableAds[0].AuthURL != (url.URL{}) { + if namespaceAd.Caps.Listings && len(availableAds) > 0 && availableAds[0].Caps.Listings { + if !namespaceAd.Caps.PublicReads && availableAds[0].AuthURL != (url.URL{}) { colUrl = availableAds[0].AuthURL.String() } else { colUrl = availableAds[0].URL.String() @@ -743,8 +743,8 @@ func redirectToOrigin(ginCtx *gin.Context) { // If we are doing a PROPFIND, check if origins enable dirlistings if ginCtx.Request.Method == "PROPFIND" { for idx, ad := range availableAds { - if ad.Listings && namespaceAd.Caps.Listings { - redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.PublicRead) + if ad.Caps.Listings && namespaceAd.Caps.Listings { + redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.Caps.PublicReads) if brokerUrl := availableAds[idx].BrokerURL; brokerUrl.String() != "" { ginCtx.Header("X-Pelican-Broker", brokerUrl.String()) } @@ -765,8 +765,8 @@ func redirectToOrigin(ginCtx *gin.Context) { // Check if we are doing a DirectRead and if it is allowed if reqParams.Has(utils.QueryDirectRead.String()) { for idx, originAd := range availableAds { - if originAd.DirectReads && namespaceAd.Caps.DirectReads { - redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.PublicRead) + if originAd.Caps.DirectReads && namespaceAd.Caps.DirectReads { + redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.Caps.PublicReads) if brokerUrl := availableAds[idx].BrokerURL; brokerUrl.String() != "" { ginCtx.Header("X-Pelican-Broker", brokerUrl.String()) } @@ -788,8 +788,8 @@ func redirectToOrigin(ginCtx *gin.Context) { // If we are doing a PUT, check to see if any origins are writeable if ginCtx.Request.Method == "PUT" { for idx, ad := range availableAds { - if ad.Writes { - redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.PublicRead) + if ad.Caps.Writes { + redirectURL = getRedirectURL(reqPath, availableAds[idx], !namespaceAd.Caps.PublicReads) if brokerUrl := availableAds[idx].BrokerURL; brokerUrl.String() != "" { ginCtx.Header("X-Pelican-Broker", brokerUrl.String()) } @@ -803,7 +803,7 @@ func redirectToOrigin(ginCtx *gin.Context) { }) return } else { // Otherwise, we are doing a GET - redirectURL := getRedirectURL(reqPath, availableAds[0], !namespaceAd.PublicRead) + redirectURL := getRedirectURL(reqPath, availableAds[0], !namespaceAd.Caps.PublicReads) if brokerUrl := availableAds[0].BrokerURL; brokerUrl.String() != "" { ginCtx.Header("X-Pelican-Broker", brokerUrl.String()) } @@ -1112,9 +1112,6 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s BrokerURL: *brokerUrl, Type: sType.String(), Caps: adV2.Caps, - Writes: adV2.Caps.Writes, - DirectReads: adV2.Caps.DirectReads, - Listings: adV2.Caps.Listings, IOLoad: 0.0, // Explicitly set to 0. The sort algorithm takes 0.0 as unknown load } @@ -1215,7 +1212,6 @@ func listNamespacesV1(ctx *gin.Context) { func listNamespacesV2(ctx *gin.Context) { namespacesAdsV2 := listNamespacesFromOrigins() namespacesAdsV2 = append(namespacesAdsV2, server_structs.NamespaceAdV2{ - PublicRead: true, Caps: server_structs.Capabilities{ PublicReads: true, Reads: true, diff --git a/origin/advertise.go b/origin/advertise.go index 9efe9af2e..54bb370ab 100644 --- a/origin/advertise.go +++ b/origin/advertise.go @@ -100,7 +100,6 @@ func (server *OriginServer) CreateAdvertisement(name, originUrlStr, originWebUrl // PublicReads implies reads reads := export.Capabilities.PublicReads || export.Capabilities.Reads nsAds = append(nsAds, server_structs.NamespaceAdV2{ - PublicRead: export.Capabilities.PublicReads, Caps: server_structs.Capabilities{ PublicReads: export.Capabilities.PublicReads, Reads: reads, diff --git a/server_structs/director.go b/server_structs/director.go index 7167f6c09..8fdc8b5d2 100644 --- a/server_structs/director.go +++ b/server_structs/director.go @@ -55,10 +55,7 @@ type ( } NamespaceAdV2 struct { - // TODO: Deprecate this top-level PublicRead field in favor of the Caps.PublicReads field. - // Should be done ~v7.10 series - PublicRead bool - Caps Capabilities // Namespace capabilities should be considered independently of the origin’s capabilities. + Caps Capabilities `json:"capabilities"` // Namespace capabilities should be considered independently of the origin’s capabilities. Path string `json:"path"` Generation []TokenGen `json:"token-generation"` Issuer []TokenIssuer `json:"token-issuer"` @@ -87,10 +84,7 @@ type ( Type string `json:"type"` Latitude float64 `json:"latitude"` Longitude float64 `json:"longitude"` - Caps Capabilities `json:"capabilities"` // TODO: Get rid of Writes, Listings, DirectReads in favor of Caps.Writes, Caps.Listings, Caps.DirectReads - Writes bool `json:"enable_write"` - Listings bool `json:"enable_listing"` // True if the origin allows directory listings - DirectReads bool `json:"enable_fallback_read"` // True if reads from the origin are permitted when no cache is available + Caps Capabilities `json:"capabilities"` FromTopology bool `json:"from_topology"` IOLoad float64 `json:"io_load"` } @@ -456,7 +450,6 @@ func ConvertNamespaceAdsV1ToV2(nsAdsV1 []NamespaceAdV1, oAd *OriginAdvertiseV1) } newNS := NamespaceAdV2{ - PublicRead: caps.PublicReads, Caps: caps, Path: nsAd.Path, } diff --git a/server_structs/director_test.go b/server_structs/director_test.go index bd7000cfc..c05e1b1f1 100644 --- a/server_structs/director_test.go +++ b/server_structs/director_test.go @@ -66,7 +66,6 @@ func TestConversion(t *testing.T) { }}, }, { - PublicRead: true, Caps: Capabilities{ PublicReads: true, Reads: true,