Skip to content

Commit

Permalink
Address code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@haoming29
haoming29 committed Mar 18, 2024
1 parent 228ff1b commit c68cabb
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 55 deletions.
64 changes: 35 additions & 29 deletions director/director.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,19 @@ type (
}

listServerResponse struct {
Name string `json:"name"`
AuthURL string `json:"authUrl"`
BrokerURL string `json:"brokerUrl"`
URL string `json:"url"` // This is server's XRootD URL for file transfer
WebURL string `json:"webUrl"` // This is server's Web interface and API
Type common.ServerType `json:"type"`
Latitude float64 `json:"latitude"`
Longitude float64 `json:"longitude"`
EnableWrite bool `json:"enableWrite"`
EnableFallbackRead bool `json:"enableFallbackRead"`
Filtered bool `json:"filtered"`
FilteredType filterType `json:"filteredType"`
Status HealthTestStatus `json:"status"`
Name string `json:"name"`
AuthURL string `json:"authUrl"`
BrokerURL string `json:"brokerUrl"`
URL string `json:"url"` // This is server's XRootD URL for file transfer
WebURL string `json:"webUrl"` // This is server's Web interface and API
Type common.ServerType `json:"type"`
Latitude float64 `json:"latitude"`
Longitude float64 `json:"longitude"`
Writes bool `json:"enableWrite"`
DirectReads bool `json:"enableFallbackRead"`
Filtered bool `json:"filtered"`
FilteredType filterType `json:"filteredType"`
Status HealthTestStatus `json:"status"`
}

statResponse struct {
Expand Down Expand Up @@ -99,19 +99,19 @@ func listServers(ctx *gin.Context) {
}
filtered, ft := checkFilter(server.Name)
res := listServerResponse{
Name: server.Name,
BrokerURL: server.BrokerURL.String(),
AuthURL: server.AuthURL.String(),
URL: server.URL.String(),
WebURL: server.WebURL.String(),
Type: server.Type,
Latitude: server.Latitude,
Longitude: server.Longitude,
EnableWrite: server.EnableWrite,
EnableFallbackRead: server.EnableFallbackRead,
Filtered: filtered,
FilteredType: ft,
Status: healthStatus,
Name: server.Name,
BrokerURL: server.BrokerURL.String(),
AuthURL: server.AuthURL.String(),
URL: server.URL.String(),
WebURL: server.WebURL.String(),
Type: server.Type,
Latitude: server.Latitude,
Longitude: server.Longitude,
Writes: server.Writes,
DirectReads: server.DirectReads,
Filtered: filtered,
FilteredType: ft,
Status: healthStatus,
}
resList = append(resList, res)
}
Expand Down Expand Up @@ -159,29 +159,35 @@ func queryOrigins(ctx *gin.Context) {
ctx.JSON(http.StatusOK, res)
}

