From c457c455a74c1ce100237423e53c7a77d54f6db7 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Thu, 17 Oct 2024 21:55:11 +0000 Subject: [PATCH 01/12] director db: setup, basic APIs, unit tests --- director/director_db.go | 109 ++++++++++++++++++ director/director_db_test.go | 57 +++++++++ director/maxmind.go | 2 +- .../20241017135850_create_db_tables.sql | 14 +++ docs/parameters.yaml | 8 ++ launchers/director_serve.go | 5 +- param/parameters.go | 1 + param/parameters_struct.go | 2 + 8 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 director/director_db.go create mode 100644 director/director_db_test.go create mode 100644 director/migrations/20241017135850_create_db_tables.sql diff --git a/director/director_db.go b/director/director_db.go new file mode 100644 index 000000000..8de7835ca --- /dev/null +++ b/director/director_db.go @@ -0,0 +1,109 @@ +/*************************************************************** + * + * Copyright (C) 2024, Pelican Project, Morgridge Institute for Research + * + * Licensed under the Apache License, Version 2.0 (the "License"); you + * may not use this file except in compliance with the License. You may + * obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ***************************************************************/ +package director + +import ( + "embed" + "time" + + "github.com/google/uuid" + "github.com/pelicanplatform/pelican/param" + "github.com/pelicanplatform/pelican/server_utils" + "github.com/pkg/errors" + "gorm.io/gorm" +) + +type ServerStatus struct { + UUID string `gorm:"primaryKey"` + URL string `gorm:"not null;default:''"` + Downtime bool `gorm:"not null;default:false"` + // We don't use gorm default gorm.Model to change ID type to string + CreatedAt time.Time + UpdatedAt time.Time + DeletedAt gorm.DeletedAt +} + +var db *gorm.DB + +//go:embed migrations/*.sql +var embedMigrations embed.FS + +func InitializeDB() error { + dbPath := param.Director_DbLocation.GetString() + tdb, err := server_utils.InitSQLiteDB(dbPath) + if err != nil { + return err + } + db = tdb + sqldb, err := db.DB() + if err != nil { + return errors.Wrapf(err, "Failed to get sql.DB from gorm DB: %s", dbPath) + } + // Run database migrations + if err := server_utils.MigrateDB(sqldb, embedMigrations); err != nil { + return err + } + return nil +} + +func ShutdownDirectorDB() error { + return server_utils.ShutdownDB(db) +} + +func CreateServerStatus(url string) error { + id, err := uuid.NewV7() + if err != nil { + return errors.Wrap(err, "Unable to create new UUID for new entry in server status table") + } + serverStatus := ServerStatus{ + UUID: id.String(), + URL: url, + Downtime: false, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + } + + if err := db.Create(serverStatus).Error; err != nil { + return err + } + return nil +} + +func GetServerDowntime(url string) (bool, error) { + var serverStatus ServerStatus + err := db.First(&serverStatus, "url = ?", url).Error + if err != nil { + return false, err + } + return serverStatus.Downtime, nil +} + +func SetServerDowntime(downtime bool, url string) error { + var serverStatus ServerStatus + err := db.First(&serverStatus, "url = ?", url).Error + if err != nil { + return err + } + serverStatus.Downtime = downtime + serverStatus.UpdatedAt = time.Now() + + if err := db.Save(&serverStatus).Error; err != nil { + return err + } + return nil +} diff --git a/director/director_db_test.go b/director/director_db_test.go new file mode 100644 index 000000000..261fd6927 --- /dev/null +++ b/director/director_db_test.go @@ -0,0 +1,57 @@ +package director + +import ( + "testing" + + "github.com/glebarez/sqlite" + "github.com/google/uuid" + "github.com/pelicanplatform/pelican/server_utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +var ( + mockSS []ServerStatus = []ServerStatus{ + {UUID: uuid.NewString(), URL: "https://4a334d532d69:8443", Downtime: false}, + {UUID: uuid.NewString(), URL: "https://my-origin.com:8443", Downtime: true}, + {UUID: uuid.NewString(), URL: "https://my-cache.com:8447"}, + } +) + +func setupMockDirectorDB(t *testing.T) { + mockDB, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + db = mockDB + require.NoError(t, err, "Error setting up mock origin DB") + err = db.AutoMigrate(&ServerStatus{}) + require.NoError(t, err, "Failed to migrate DB for Globus table") +} + +func teardownMockDirectorDB(t *testing.T) { + err := ShutdownDirectorDB() + require.NoError(t, err, "Error tearing down mock director DB") +} + +func insertMockDBData(ss []ServerStatus) error { + return db.Create(&ss).Error +} + +func TestDirectorDBBasics(t *testing.T) { + server_utils.ResetTestState() + setupMockDirectorDB(t) + t.Cleanup(func() { + teardownMockDirectorDB(t) + }) + err := insertMockDBData(mockSS) + require.NoError(t, err) + + downtime, err := GetServerDowntime(mockSS[1].URL) + assert.True(t, downtime) + require.NoError(t, err) + + err = SetServerDowntime(false, mockSS[1].URL) + require.NoError(t, err) + downtime, err = GetServerDowntime(mockSS[1].URL) + assert.False(t, downtime) + require.NoError(t, err) +} diff --git a/director/maxmind.go b/director/maxmind.go index 0b985fcd9..340334be2 100644 --- a/director/maxmind.go +++ b/director/maxmind.go @@ -144,7 +144,7 @@ func periodicMaxMindReload(ctx context.Context) { } } -func InitializeDB(ctx context.Context) { +func InitializeGeoIPDB(ctx context.Context) { go periodicMaxMindReload(ctx) localFile := param.Director_GeoIPLocation.GetString() localReader, err := geoip2.Open(localFile) diff --git a/director/migrations/20241017135850_create_db_tables.sql b/director/migrations/20241017135850_create_db_tables.sql new file mode 100644 index 000000000..05a750721 --- /dev/null +++ b/director/migrations/20241017135850_create_db_tables.sql @@ -0,0 +1,14 @@ +-- +goose Up +-- +goose StatementBegin +CREATE TABLE server_status ( + uuid TEXT PRIMARY KEY, + url TEXT NOT NULL DEFAULT '', + downtime BOOLEAN NOT NULL DEFAULT 0, + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL +); +-- +goose StatementEnd + +-- +goose Down +-- +goose StatementBegin +-- +goose StatementEnd diff --git a/docs/parameters.yaml b/docs/parameters.yaml index abbac6268..327fb7eef 100644 --- a/docs/parameters.yaml +++ b/docs/parameters.yaml @@ -1243,6 +1243,14 @@ components: ["cache"] ############################ # Director-level configs # ############################ +name: Director.DbLocation +description: |+ + A filepath to the intended location of the director's database. +type: filename +root_default: /var/lib/pelican/director.sqlite +default: $ConfigBase/director.sqlite +components: ["director"] +--- name: Director.DefaultResponse description: |+ The default response type of a redirect for a director instance. Can be either "cache" or "origin". If a director diff --git a/launchers/director_serve.go b/launchers/director_serve.go index 44c72b462..06baae90c 100644 --- a/launchers/director_serve.go +++ b/launchers/director_serve.go @@ -37,7 +37,7 @@ import ( func DirectorServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group) error { log.Info("Initializing Director GeoIP database...") - director.InitializeDB(ctx) + director.InitializeGeoIPDB(ctx) director.ConfigFilterdServers() @@ -75,6 +75,9 @@ func DirectorServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group return errors.Wrap(err, "invalid URL for Director.SupportContactUrl") } } + if err := director.InitializeDB(); err != nil { + return errors.Wrap(err, "failed to initialize director sqlite database") + } rootGroup := engine.Group("/") director.RegisterDirectorOIDCAPI(rootGroup) director.RegisterDirectorWebAPI(rootGroup) diff --git a/param/parameters.go b/param/parameters.go index f00cb0909..a8c9da600 100644 --- a/param/parameters.go +++ b/param/parameters.go @@ -152,6 +152,7 @@ var ( Cache_Url = StringParam{"Cache.Url"} Cache_XRootDPrefix = StringParam{"Cache.XRootDPrefix"} Director_CacheSortMethod = StringParam{"Director.CacheSortMethod"} + Director_DbLocation = StringParam{"Director.DbLocation"} Director_DefaultResponse = StringParam{"Director.DefaultResponse"} Director_GeoIPLocation = StringParam{"Director.GeoIPLocation"} Director_MaxMindKeyFile = StringParam{"Director.MaxMindKeyFile"} diff --git a/param/parameters_struct.go b/param/parameters_struct.go index 1e39835a2..9c5ba988d 100644 --- a/param/parameters_struct.go +++ b/param/parameters_struct.go @@ -68,6 +68,7 @@ type Config struct { CachesPullFromCaches bool `mapstructure:"cachespullfromcaches"` CheckCachePresence bool `mapstructure:"checkcachepresence"` CheckOriginPresence bool `mapstructure:"checkoriginpresence"` + DbLocation string `mapstructure:"dblocation"` DefaultResponse string `mapstructure:"defaultresponse"` EnableBroker bool `mapstructure:"enablebroker"` EnableOIDC bool `mapstructure:"enableoidc"` @@ -370,6 +371,7 @@ type configWithType struct { CachesPullFromCaches struct { Type string; Value bool } CheckCachePresence struct { Type string; Value bool } CheckOriginPresence struct { Type string; Value bool } + DbLocation struct { Type string; Value string } DefaultResponse struct { Type string; Value string } EnableBroker struct { Type string; Value bool } EnableOIDC struct { Type string; Value bool } From fd936b128f177e0baad73205a720d2680ca26bcb Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Fri, 18 Oct 2024 22:45:32 +0000 Subject: [PATCH 02/12] integrate new DB, with some TODOs left --- director/director_api.go | 11 +++- director/director_db.go | 58 +++++++++++++------ director/director_db_test.go | 46 +++++++++++---- director/director_ui.go | 4 ++ .../20241017135850_create_db_tables.sql | 4 +- 5 files changed, 91 insertions(+), 32 deletions(-) diff --git a/director/director_api.go b/director/director_api.go index f91c16ec8..0b2d5403b 100644 --- a/director/director_api.go +++ b/director/director_api.go @@ -223,7 +223,7 @@ func hookServerAdsCache() { }) } -// Populate internal filteredServers map by Director.FilteredServers +// Populate internal filteredServers map by Director.FilteredServers and director db func ConfigFilterdServers() { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() @@ -235,6 +235,15 @@ func ConfigFilterdServers() { for _, sn := range param.Director_FilteredServers.GetStringSlice() { filteredServers[sn] = permFiltered } + + persistedServerStatuses, err := GetAllServerStatuses() + if err != nil { + log.Error("Failed to read persisted ServerStatuses from director db", err) + return + } + for _, serverStatus := range persistedServerStatuses { + filteredServers[serverStatus.Name] = serverStatus.FilterType + } } // Start a goroutine to query director's Prometheus endpoint for origin/cache server I/O stats diff --git a/director/director_db.go b/director/director_db.go index 8de7835ca..c877f6285 100644 --- a/director/director_db.go +++ b/director/director_db.go @@ -29,9 +29,9 @@ import ( ) type ServerStatus struct { - UUID string `gorm:"primaryKey"` - URL string `gorm:"not null;default:''"` - Downtime bool `gorm:"not null;default:false"` + UUID string `gorm:"primaryKey"` + Name string `gorm:"not null;unique"` + FilterType filterType `gorm:"type:text;not null"` // We don't use gorm default gorm.Model to change ID type to string CreatedAt time.Time UpdatedAt time.Time @@ -65,17 +65,17 @@ func ShutdownDirectorDB() error { return server_utils.ShutdownDB(db) } -func CreateServerStatus(url string) error { +func CreateServerStatus(name string, filterType filterType) error { id, err := uuid.NewV7() if err != nil { return errors.Wrap(err, "Unable to create new UUID for new entry in server status table") } serverStatus := ServerStatus{ - UUID: id.String(), - URL: url, - Downtime: false, - CreatedAt: time.Now(), - UpdatedAt: time.Now(), + UUID: id.String(), + Name: name, + FilterType: filterType, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), } if err := db.Create(serverStatus).Error; err != nil { @@ -84,22 +84,37 @@ func CreateServerStatus(url string) error { return nil } -func GetServerDowntime(url string) (bool, error) { +func GetServerStatus(name string) (filterType, error) { var serverStatus ServerStatus - err := db.First(&serverStatus, "url = ?", url).Error + err := db.First(&serverStatus, "name = ?", name).Error if err != nil { - return false, err + return "", err } - return serverStatus.Downtime, nil + return filterType(serverStatus.FilterType), nil } -func SetServerDowntime(downtime bool, url string) error { +func GetAllServerStatuses() ([]ServerStatus, error) { + var statuses []ServerStatus + result := db.Find(&statuses) + + if result.Error != nil { + return nil, result.Error + } + return statuses, nil +} + +// Set filterType of a given server. If the server doesn't exist in director db, create a new entry for it +func SetServerStatus(name string, filterType filterType) error { var serverStatus ServerStatus - err := db.First(&serverStatus, "url = ?", url).Error - if err != nil { - return err + err := db.First(&serverStatus, "name = ?", name).Error + + if errors.Is(err, gorm.ErrRecordNotFound) { + return CreateServerStatus(name, filterType) + } else if err != nil { + return errors.Wrap(err, "Error retrieving Server Status") } - serverStatus.Downtime = downtime + + serverStatus.FilterType = filterType serverStatus.UpdatedAt = time.Now() if err := db.Save(&serverStatus).Error; err != nil { @@ -107,3 +122,10 @@ func SetServerDowntime(downtime bool, url string) error { } return nil } + +func DeleteServerStatus(name string) error { + if err := db.Where("name = ?", name).Delete(&ServerStatus{}).Error; err != nil { + return errors.Wrap(err, "Failed to delete an entry in Server Status table") + } + return nil +} diff --git a/director/director_db_test.go b/director/director_db_test.go index 261fd6927..24df9438d 100644 --- a/director/director_db_test.go +++ b/director/director_db_test.go @@ -13,9 +13,9 @@ import ( var ( mockSS []ServerStatus = []ServerStatus{ - {UUID: uuid.NewString(), URL: "https://4a334d532d69:8443", Downtime: false}, - {UUID: uuid.NewString(), URL: "https://my-origin.com:8443", Downtime: true}, - {UUID: uuid.NewString(), URL: "https://my-cache.com:8447"}, + {UUID: uuid.NewString(), Name: "/4a334d532d69:8443", FilterType: tempAllowed}, + {UUID: uuid.NewString(), Name: "/my-origin.com/foo/Bar", FilterType: permFiltered}, + {UUID: uuid.NewString(), Name: "/my-cache.com/chtc", FilterType: permFiltered}, } ) @@ -45,13 +45,37 @@ func TestDirectorDBBasics(t *testing.T) { err := insertMockDBData(mockSS) require.NoError(t, err) - downtime, err := GetServerDowntime(mockSS[1].URL) - assert.True(t, downtime) - require.NoError(t, err) + t.Run("get-downtime", func(t *testing.T) { + filterType, err := GetServerStatus(mockSS[1].Name) + assert.Equal(t, filterType, permFiltered) + require.NoError(t, err) + }) - err = SetServerDowntime(false, mockSS[1].URL) - require.NoError(t, err) - downtime, err = GetServerDowntime(mockSS[1].URL) - assert.False(t, downtime) - require.NoError(t, err) + t.Run("get-all-downtime", func(t *testing.T) { + statuses, err := GetAllServerStatuses() + require.NoError(t, err) + assert.Len(t, statuses, len(mockSS)) + }) + + t.Run("set-downtime", func(t *testing.T) { + err = SetServerStatus(mockSS[1].Name, tempAllowed) + require.NoError(t, err) + filterType, err := GetServerStatus(mockSS[1].Name) + assert.Equal(t, filterType, tempAllowed) + require.NoError(t, err) + }) + + t.Run("duplicate-name-insert", func(t *testing.T) { + err := CreateServerStatus(mockSS[1].Name, tempAllowed) + require.Error(t, err) + assert.Contains(t, err.Error(), "UNIQUE constraint failed") + }) + + t.Run("delete-downtime-entry-from-directory-db", func(t *testing.T) { + err = DeleteServerStatus(mockSS[0].Name) + require.NoError(t, err, "Error deleting server status") + + _, err = GetServerStatus(mockSS[0].Name) + assert.Error(t, err, "Expected error retrieving deleted server status") + }) } diff --git a/director/director_ui.go b/director/director_ui.go index fe70b0753..5eb51369d 100644 --- a/director/director_ui.go +++ b/director/director_ui.go @@ -251,8 +251,10 @@ func handleFilterServer(ctx *gin.Context) { // If we previously temporarily allowed a server, we switch to permFiltered (reset) if filterType == tempAllowed { filteredServers[sn] = permFiltered + SetServerStatus(sn, permFiltered) } else { filteredServers[sn] = tempFiltered + SetServerStatus(sn, tempFiltered) } ctx.JSON(http.StatusOK, server_structs.SimpleApiResp{Status: server_structs.RespOK, Msg: "success"}) } @@ -284,9 +286,11 @@ func handleAllowServer(ctx *gin.Context) { if ft == tempFiltered { // For temporarily filtered server, allowing them by removing the server from the map delete(filteredServers, sn) + DeleteServerStatus(sn) } else if ft == permFiltered { // For servers to filter from the config, temporarily allow the server filteredServers[sn] = tempAllowed + SetServerStatus(sn, tempAllowed) } else if ft == topoFiltered { ctx.JSON(http.StatusBadRequest, server_structs.SimpleApiResp{ Status: server_structs.RespFailed, diff --git a/director/migrations/20241017135850_create_db_tables.sql b/director/migrations/20241017135850_create_db_tables.sql index 05a750721..5915f84d9 100644 --- a/director/migrations/20241017135850_create_db_tables.sql +++ b/director/migrations/20241017135850_create_db_tables.sql @@ -2,8 +2,8 @@ -- +goose StatementBegin CREATE TABLE server_status ( uuid TEXT PRIMARY KEY, - url TEXT NOT NULL DEFAULT '', - downtime BOOLEAN NOT NULL DEFAULT 0, + name TEXT NOT NULL UNIQUE, + filter_type TEXT NOT NULL, created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL ); From ab480367d04ca07dc6804dcb7f6f77618439969d Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Mon, 21 Oct 2024 21:11:47 +0000 Subject: [PATCH 03/12] populate the downtimes from db; improve func naming and comments --- director/director_api.go | 33 ++++++++------- director/director_db.go | 41 ++++++++++--------- director/director_db_test.go | 20 ++++----- director/director_ui.go | 8 ++-- .../20241017135850_create_db_tables.sql | 2 +- launchers/director_serve.go | 6 +-- 6 files changed, 57 insertions(+), 53 deletions(-) diff --git a/director/director_api.go b/director/director_api.go index 0b2d5403b..d20e8a2ac 100644 --- a/director/director_api.go +++ b/director/director_api.go @@ -66,7 +66,7 @@ func listAdvertisement(serverTypes []server_structs.ServerType) []*server_struct func checkFilter(serverName string) (bool, filterType) { filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() - + log.Debugln("Checking the filter applied on server", serverName, "Current filteredServers map:", filteredServers) status, exists := filteredServers[serverName] // No filter entry if !exists { @@ -223,26 +223,29 @@ func hookServerAdsCache() { }) } -// Populate internal filteredServers map by Director.FilteredServers and director db +// Populate internal filteredServers map using Director.FilteredServers param and director db func ConfigFilterdServers() { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() - if !param.Director_FilteredServers.IsSet() { - return - } - - for _, sn := range param.Director_FilteredServers.GetStringSlice() { - filteredServers[sn] = permFiltered + if param.Director_FilteredServers.IsSet() { + for _, sn := range param.Director_FilteredServers.GetStringSlice() { + filteredServers[sn] = permFiltered + } + log.Debugln("Loaded filtered servers config from Director.FilteredServers param", filteredServers) } - persistedServerStatuses, err := GetAllServerStatuses() - if err != nil { - log.Error("Failed to read persisted ServerStatuses from director db", err) - return - } - for _, serverStatus := range persistedServerStatuses { - filteredServers[serverStatus.Name] = serverStatus.FilterType + if param.Director_DbLocation.IsSet() { + persistedServerDowntimes, err := GetAllServerDowntimes() + if err != nil { + log.Error("Failed to read persisted ServerDowntimes from director db", err) + return + } + for _, serverDowntime := range persistedServerDowntimes { + filteredServers[serverDowntime.Name] = serverDowntime.FilterType + } + log.Debugln("Loaded filtered servers config from director db's server_downtimes table, the final filteredServers var has the following value", filteredServers) + // if a filtered server config rule is set in both Director.FilteredServers param and director db, the latter one will eventually be used } } diff --git a/director/director_db.go b/director/director_db.go index c877f6285..9a4c93226 100644 --- a/director/director_db.go +++ b/director/director_db.go @@ -26,16 +26,16 @@ import ( "github.com/pelicanplatform/pelican/server_utils" "github.com/pkg/errors" "gorm.io/gorm" + "gorm.io/gorm/logger" ) -type ServerStatus struct { +type ServerDowntime struct { UUID string `gorm:"primaryKey"` Name string `gorm:"not null;unique"` FilterType filterType `gorm:"type:text;not null"` // We don't use gorm default gorm.Model to change ID type to string CreatedAt time.Time UpdatedAt time.Time - DeletedAt gorm.DeletedAt } var db *gorm.DB @@ -65,12 +65,12 @@ func ShutdownDirectorDB() error { return server_utils.ShutdownDB(db) } -func CreateServerStatus(name string, filterType filterType) error { +func CreateServerDowntime(name string, filterType filterType) error { id, err := uuid.NewV7() if err != nil { return errors.Wrap(err, "Unable to create new UUID for new entry in server status table") } - serverStatus := ServerStatus{ + serverDowntime := ServerDowntime{ UUID: id.String(), Name: name, FilterType: filterType, @@ -78,23 +78,23 @@ func CreateServerStatus(name string, filterType filterType) error { UpdatedAt: time.Now(), } - if err := db.Create(serverStatus).Error; err != nil { + if err := db.Create(serverDowntime).Error; err != nil { return err } return nil } -func GetServerStatus(name string) (filterType, error) { - var serverStatus ServerStatus - err := db.First(&serverStatus, "name = ?", name).Error +func GetServerDowntime(name string) (filterType, error) { + var serverDowntime ServerDowntime + err := db.First(&serverDowntime, "name = ?", name).Error if err != nil { return "", err } - return filterType(serverStatus.FilterType), nil + return filterType(serverDowntime.FilterType), nil } -func GetAllServerStatuses() ([]ServerStatus, error) { - var statuses []ServerStatus +func GetAllServerDowntimes() ([]ServerDowntime, error) { + var statuses []ServerDowntime result := db.Find(&statuses) if result.Error != nil { @@ -104,27 +104,28 @@ func GetAllServerStatuses() ([]ServerStatus, error) { } // Set filterType of a given server. If the server doesn't exist in director db, create a new entry for it -func SetServerStatus(name string, filterType filterType) error { - var serverStatus ServerStatus - err := db.First(&serverStatus, "name = ?", name).Error +func SetServerDowntime(name string, filterType filterType) error { + var serverDowntime ServerDowntime + // slience the logger for this query because there's definitely an ErrRecordNotFound when a new downtime info inserted + err := db.Session(&gorm.Session{Logger: db.Logger.LogMode(logger.Silent)}).First(&serverDowntime, "name = ?", name).Error if errors.Is(err, gorm.ErrRecordNotFound) { - return CreateServerStatus(name, filterType) + return CreateServerDowntime(name, filterType) } else if err != nil { return errors.Wrap(err, "Error retrieving Server Status") } - serverStatus.FilterType = filterType - serverStatus.UpdatedAt = time.Now() + serverDowntime.FilterType = filterType + serverDowntime.UpdatedAt = time.Now() - if err := db.Save(&serverStatus).Error; err != nil { + if err := db.Save(&serverDowntime).Error; err != nil { return err } return nil } -func DeleteServerStatus(name string) error { - if err := db.Where("name = ?", name).Delete(&ServerStatus{}).Error; err != nil { +func DeleteServerDowntime(name string) error { + if err := db.Where("name = ?", name).Delete(&ServerDowntime{}).Error; err != nil { return errors.Wrap(err, "Failed to delete an entry in Server Status table") } return nil diff --git a/director/director_db_test.go b/director/director_db_test.go index 24df9438d..d4f9e8ff9 100644 --- a/director/director_db_test.go +++ b/director/director_db_test.go @@ -12,7 +12,7 @@ import ( ) var ( - mockSS []ServerStatus = []ServerStatus{ + mockSS []ServerDowntime = []ServerDowntime{ {UUID: uuid.NewString(), Name: "/4a334d532d69:8443", FilterType: tempAllowed}, {UUID: uuid.NewString(), Name: "/my-origin.com/foo/Bar", FilterType: permFiltered}, {UUID: uuid.NewString(), Name: "/my-cache.com/chtc", FilterType: permFiltered}, @@ -23,7 +23,7 @@ func setupMockDirectorDB(t *testing.T) { mockDB, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) db = mockDB require.NoError(t, err, "Error setting up mock origin DB") - err = db.AutoMigrate(&ServerStatus{}) + err = db.AutoMigrate(&ServerDowntime{}) require.NoError(t, err, "Failed to migrate DB for Globus table") } @@ -32,7 +32,7 @@ func teardownMockDirectorDB(t *testing.T) { require.NoError(t, err, "Error tearing down mock director DB") } -func insertMockDBData(ss []ServerStatus) error { +func insertMockDBData(ss []ServerDowntime) error { return db.Create(&ss).Error } @@ -46,36 +46,36 @@ func TestDirectorDBBasics(t *testing.T) { require.NoError(t, err) t.Run("get-downtime", func(t *testing.T) { - filterType, err := GetServerStatus(mockSS[1].Name) + filterType, err := GetServerDowntime(mockSS[1].Name) assert.Equal(t, filterType, permFiltered) require.NoError(t, err) }) t.Run("get-all-downtime", func(t *testing.T) { - statuses, err := GetAllServerStatuses() + statuses, err := GetAllServerDowntimes() require.NoError(t, err) assert.Len(t, statuses, len(mockSS)) }) t.Run("set-downtime", func(t *testing.T) { - err = SetServerStatus(mockSS[1].Name, tempAllowed) + err = SetServerDowntime(mockSS[1].Name, tempAllowed) require.NoError(t, err) - filterType, err := GetServerStatus(mockSS[1].Name) + filterType, err := GetServerDowntime(mockSS[1].Name) assert.Equal(t, filterType, tempAllowed) require.NoError(t, err) }) t.Run("duplicate-name-insert", func(t *testing.T) { - err := CreateServerStatus(mockSS[1].Name, tempAllowed) + err := CreateServerDowntime(mockSS[1].Name, tempAllowed) require.Error(t, err) assert.Contains(t, err.Error(), "UNIQUE constraint failed") }) t.Run("delete-downtime-entry-from-directory-db", func(t *testing.T) { - err = DeleteServerStatus(mockSS[0].Name) + err = DeleteServerDowntime(mockSS[0].Name) require.NoError(t, err, "Error deleting server status") - _, err = GetServerStatus(mockSS[0].Name) + _, err = GetServerDowntime(mockSS[0].Name) assert.Error(t, err, "Expected error retrieving deleted server status") }) } diff --git a/director/director_ui.go b/director/director_ui.go index 5eb51369d..9cc6d2746 100644 --- a/director/director_ui.go +++ b/director/director_ui.go @@ -251,10 +251,10 @@ func handleFilterServer(ctx *gin.Context) { // If we previously temporarily allowed a server, we switch to permFiltered (reset) if filterType == tempAllowed { filteredServers[sn] = permFiltered - SetServerStatus(sn, permFiltered) + SetServerDowntime(sn, permFiltered) } else { filteredServers[sn] = tempFiltered - SetServerStatus(sn, tempFiltered) + SetServerDowntime(sn, tempFiltered) } ctx.JSON(http.StatusOK, server_structs.SimpleApiResp{Status: server_structs.RespOK, Msg: "success"}) } @@ -286,11 +286,11 @@ func handleAllowServer(ctx *gin.Context) { if ft == tempFiltered { // For temporarily filtered server, allowing them by removing the server from the map delete(filteredServers, sn) - DeleteServerStatus(sn) + DeleteServerDowntime(sn) } else if ft == permFiltered { // For servers to filter from the config, temporarily allow the server filteredServers[sn] = tempAllowed - SetServerStatus(sn, tempAllowed) + SetServerDowntime(sn, tempAllowed) } else if ft == topoFiltered { ctx.JSON(http.StatusBadRequest, server_structs.SimpleApiResp{ Status: server_structs.RespFailed, diff --git a/director/migrations/20241017135850_create_db_tables.sql b/director/migrations/20241017135850_create_db_tables.sql index 5915f84d9..1460aac84 100644 --- a/director/migrations/20241017135850_create_db_tables.sql +++ b/director/migrations/20241017135850_create_db_tables.sql @@ -1,6 +1,6 @@ -- +goose Up -- +goose StatementBegin -CREATE TABLE server_status ( +CREATE TABLE server_downtimes ( uuid TEXT PRIMARY KEY, name TEXT NOT NULL UNIQUE, filter_type TEXT NOT NULL, diff --git a/launchers/director_serve.go b/launchers/director_serve.go index 06baae90c..4fde00fc8 100644 --- a/launchers/director_serve.go +++ b/launchers/director_serve.go @@ -39,6 +39,9 @@ func DirectorServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group log.Info("Initializing Director GeoIP database...") director.InitializeGeoIPDB(ctx) + if err := director.InitializeDB(); err != nil { + return errors.Wrap(err, "failed to initialize director sqlite database") + } director.ConfigFilterdServers() director.LaunchTTLCache(ctx, egrp) @@ -75,9 +78,6 @@ func DirectorServe(ctx context.Context, engine *gin.Engine, egrp *errgroup.Group return errors.Wrap(err, "invalid URL for Director.SupportContactUrl") } } - if err := director.InitializeDB(); err != nil { - return errors.Wrap(err, "failed to initialize director sqlite database") - } rootGroup := engine.Group("/") director.RegisterDirectorOIDCAPI(rootGroup) director.RegisterDirectorWebAPI(rootGroup) From 7230811434b6df442b7cf4571f81e11af208326c Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Tue, 22 Oct 2024 17:15:48 +0000 Subject: [PATCH 04/12] fix linting issues and set director dblocation in test --- director/director_ui.go | 16 ++++++++++++---- fed_test_utils/fed.go | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/director/director_ui.go b/director/director_ui.go index 9cc6d2746..b1fb0d135 100644 --- a/director/director_ui.go +++ b/director/director_ui.go @@ -251,10 +251,14 @@ func handleFilterServer(ctx *gin.Context) { // If we previously temporarily allowed a server, we switch to permFiltered (reset) if filterType == tempAllowed { filteredServers[sn] = permFiltered - SetServerDowntime(sn, permFiltered) + if err := SetServerDowntime(sn, permFiltered); err != nil { + log.Errorln("Failed to persist the downtime in director db", err) + } } else { filteredServers[sn] = tempFiltered - SetServerDowntime(sn, tempFiltered) + if err := SetServerDowntime(sn, tempFiltered); err != nil { + log.Errorln("Failed to persist the downtime in director db", err) + } } ctx.JSON(http.StatusOK, server_structs.SimpleApiResp{Status: server_structs.RespOK, Msg: "success"}) } @@ -286,11 +290,15 @@ func handleAllowServer(ctx *gin.Context) { if ft == tempFiltered { // For temporarily filtered server, allowing them by removing the server from the map delete(filteredServers, sn) - DeleteServerDowntime(sn) + if err := DeleteServerDowntime(sn); err != nil { + log.Errorf("Failed to remove the downtime of server %s in director db", sn) + } } else if ft == permFiltered { // For servers to filter from the config, temporarily allow the server filteredServers[sn] = tempAllowed - SetServerDowntime(sn, tempAllowed) + if err := SetServerDowntime(sn, tempAllowed); err != nil { + log.Errorf("Failed to persist the status change of server %s in director db", sn) + } } else if ft == topoFiltered { ctx.JSON(http.StatusBadRequest, server_structs.SimpleApiResp{ Status: server_structs.RespFailed, diff --git a/fed_test_utils/fed.go b/fed_test_utils/fed.go index 2dc26416b..9b63b8d6d 100644 --- a/fed_test_utils/fed.go +++ b/fed_test_utils/fed.go @@ -147,6 +147,7 @@ func NewFedTest(t *testing.T, originConfig string) (ft *FedTest) { viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) viper.Set("Director.CacheSortMethod", "distance") + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) err = config.InitServer(ctx, modules) require.NoError(t, err) From 6253842439d7c8d8b4ec958d72e78c778840c987 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Tue, 22 Oct 2024 19:41:22 +0000 Subject: [PATCH 05/12] fix remaining linting issues and director.DbLocation setting problem --- cmd/fed_serve_cache_test.go | 1 + cmd/fed_serve_test.go | 2 ++ director/director_db.go | 5 +++-- director/director_db_test.go | 3 ++- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cmd/fed_serve_cache_test.go b/cmd/fed_serve_cache_test.go index 22aa46d10..872ecc5ff 100644 --- a/cmd/fed_serve_cache_test.go +++ b/cmd/fed_serve_cache_test.go @@ -109,6 +109,7 @@ func TestFedServeCache(t *testing.T) { viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) viper.Set("Origin.EnablePublicReads", false) + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) require.NoError(t, err) diff --git a/cmd/fed_serve_test.go b/cmd/fed_serve_test.go index a5f39c646..566c19d85 100644 --- a/cmd/fed_serve_test.go +++ b/cmd/fed_serve_test.go @@ -93,6 +93,8 @@ func TestFedServePosixOrigin(t *testing.T) { viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) + defer cancel() _, fedCancel, err := launchers.LaunchModules(ctx, modules) diff --git a/director/director_db.go b/director/director_db.go index 9a4c93226..fd496b9d3 100644 --- a/director/director_db.go +++ b/director/director_db.go @@ -22,11 +22,12 @@ import ( "time" "github.com/google/uuid" - "github.com/pelicanplatform/pelican/param" - "github.com/pelicanplatform/pelican/server_utils" "github.com/pkg/errors" "gorm.io/gorm" "gorm.io/gorm/logger" + + "github.com/pelicanplatform/pelican/param" + "github.com/pelicanplatform/pelican/server_utils" ) type ServerDowntime struct { diff --git a/director/director_db_test.go b/director/director_db_test.go index d4f9e8ff9..aca71dace 100644 --- a/director/director_db_test.go +++ b/director/director_db_test.go @@ -5,10 +5,11 @@ import ( "github.com/glebarez/sqlite" "github.com/google/uuid" - "github.com/pelicanplatform/pelican/server_utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gorm.io/gorm" + + "github.com/pelicanplatform/pelican/server_utils" ) var ( From baceaa927246e07f2760e5a382821d5e5180b57a Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Tue, 22 Oct 2024 19:56:25 +0000 Subject: [PATCH 06/12] add Director.DbLocation in plugin test --- cmd/plugin_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/plugin_test.go b/cmd/plugin_test.go index 52a9943af..e7d95b520 100644 --- a/cmd/plugin_test.go +++ b/cmd/plugin_test.go @@ -189,6 +189,7 @@ func (f *FedTest) Spinup() { viper.Set("Origin.Port", 0) viper.Set("Server.WebPort", 0) viper.Set("Origin.RunLocation", tmpPath) + viper.Set("Director.DbLocation", filepath.Join(f.T.TempDir(), "ns-director.sqlite")) err = config.InitServer(ctx, modules) require.NoError(f.T, err) From b7ce364657a294b99747349226f630dee8170a24 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Tue, 22 Oct 2024 20:39:01 +0000 Subject: [PATCH 07/12] add Director.DbLocation to github scripts --- github_scripts/get_put_test.sh | 3 ++- github_scripts/stat_test.sh | 4 +++- github_scripts/x509_test.sh | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/github_scripts/get_put_test.sh b/github_scripts/get_put_test.sh index 873646783..1db9c7a72 100755 --- a/github_scripts/get_put_test.sh +++ b/github_scripts/get_put_test.sh @@ -34,7 +34,8 @@ export PELICAN_ORIGIN_ENABLEDIRECTREADS=true export PELICAN_SERVER_ENABLEUI=false export PELICAN_ORIGIN_RUNLOCATION=$PWD/xrootdRunLocation export PELICAN_CONFIGDIR=$PWD/get_put_tmp/config -export PELICAN_REGISTRY_DBLOCATION=$PWD/get_put_tmp/config/test.sql +export PELICAN_REGISTRY_DBLOCATION=$PWD/get_put_tmp/config/test-registry.sql +export PELICAN_DIRECTOR_DBLOCATION=$PWD/get_put_tmp/config/test-director.sql export PELICAN_OIDC_CLIENTID="sometexthere" export PELICAN_ORIGIN_FEDERATIONPREFIX="/test" export PELICAN_ORIGIN_STORAGEPREFIX="$PWD/get_put_tmp/origin" diff --git a/github_scripts/stat_test.sh b/github_scripts/stat_test.sh index d1e0ab8e5..ea7ec67be 100755 --- a/github_scripts/stat_test.sh +++ b/github_scripts/stat_test.sh @@ -32,7 +32,8 @@ export PELICAN_SERVER_ENABLEUI=false export PELICAN_ORIGIN_RUNLOCATION=/tmp/pelican-test/stat_test/xrootdRunLocation export PELICAN_CONFIGDIR=/tmp/pelican-test/stat_test -export PELICAN_REGISTRY_DBLOCATION=/tmp/pelican-test/stat_test/test.sql +export PELICAN_REGISTRY_DBLOCATION=/tmp/pelican-test/stat_test/test-registry.sql +export PELICAN_DIRECTOR_DBLOCATION=/tmp/pelican-test/stat_test/test-director.sql export PELICAN_OIDC_CLIENTID="sometexthere" export PELICAN_OIDC_CLIENTSECRETFILE=/tmp/pelican-test/stat_test/oidc-secret echo "Placeholder OIDC secret" > /tmp/pelican-test/stat_test/oidc-secret @@ -63,6 +64,7 @@ cleanup() { unset PELICAN_FEDERATION_REGISTRYURL unset PELICAN_TLSSKIPVERIFY unset PELICAN_REGISTRY_DBLOCATION + unset PELICAN_DIRECTOR_DBLOCATION unset PELICAN_SERVER_ENABLEUI unset PELICAN_OIDC_CLIENTID unset PELICAN_OIDC_CLIENTSECRETFILE diff --git a/github_scripts/x509_test.sh b/github_scripts/x509_test.sh index ebe533672..f0b5ff18a 100755 --- a/github_scripts/x509_test.sh +++ b/github_scripts/x509_test.sh @@ -38,7 +38,8 @@ export PELICAN_SERVER_ENABLEUI=false export PELICAN_ORIGIN_RUNLOCATION=$PWD/xrootdRunLocation export PELICAN_CACHE_RUNLOCATION=$PWD/xrootdCacheRunLocation export PELICAN_CONFIGDIR=$PWD/x509/config -export PELICAN_REGISTRY_DBLOCATION=$PWD/x509/config/test.sql +export PELICAN_REGISTRY_DBLOCATION=$PWD/x509/config/test-registry.sql +export PELICAN_DIRECTOR_DBLOCATION=$PWD/x509/config/test-director.sql export PELICAN_OIDC_CLIENTID="sometexthere" export PELICAN_ORIGIN_EXPORTVOLUMES="$PWD/x509/origin:/test $PWD/x509/defer:/defer/" export PELICAN_DIRECTOR_X509CLIENTAUTHENTICATIONPREFIXES="/defer" From c28454fa513aff4146f9d709e42a549de7ec4256 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Fri, 25 Oct 2024 20:56:52 +0000 Subject: [PATCH 08/12] set default director db location --- config/config.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/config.go b/config/config.go index 5314e12b3..4bd708dbb 100644 --- a/config/config.go +++ b/config/config.go @@ -1030,6 +1030,7 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e viper.SetDefault("Origin.Multiuser", true) viper.SetDefault(param.Origin_DbLocation.GetName(), "/var/lib/pelican/origin.sqlite") + viper.SetDefault(param.Director_DbLocation.GetName(), "/var/lib/pelican/director.sqlite") viper.SetDefault("Director.GeoIPLocation", "/var/cache/pelican/maxmind/GeoLite2-City.mmdb") viper.SetDefault("Registry.DbLocation", "/var/lib/pelican/registry.sqlite") // The lotman db will actually take this path and create the lot at /path/.lot/lotman_cpp.sqlite @@ -1040,6 +1041,7 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e viper.SetDefault(param.Origin_GlobusConfigLocation.GetName(), filepath.Join("/run", "pelican", "xrootd", "origin", "globus")) } else { viper.SetDefault(param.Origin_DbLocation.GetName(), filepath.Join(configDir, "origin.sqlite")) + viper.SetDefault(param.Director_DbLocation.GetName(), filepath.Join(configDir, "ns-director.sqlite")) viper.SetDefault("Director.GeoIPLocation", filepath.Join(configDir, "maxmind", "GeoLite2-City.mmdb")) viper.SetDefault("Registry.DbLocation", filepath.Join(configDir, "ns-registry.sqlite")) // Lotdb will live at /.lot/lotman_cpp.sqlite From 1ace5bee22ae7f8aedddb2b6eee6f97527bf8274 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Fri, 25 Oct 2024 21:01:05 +0000 Subject: [PATCH 09/12] cleanup inappropriate file name: ns-director.sqlite --- cmd/fed_serve_cache_test.go | 2 +- cmd/fed_serve_test.go | 2 +- cmd/plugin_test.go | 2 +- config/config.go | 2 +- fed_test_utils/fed.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/fed_serve_cache_test.go b/cmd/fed_serve_cache_test.go index 872ecc5ff..22a7b29c5 100644 --- a/cmd/fed_serve_cache_test.go +++ b/cmd/fed_serve_cache_test.go @@ -109,7 +109,7 @@ func TestFedServeCache(t *testing.T) { viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) viper.Set("Origin.EnablePublicReads", false) - viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "director.sqlite")) require.NoError(t, err) diff --git a/cmd/fed_serve_test.go b/cmd/fed_serve_test.go index 566c19d85..e27f91e4f 100644 --- a/cmd/fed_serve_test.go +++ b/cmd/fed_serve_test.go @@ -93,7 +93,7 @@ func TestFedServePosixOrigin(t *testing.T) { viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) - viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "director.sqlite")) defer cancel() diff --git a/cmd/plugin_test.go b/cmd/plugin_test.go index e7d95b520..00d034e9c 100644 --- a/cmd/plugin_test.go +++ b/cmd/plugin_test.go @@ -189,7 +189,7 @@ func (f *FedTest) Spinup() { viper.Set("Origin.Port", 0) viper.Set("Server.WebPort", 0) viper.Set("Origin.RunLocation", tmpPath) - viper.Set("Director.DbLocation", filepath.Join(f.T.TempDir(), "ns-director.sqlite")) + viper.Set("Director.DbLocation", filepath.Join(f.T.TempDir(), "director.sqlite")) err = config.InitServer(ctx, modules) require.NoError(f.T, err) diff --git a/config/config.go b/config/config.go index 4bd708dbb..255131041 100644 --- a/config/config.go +++ b/config/config.go @@ -1041,7 +1041,7 @@ func InitServer(ctx context.Context, currentServers server_structs.ServerType) e viper.SetDefault(param.Origin_GlobusConfigLocation.GetName(), filepath.Join("/run", "pelican", "xrootd", "origin", "globus")) } else { viper.SetDefault(param.Origin_DbLocation.GetName(), filepath.Join(configDir, "origin.sqlite")) - viper.SetDefault(param.Director_DbLocation.GetName(), filepath.Join(configDir, "ns-director.sqlite")) + viper.SetDefault(param.Director_DbLocation.GetName(), filepath.Join(configDir, "director.sqlite")) viper.SetDefault("Director.GeoIPLocation", filepath.Join(configDir, "maxmind", "GeoLite2-City.mmdb")) viper.SetDefault("Registry.DbLocation", filepath.Join(configDir, "ns-registry.sqlite")) // Lotdb will live at /.lot/lotman_cpp.sqlite diff --git a/fed_test_utils/fed.go b/fed_test_utils/fed.go index 9b63b8d6d..300ffe27d 100644 --- a/fed_test_utils/fed.go +++ b/fed_test_utils/fed.go @@ -147,7 +147,7 @@ func NewFedTest(t *testing.T, originConfig string) (ft *FedTest) { viper.Set("Registry.RequireOriginApproval", false) viper.Set("Registry.RequireCacheApproval", false) viper.Set("Director.CacheSortMethod", "distance") - viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "ns-director.sqlite")) + viper.Set("Director.DbLocation", filepath.Join(t.TempDir(), "director.sqlite")) err = config.InitServer(ctx, modules) require.NoError(t, err) From 7117f8d83df3acc25ee909ea7cefb1ec26fb42cb Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Fri, 8 Nov 2024 22:09:00 +0000 Subject: [PATCH 10/12] var names, err msgs - address Justin's reviews --- director/director_api.go | 12 +++--- director/director_db.go | 74 ++++++++++++++++++++++++------------ director/director_db_test.go | 24 ++++++------ docs/parameters.yaml | 2 +- 4 files changed, 68 insertions(+), 44 deletions(-) diff --git a/director/director_api.go b/director/director_api.go index d20e8a2ac..c85e3da0c 100644 --- a/director/director_api.go +++ b/director/director_api.go @@ -66,7 +66,7 @@ func listAdvertisement(serverTypes []server_structs.ServerType) []*server_struct func checkFilter(serverName string) (bool, filterType) { filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() - log.Debugln("Checking the filter applied on server", serverName, "Current filteredServers map:", filteredServers) + log.Debugf("Checking for a downtime filter applied to server %s", serverName) status, exists := filteredServers[serverName] // No filter entry if !exists { @@ -232,19 +232,19 @@ func ConfigFilterdServers() { for _, sn := range param.Director_FilteredServers.GetStringSlice() { filteredServers[sn] = permFiltered } - log.Debugln("Loaded filtered servers config from Director.FilteredServers param", filteredServers) + log.Debugln("Loaded server downtime configuration from the Director.FilteredServers parameter:", filteredServers) } - if param.Director_DbLocation.IsSet() { - persistedServerDowntimes, err := GetAllServerDowntimes() + if param.Director_DbLocation.GetString() != "" { + persistedServerDowntimes, err := getAllServerDowntimes() if err != nil { - log.Error("Failed to read persisted ServerDowntimes from director db", err) + log.Error("Failed to read persisted server downtimes from director db", err) return } for _, serverDowntime := range persistedServerDowntimes { filteredServers[serverDowntime.Name] = serverDowntime.FilterType } - log.Debugln("Loaded filtered servers config from director db's server_downtimes table, the final filteredServers var has the following value", filteredServers) + log.Debugln("Loaded filtered servers config from director db:", filteredServers) // if a filtered server config rule is set in both Director.FilteredServers param and director db, the latter one will eventually be used } } diff --git a/director/director_db.go b/director/director_db.go index fd496b9d3..47a22ce69 100644 --- a/director/director_db.go +++ b/director/director_db.go @@ -44,90 +44,114 @@ var db *gorm.DB //go:embed migrations/*.sql var embedMigrations embed.FS +// Initialize the Director's sqlite database, which is used to persist information about server downtimes func InitializeDB() error { dbPath := param.Director_DbLocation.GetString() tdb, err := server_utils.InitSQLiteDB(dbPath) if err != nil { - return err + return errors.Wrap(err, "failed to initialize the Director's sqlite database") } db = tdb sqldb, err := db.DB() if err != nil { - return errors.Wrapf(err, "Failed to get sql.DB from gorm DB: %s", dbPath) + return errors.Wrapf(err, "failed to get sql.DB from gorm DB: %s", dbPath) } // Run database migrations if err := server_utils.MigrateDB(sqldb, embedMigrations); err != nil { - return err + return errors.Wrap(err, "failed to migrate the Director's sqlite database using embedded migration files") } return nil } -func ShutdownDirectorDB() error { +// Shut down the Director's sqlite database +func shutdownDirectorDB() error { return server_utils.ShutdownDB(db) } -func CreateServerDowntime(name string, filterType filterType) error { +// Create a new db entry representing the downtime info of a server +func createServerDowntime(serverName string, filterType filterType) error { id, err := uuid.NewV7() if err != nil { - return errors.Wrap(err, "Unable to create new UUID for new entry in server status table") + return errors.Wrap(err, "unable to create new UUID for new entry in server status table") } serverDowntime := ServerDowntime{ UUID: id.String(), - Name: name, + Name: serverName, FilterType: filterType, CreatedAt: time.Now(), UpdatedAt: time.Now(), } if err := db.Create(serverDowntime).Error; err != nil { - return err + return errors.Wrap(err, "unable to create server downtime table") } return nil } -func GetServerDowntime(name string) (filterType, error) { +// Retrieve the downtime info of a given server (filter applied to the server) +func getServerDowntime(serverName string) (filterType, error) { var serverDowntime ServerDowntime - err := db.First(&serverDowntime, "name = ?", name).Error + err := db.First(&serverDowntime, "name = ?", serverName).Error if err != nil { - return "", err + if errors.Is(err, gorm.ErrRecordNotFound) { + return "", errors.Wrapf(err, "%s is not found in the Director db", serverName) + } + return "", errors.Wrapf(err, "unable to get the downtime of %s", serverName) } return filterType(serverDowntime.FilterType), nil } -func GetAllServerDowntimes() ([]ServerDowntime, error) { +// Retrieve the downtime info of all servers saved in the Director's sqlite database +func getAllServerDowntimes() ([]ServerDowntime, error) { var statuses []ServerDowntime result := db.Find(&statuses) if result.Error != nil { - return nil, result.Error + return nil, errors.Wrap(result.Error, "unable to get the downtime of all servers") } return statuses, nil } -// Set filterType of a given server. If the server doesn't exist in director db, create a new entry for it -func SetServerDowntime(name string, filterType filterType) error { +// Set the downtime info (filterType) of a given server +func setServerDowntime(serverName string, filterType filterType) error { var serverDowntime ServerDowntime - // slience the logger for this query because there's definitely an ErrRecordNotFound when a new downtime info inserted - err := db.Session(&gorm.Session{Logger: db.Logger.LogMode(logger.Silent)}).First(&serverDowntime, "name = ?", name).Error + // slience the logger for this query because there's definitely an ErrRecordNotFound when a new downtime info entry inserted + err := db.Session(&gorm.Session{Logger: db.Logger.LogMode(logger.Silent)}).First(&serverDowntime, "name = ?", serverName).Error - if errors.Is(err, gorm.ErrRecordNotFound) { - return CreateServerDowntime(name, filterType) - } else if err != nil { - return errors.Wrap(err, "Error retrieving Server Status") + // If the server doesn't exist in director db, create a new entry for it + if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return createServerDowntime(serverName, filterType) + } + + return errors.Wrapf(err, "unable to retrieve downtime status for server %s", serverName) } serverDowntime.FilterType = filterType serverDowntime.UpdatedAt = time.Now() if err := db.Save(&serverDowntime).Error; err != nil { - return err + return errors.Wrap(err, "unable to update") } return nil } -func DeleteServerDowntime(name string) error { - if err := db.Where("name = ?", name).Delete(&ServerDowntime{}).Error; err != nil { - return errors.Wrap(err, "Failed to delete an entry in Server Status table") +// Define a function type for setServerDowntime +type setServerDowntimeFunc func(string, filterType) error + +// Make the function a variable so it can be mocked in tests +var setServerDowntimeFn setServerDowntimeFunc = setServerDowntime + +// Delete the downtime info of a given server from the Director's sqlite database +func deleteServerDowntime(serverName string) error { + if err := db.Where("name = ?", serverName).Delete(&ServerDowntime{}).Error; err != nil { + return errors.Wrap(err, "failed to delete an entry in Server Status table") } return nil } + +// Define a function type for deleteServerDowntime +type deleteServerDowntimeFunc func(string) error + +// Make the function a variable so it can be mocked in tests +var deleteServerDowntimeFn deleteServerDowntimeFunc = deleteServerDowntime diff --git a/director/director_db_test.go b/director/director_db_test.go index aca71dace..094ce59e8 100644 --- a/director/director_db_test.go +++ b/director/director_db_test.go @@ -20,7 +20,7 @@ var ( } ) -func setupMockDirectorDB(t *testing.T) { +func SetupMockDirectorDB(t *testing.T) { mockDB, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) db = mockDB require.NoError(t, err, "Error setting up mock origin DB") @@ -28,8 +28,8 @@ func setupMockDirectorDB(t *testing.T) { require.NoError(t, err, "Failed to migrate DB for Globus table") } -func teardownMockDirectorDB(t *testing.T) { - err := ShutdownDirectorDB() +func TeardownMockDirectorDB(t *testing.T) { + err := shutdownDirectorDB() require.NoError(t, err, "Error tearing down mock director DB") } @@ -39,44 +39,44 @@ func insertMockDBData(ss []ServerDowntime) error { func TestDirectorDBBasics(t *testing.T) { server_utils.ResetTestState() - setupMockDirectorDB(t) + SetupMockDirectorDB(t) t.Cleanup(func() { - teardownMockDirectorDB(t) + TeardownMockDirectorDB(t) }) err := insertMockDBData(mockSS) require.NoError(t, err) t.Run("get-downtime", func(t *testing.T) { - filterType, err := GetServerDowntime(mockSS[1].Name) + filterType, err := getServerDowntime(mockSS[1].Name) assert.Equal(t, filterType, permFiltered) require.NoError(t, err) }) t.Run("get-all-downtime", func(t *testing.T) { - statuses, err := GetAllServerDowntimes() + statuses, err := getAllServerDowntimes() require.NoError(t, err) assert.Len(t, statuses, len(mockSS)) }) t.Run("set-downtime", func(t *testing.T) { - err = SetServerDowntime(mockSS[1].Name, tempAllowed) + err = setServerDowntime(mockSS[1].Name, tempAllowed) require.NoError(t, err) - filterType, err := GetServerDowntime(mockSS[1].Name) + filterType, err := getServerDowntime(mockSS[1].Name) assert.Equal(t, filterType, tempAllowed) require.NoError(t, err) }) t.Run("duplicate-name-insert", func(t *testing.T) { - err := CreateServerDowntime(mockSS[1].Name, tempAllowed) + err := createServerDowntime(mockSS[1].Name, tempAllowed) require.Error(t, err) assert.Contains(t, err.Error(), "UNIQUE constraint failed") }) t.Run("delete-downtime-entry-from-directory-db", func(t *testing.T) { - err = DeleteServerDowntime(mockSS[0].Name) + err = deleteServerDowntime(mockSS[0].Name) require.NoError(t, err, "Error deleting server status") - _, err = GetServerDowntime(mockSS[0].Name) + _, err = getServerDowntime(mockSS[0].Name) assert.Error(t, err, "Expected error retrieving deleted server status") }) } diff --git a/docs/parameters.yaml b/docs/parameters.yaml index 327fb7eef..f38f52f4e 100644 --- a/docs/parameters.yaml +++ b/docs/parameters.yaml @@ -1245,7 +1245,7 @@ components: ["cache"] ############################ name: Director.DbLocation description: |+ - A filepath to the intended location of the director's database. + A filepath to the intended location of the director's database, where server downtime info is stored. type: filename root_default: /var/lib/pelican/director.sqlite default: $ConfigBase/director.sqlite From e83cdd885f82f6ca0a55bbe3813c128bd3ef2e79 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Fri, 8 Nov 2024 22:11:39 +0000 Subject: [PATCH 11/12] data integrity, more robust unit tests - address BrianB's reviews --- director/director_test.go | 230 ++++++++++++++++++++++++++++++++++++++ director/director_ui.go | 72 +++++++++--- 2 files changed, 288 insertions(+), 14 deletions(-) diff --git a/director/director_test.go b/director/director_test.go index 33b86ee4d..48350e0c5 100644 --- a/director/director_test.go +++ b/director/director_test.go @@ -1734,7 +1734,9 @@ func TestHandleFilterServer(t *testing.T) { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() filteredServers = map[string]filterType{} + TeardownMockDirectorDB(t) }) + SetupMockDirectorDB(t) router := gin.Default() router.GET("/servers/filter/*name", handleFilterServer) @@ -1750,26 +1752,43 @@ func TestHandleFilterServer(t *testing.T) { // Check the response require.Equal(t, 200, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, tempFiltered, filteredServers["mock-dne"]) + + // Check the Director database + filterType, err := getServerDowntime("mock-dne") + assert.Equal(t, tempFiltered, filterType) + require.NoError(t, err) }) t.Run("filter-server-w-permFiltered", func(t *testing.T) { // Create a request to the endpoint w := httptest.NewRecorder() req, _ := http.NewRequest("GET", "/servers/filter/mock-pf", nil) + + // Tweak the downtime status (filter type) to permFiltered filteredServersMutex.Lock() filteredServers["mock-pf"] = permFiltered filteredServersMutex.Unlock() + err := setServerDowntime("mock-pf", permFiltered) + require.NoError(t, err) + router.ServeHTTP(w, req) // Check the response require.Equal(t, 400, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, permFiltered, filteredServers["mock-pf"]) + // Check the Director database + filterType, err := getServerDowntime("mock-pf") + assert.Equal(t, permFiltered, filterType) + require.NoError(t, err) + resB, err := io.ReadAll(w.Body) require.NoError(t, err) assert.Contains(t, string(resB), "Can't filter a server that already has been fitlered") @@ -1781,15 +1800,24 @@ func TestHandleFilterServer(t *testing.T) { filteredServersMutex.Lock() filteredServers["mock-tf"] = tempFiltered filteredServersMutex.Unlock() + err := setServerDowntime("mock-tf", tempFiltered) + require.NoError(t, err) + router.ServeHTTP(w, req) // Check the response require.Equal(t, 400, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, tempFiltered, filteredServers["mock-tf"]) + // Check the Director database + filterType, err := getServerDowntime("mock-tf") + assert.Equal(t, tempFiltered, filterType) + require.NoError(t, err) + resB, err := io.ReadAll(w.Body) require.NoError(t, err) assert.Contains(t, string(resB), "Can't filter a server that already has been fitlered") @@ -1806,9 +1834,15 @@ func TestHandleFilterServer(t *testing.T) { // Check the response require.Equal(t, 200, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, permFiltered, filteredServers["mock-ta"]) + + // Check the Director database + filterType, err := getServerDowntime("mock-ta") + assert.Equal(t, permFiltered, filterType) + require.NoError(t, err) }) t.Run("filter-with-invalid-name", func(t *testing.T) { // Create a request to the endpoint @@ -1824,12 +1858,61 @@ func TestHandleFilterServer(t *testing.T) { }) } +func TestHandleFilterServerDataIntegrity(t *testing.T) { + t.Cleanup(func() { + filteredServersMutex.Lock() + defer filteredServersMutex.Unlock() + filteredServers = map[string]filterType{} + TeardownMockDirectorDB(t) + }) + SetupMockDirectorDB(t) + + router := gin.Default() + router.GET("/servers/filter/*name", handleFilterServer) + + t.Run("db-error-when-setting-downtime", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/servers/filter/mock-error", nil) + + // Set up original filter type + filteredServersMutex.Lock() + filteredServers["mock-error"] = tempAllowed + filteredServersMutex.Unlock() + + // Mock setServerDowntime to return error + origSetServerDowntime := setServerDowntime + defer func() { setServerDowntimeFn = origSetServerDowntime }() + setServerDowntimeFn = func(serverName string, ft filterType) error { + return fmt.Errorf("mock db error") + } + + router.ServeHTTP(w, req) + + // Check response code + require.Equal(t, http.StatusInternalServerError, w.Code) + + // Verify the server was reverted to original filter type + filteredServersMutex.RLock() + actualType, exists := filteredServers["mock-error"] + filteredServersMutex.RUnlock() + assert.True(t, exists, "Server should exist in filteredServers") + assert.Equal(t, tempAllowed, actualType, "Filter type should be reverted to original value") + + // Check error message + resB, err := io.ReadAll(w.Body) + require.NoError(t, err) + assert.Contains(t, string(resB), "Failed to persist server downtime due to database error") + }) +} + func TestHandleAllowServer(t *testing.T) { t.Cleanup(func() { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() filteredServers = map[string]filterType{} + TeardownMockDirectorDB(t) }) + SetupMockDirectorDB(t) router := gin.Default() router.GET("/servers/allow/*name", handleAllowServer) @@ -1837,9 +1920,15 @@ func TestHandleAllowServer(t *testing.T) { // Create a request to the endpoint w := httptest.NewRecorder() req, _ := http.NewRequest("GET", "/servers/allow/mock-dne", nil) + + // Server is not in the in-memory downtime map filteredServersMutex.Lock() delete(filteredServers, "mock-dne") filteredServersMutex.Unlock() + // Server is also not in the Director db downtime table + err := deleteServerDowntime("mock-dne") + require.NoError(t, err) + // Note: Both map deletion and db deletion do not trigger an error if there’s nothing to delete router.ServeHTTP(w, req) // Check the response @@ -1860,9 +1949,15 @@ func TestHandleAllowServer(t *testing.T) { // Check the response require.Equal(t, 200, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, tempAllowed, filteredServers["mock-pf"]) + + // Check the Director database + filterType, err := getServerDowntime("mock-pf") + assert.Equal(t, tempAllowed, filterType) + require.NoError(t, err) }) t.Run("allow-server-w-tempFiltered", func(t *testing.T) { // Create a request to the endpoint @@ -1876,9 +1971,15 @@ func TestHandleAllowServer(t *testing.T) { // Check the response require.Equal(t, 200, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Empty(t, filteredServers["mock-tf"]) + + // Check the Director database + filterType, err := getServerDowntime("mock-tf") + assert.Equal(t, "", string(filterType)) + assert.Contains(t, err.Error(), "is not found in the Director db") }) t.Run("allow-server-w-tempAllowed", func(t *testing.T) { // Create a request to the endpoint @@ -1887,11 +1988,15 @@ func TestHandleAllowServer(t *testing.T) { filteredServersMutex.Lock() filteredServers["mock-ta"] = tempAllowed filteredServersMutex.Unlock() + err := setServerDowntime("mock-ta", tempAllowed) + require.NoError(t, err) + router.ServeHTTP(w, req) // Check the response require.Equal(t, 400, w.Code) + // Check the in-memory cache storage filteredServersMutex.RLock() defer filteredServersMutex.RUnlock() assert.Equal(t, tempAllowed, filteredServers["mock-ta"]) @@ -1899,6 +2004,11 @@ func TestHandleAllowServer(t *testing.T) { resB, err := io.ReadAll(w.Body) require.NoError(t, err) assert.Contains(t, string(resB), "Can't allow server mock-ta that is not being filtered") + + // Check the Director database + filterType, err := getServerDowntime("mock-ta") + assert.Equal(t, tempAllowed, filterType) + require.NoError(t, err) }) t.Run("allow-with-invalid-name", func(t *testing.T) { // Create a request to the endpoint @@ -1914,6 +2024,126 @@ func TestHandleAllowServer(t *testing.T) { }) } +func TestHandleAllowServerDataIntegrity(t *testing.T) { + t.Cleanup(func() { + filteredServersMutex.Lock() + defer filteredServersMutex.Unlock() + filteredServers = map[string]filterType{} + TeardownMockDirectorDB(t) + }) + SetupMockDirectorDB(t) + + router := gin.Default() + router.GET("/servers/allow/*name", handleAllowServer) + + // Sub-test 1: When server is permanently filtered + t.Run("permFiltered-db-error", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/servers/allow/mock-error", nil) + + // Set up initial state as permFiltered + filteredServersMutex.Lock() + filteredServers["mock-error"] = permFiltered + filteredServersMutex.Unlock() + + // Mock setServerDowntimeFn to return error + origSetServerDowntime := setServerDowntimeFn + defer func() { setServerDowntimeFn = origSetServerDowntime }() + setServerDowntimeFn = func(serverName string, ft filterType) error { + return fmt.Errorf("mock db error") + } + + router.ServeHTTP(w, req) + + // Check response code + require.Equal(t, http.StatusInternalServerError, w.Code) + + // Verify the server maintained original filter type + filteredServersMutex.RLock() + actualType, exists := filteredServers["mock-error"] + filteredServersMutex.RUnlock() + assert.True(t, exists, "Server should still exist in filteredServers") + assert.Equal(t, permFiltered, actualType, "Filter type should remain as permFiltered") + + // Check error message + resB, err := io.ReadAll(w.Body) + require.NoError(t, err) + assert.Contains(t, string(resB), "Failed to remove the downtime of server mock-error in director db") + }) + + // Sub-test 2: When server is temporarily filtered and deletion fails + t.Run("tempFiltered-deletion-error", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/servers/allow/mock-error", nil) + + // Set up initial state as tempFiltered + filteredServersMutex.Lock() + filteredServers["mock-error"] = tempFiltered + filteredServersMutex.Unlock() + + // Mock deleteServerDowntime to return error + origDeleteServerDowntime := deleteServerDowntimeFn + defer func() { deleteServerDowntimeFn = origDeleteServerDowntime }() + deleteServerDowntimeFn = func(serverName string) error { + return fmt.Errorf("mock deletion error") + } + + router.ServeHTTP(w, req) + + // Check response code + require.Equal(t, http.StatusInternalServerError, w.Code) + + // Verify the server maintained original filter type + filteredServersMutex.RLock() + actualType, exists := filteredServers["mock-error"] + filteredServersMutex.RUnlock() + assert.True(t, exists, "Server should still exist in filteredServers") + assert.Equal(t, tempFiltered, actualType, "Filter type should remain as tempFiltered") + + // Check error message + resB, err := io.ReadAll(w.Body) + require.NoError(t, err) + assert.Contains(t, string(resB), "Failed to remove the downtime of server mock-error in director db") + }) + + // Sub-test 3: When server is already tempAllowed and db error occurs + t.Run("tempAllowed-db-error", func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/servers/allow/mock-error", nil) + + // Set up initial state as tempAllowed + filteredServersMutex.Lock() + filteredServers["mock-error"] = tempAllowed + filteredServersMutex.Unlock() + err := setServerDowntime("mock-error", tempAllowed) + require.NoError(t, err) + + // Mock setServerDowntimeFn to return error + origSetServerDowntime := setServerDowntimeFn + defer func() { setServerDowntimeFn = origSetServerDowntime }() + setServerDowntimeFn = func(serverName string, ft filterType) error { + return fmt.Errorf("mock db error") + } + + router.ServeHTTP(w, req) + + // Should return 400 as the server is already tempAllowed + require.Equal(t, http.StatusBadRequest, w.Code) + + // Verify the server maintained original filter type + filteredServersMutex.RLock() + actualType, exists := filteredServers["mock-error"] + filteredServersMutex.RUnlock() + assert.True(t, exists, "Server should still exist in filteredServers") + assert.Equal(t, tempAllowed, actualType, "Filter type should remain as tempAllowed") + + // Check error message + resB, err := io.ReadAll(w.Body) + require.NoError(t, err) + assert.Contains(t, string(resB), "Can't allow server") + }) +} + func TestGetRedirectUrl(t *testing.T) { adFromTopo := server_structs.ServerAd{ URL: url.URL{ diff --git a/director/director_ui.go b/director/director_ui.go index b1fb0d135..2930a217a 100644 --- a/director/director_ui.go +++ b/director/director_ui.go @@ -248,18 +248,33 @@ func handleFilterServer(ctx *gin.Context) { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() + // Backup the original filter type to revert in case of failure + originalFilterType, hasOriginalFilter := filteredServers[sn] + + // Decide new filter type and update map // If we previously temporarily allowed a server, we switch to permFiltered (reset) + newFilterType := tempFiltered if filterType == tempAllowed { - filteredServers[sn] = permFiltered - if err := SetServerDowntime(sn, permFiltered); err != nil { - log.Errorln("Failed to persist the downtime in director db", err) - } - } else { - filteredServers[sn] = tempFiltered - if err := SetServerDowntime(sn, tempFiltered); err != nil { - log.Errorln("Failed to persist the downtime in director db", err) + newFilterType = permFiltered + } + filteredServers[sn] = newFilterType + + // Attempt to persist change in the database + if err := setServerDowntimeFn(sn, newFilterType); err != nil { + // Revert the change in filteredServers if SetServerDowntime fails + if hasOriginalFilter { + filteredServers[sn] = originalFilterType + } else { + delete(filteredServers, sn) } + + ctx.JSON(http.StatusInternalServerError, server_structs.SimpleApiResp{ + Status: server_structs.RespFailed, + Msg: "Failed to persist server downtime due to database error", + }) + return } + ctx.JSON(http.StatusOK, server_structs.SimpleApiResp{Status: server_structs.RespOK, Msg: "success"}) } @@ -287,19 +302,48 @@ func handleAllowServer(ctx *gin.Context) { filteredServersMutex.Lock() defer filteredServersMutex.Unlock() + // Backup the original filter (downtime) type to revert in case of failure + originalFilterType, hasOriginalFilter := filteredServers[sn] + + // Perform actions based on the current filter type if ft == tempFiltered { - // For temporarily filtered server, allowing them by removing the server from the map + // Temporarily filtered server: allow it by removing from map delete(filteredServers, sn) - if err := DeleteServerDowntime(sn); err != nil { - log.Errorf("Failed to remove the downtime of server %s in director db", sn) + + if err := deleteServerDowntimeFn(sn); err != nil { + // Revert the change in filteredServers if DeleteServerDowntime fails + if hasOriginalFilter { + filteredServers[sn] = originalFilterType + } else { + delete(filteredServers, sn) + } + + ctx.JSON(http.StatusInternalServerError, server_structs.SimpleApiResp{ + Status: server_structs.RespFailed, + Msg: fmt.Sprintf("Failed to remove the downtime of server %s in director db", sn), + }) + return } } else if ft == permFiltered { - // For servers to filter from the config, temporarily allow the server + // Permanently filtered server: temporarily allow it filteredServers[sn] = tempAllowed - if err := SetServerDowntime(sn, tempAllowed); err != nil { - log.Errorf("Failed to persist the status change of server %s in director db", sn) + + if err := setServerDowntimeFn(sn, tempAllowed); err != nil { + // Revert the change in filteredServers if SetServerDowntime fails + if hasOriginalFilter { + filteredServers[sn] = originalFilterType + } else { + delete(filteredServers, sn) + } + + ctx.JSON(http.StatusInternalServerError, server_structs.SimpleApiResp{ + Status: server_structs.RespFailed, + Msg: fmt.Sprintf("Failed to remove the downtime of server %s in director db", sn), + }) + return } } else if ft == topoFiltered { + // Server is disabled by OSG Topology ctx.JSON(http.StatusBadRequest, server_structs.SimpleApiResp{ Status: server_structs.RespFailed, Msg: fmt.Sprintf("Can't allow server %s that is disabled by the OSG Topology. Contact OSG admin at support@osg-htc.org to enable the server.", sn), From 075cdeca817fbd87c2aee203c070375fe1f56c33 Mon Sep 17 00:00:00 2001 From: Howard Zhong Date: Mon, 11 Nov 2024 19:45:13 +0000 Subject: [PATCH 12/12] minor syntax fix --- director/director_api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/director/director_api.go b/director/director_api.go index c85e3da0c..a427c92c2 100644 --- a/director/director_api.go +++ b/director/director_api.go @@ -238,7 +238,7 @@ func ConfigFilterdServers() { if param.Director_DbLocation.GetString() != "" { persistedServerDowntimes, err := getAllServerDowntimes() if err != nil { - log.Error("Failed to read persisted server downtimes from director db", err) + log.Error("Failed to read persisted server downtimes from director db:", err) return } for _, serverDowntime := range persistedServerDowntimes {