// A gin route handler that given a server hostname through path variable `name`,
// checks and adds the server to a list of servers to be bypassed when the director redirects
// object requests from the client
func handleFilterServer(ctx *gin.Context) {
sn := strings.TrimPrefix(ctx.Param("name"), "/")
if sn == "" {
ctx.JSON(http.StatusBadRequest, gin.H{"error": "name is a required path parameter"})
return
}
filtered, ft := checkFilter(sn)
filtered, filterType := checkFilter(sn)
if filtered {
ctx.JSON(http.StatusBadRequest, gin.H{"error": "Can't filter a server that already has been fitlered with type " + ft})
ctx.JSON(http.StatusBadRequest, gin.H{"error": "Can't filter a server that already has been fitlered with type " + filterType})
return
}
filteredServersMutex.Lock()
defer filteredServersMutex.Unlock()

// If we previously temporarily allowed a server, we switch to permFiltered (reset)
if ft == tempAllowed {
if filterType == tempAllowed {
filteredServers[sn] = permFiltered
} else {
filteredServers[sn] = tempFiltered
}
ctx.JSON(http.StatusOK, gin.H{"message": "success"})
}

// A gin route handler that given a server hostname through path variable `name`,
// checks and removes the server from a list of servers to be bypassed when the director redirects
// object requests from the client
func handleAllowServer(ctx *gin.Context) {
sn := strings.TrimPrefix(ctx.Param("name"), "/")
if sn == "" {
Expand Down
6 changes: 3 additions & 3 deletions director/director_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ func checkFilter(serverName string) (bool, filterType) {
filteredServersMutex.RLock()
defer filteredServersMutex.RUnlock()

status, ok := filteredServers[serverName]
status, exists := filteredServers[serverName]
// No filter entry
if !ok {
if !exists {
return false, ""
} else {
// Has filter entry
Expand Down Expand Up @@ -151,7 +151,7 @@ func ConfigFilterdServers() {
filteredServersMutex.Lock()
defer filteredServersMutex.Unlock()

if len(param.Director_FilteredServers.GetStringSlice()) == 0 {
if !param.Director_FilteredServers.IsSet() {
return
}

Expand Down
44 changes: 22 additions & 22 deletions director/director_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,30 +46,30 @@ func TestListServers(t *testing.T) {
}()

mocklistOriginRes := listServerResponse{
Name: mockOriginServerAd.Name,
BrokerURL: mockOriginServerAd.BrokerURL.String(),
AuthURL: mockOriginServerAd.AuthURL.String(),
URL: mockOriginServerAd.URL.String(),
WebURL: mockOriginServerAd.WebURL.String(),
Type: mockOriginServerAd.Type,
Latitude: mockOriginServerAd.Latitude,
Longitude: mockOriginServerAd.Longitude,
EnableWrite: mockOriginServerAd.EnableWrite,
EnableFallbackRead: mockOriginServerAd.EnableFallbackRead,
Status: HealthStatusUnknown,
Name: mockOriginServerAd.Name,
BrokerURL: mockOriginServerAd.BrokerURL.String(),
AuthURL: mockOriginServerAd.AuthURL.String(),
URL: mockOriginServerAd.URL.String(),
WebURL: mockOriginServerAd.WebURL.String(),
Type: mockOriginServerAd.Type,
Latitude: mockOriginServerAd.Latitude,
Longitude: mockOriginServerAd.Longitude,
Writes: mockOriginServerAd.Writes,
DirectReads: mockOriginServerAd.DirectReads,
Status: HealthStatusUnknown,
}
mocklistCacheRes := listServerResponse{
Name: mockCacheServerAd.Name,
BrokerURL: mockCacheServerAd.BrokerURL.String(),
AuthURL: mockCacheServerAd.AuthURL.String(),
URL: mockCacheServerAd.URL.String(),
WebURL: mockCacheServerAd.WebURL.String(),
Type: mockCacheServerAd.Type,
Latitude: mockCacheServerAd.Latitude,
Longitude: mockCacheServerAd.Longitude,
EnableWrite: mockCacheServerAd.EnableWrite,
EnableFallbackRead: mockCacheServerAd.EnableFallbackRead,
Status: HealthStatusUnknown,
Name: mockCacheServerAd.Name,
BrokerURL: mockCacheServerAd.BrokerURL.String(),
AuthURL: mockCacheServerAd.AuthURL.String(),
URL: mockCacheServerAd.URL.String(),
WebURL: mockCacheServerAd.WebURL.String(),
Type: mockCacheServerAd.Type,
Latitude: mockCacheServerAd.Latitude,
Longitude: mockCacheServerAd.Longitude,
Writes: mockCacheServerAd.Writes,
DirectReads: mockCacheServerAd.DirectReads,
Status: HealthStatusUnknown,
}

t.Run("query-origin", func(t *testing.T) {
Expand Down
3 changes: 2 additions & 1 deletion docs/parameters.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,8 @@ components: ["director"]
---
name: Director.FilteredServers
description: >-
A list of server names to not to redirect client requests to.
A list of server host names to not to redirect client requests to. This is for admins to put a list of
servers in the federation into downtime.
type: stringSlice
default: none
components: ["director"]
Expand Down

0 comments on commit c68cabb

Please sign in to comment.