You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/11/24 20:24:50 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5328: Delivery Service/server assignment safties
ocket8888 opened a new pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328
## What does this PR (Pull Request) do?
- [x] This PR fixes #5218
## Which Traffic Control components are affected by this PR?
- Traffic Ops
- Traffic Ops client (Go)
## What is the best way to verify this PR?
Run the API tests
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536260387
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
Review comment:
I think it might even overlay them in the same place on the page. Or maybe I'm thinking of the implementation in `experimental/traffic-portal`.
Anyway, I'll change it to one alert.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535732709
##########
File path: traffic_ops/v3-client/deliveryservice.go
##########
@@ -206,11 +206,13 @@ func (to *Session) GetDeliveryServicesByCDNID(cdnID int) ([]tc.DeliveryServiceNu
return to.GetDeliveryServicesByCDNIDWithHdr(cdnID, nil)
}
-func (to *Session) GetDeliveryServiceNullableWithHdr(id string, header http.Header) (*tc.DeliveryServiceNullable, ReqInf, error) {
+// GetDeliveryServiceNullableWithHdr fetches the Delivery Service with the given ID.
+func (to *Session) GetDeliveryServiceNullableWithHdr(id int, header http.Header) (*tc.DeliveryServiceNullableV30, ReqInf, error) {
Review comment:
Yeah, this'll need to be reverted if this doesn't get into 5.0.0. Which kind of sucks, but I can't put it in the 4.0 client because that doesn't exist yet, so maybe I should just not change this at all?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536396097
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
+ CommonServerProperties: tc.CommonServerProperties{
+ CachegroupID: &cacheGroupID,
+ CDNID: &cdnID,
+ DomainName: &xmlid,
+ HostName: &xmlid,
+ HTTPSPort: util.IntPtr(443),
+ PhysLocationID: &physLoc.ID,
+ ProfileID: &profile.ID,
+ RevalPending: new(bool),
+ StatusID: &statusID,
+ TCPPort: util.IntPtr(80),
+ TypeID: &serverTypeID,
+ UpdPending: new(bool),
+ },
+ Interfaces: []tc.ServerInterfaceInfo{
+ {
+ Name: "eth0",
+ IPAddresses: []tc.ServerIPAddress{
+ {
+ Address: "198.51.100.1",
+ ServiceAddress: true,
+ },
+ },
+ },
+ },
+ }
+ alerts, _, err = TOSession.CreateServerWithHdr(server, nil)
+ if err != nil {
+ t.Fatalf("Failed to create server: %v - alerts: %s", err, strings.Join(alerts.ToStrings(), ", "))
+ }
+ params := url.Values{}
+ params.Set("hostName", *server.HostName)
+ servers, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch server after creation: %v", err)
+ }
+ if len(servers.Response) != 1 {
+ t.Fatalf("Expected exactly 1 server with hostname '%s'; got: %d", *server.HostName, len(servers.Response))
+ }
+ server = servers.Response[0]
+ if server.ID == nil {
+ t.Fatal("Server had null/undefined ID after creation")
+ }
Review comment:
I did that, also made a note that the tests expect all newly added Delivery Services in the fixture data to belong to the `tenant1` Tenant, or the tenancy tests will break
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535726946
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
+ CommonServerProperties: tc.CommonServerProperties{
+ CachegroupID: &cacheGroupID,
+ CDNID: &cdnID,
+ DomainName: &xmlid,
+ HostName: &xmlid,
+ HTTPSPort: util.IntPtr(443),
+ PhysLocationID: &physLoc.ID,
+ ProfileID: &profile.ID,
+ RevalPending: new(bool),
+ StatusID: &statusID,
+ TCPPort: util.IntPtr(80),
+ TypeID: &serverTypeID,
+ UpdPending: new(bool),
+ },
+ Interfaces: []tc.ServerInterfaceInfo{
+ {
+ Name: "eth0",
+ IPAddresses: []tc.ServerIPAddress{
+ {
+ Address: "198.51.100.1",
+ ServiceAddress: true,
+ },
+ },
+ },
+ },
+ }
+ alerts, _, err = TOSession.CreateServerWithHdr(server, nil)
+ if err != nil {
+ t.Fatalf("Failed to create server: %v - alerts: %s", err, strings.Join(alerts.ToStrings(), ", "))
+ }
+ params := url.Values{}
+ params.Set("hostName", *server.HostName)
+ servers, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch server after creation: %v", err)
+ }
+ if len(servers.Response) != 1 {
+ t.Fatalf("Expected exactly 1 server with hostname '%s'; got: %d", *server.HostName, len(servers.Response))
+ }
+ server = servers.Response[0]
+ if server.ID == nil {
+ t.Fatal("Server had null/undefined ID after creation")
+ }
Review comment:
Because then I have to depend on the structure of `tc-fixtures` and changes therein could break the test. Like how `AssignTestDeliveryService` takes the first server and first DS in the fixture data and assigns them to one another, then if I put mine in as the first ds and server then I have to go change _that_ test to use some other hard-coded data. And If I put my ds and server last in the fixture data, then any time someone adds or remove a DS or server from the fixture data this test breaks.
This is very annoying, but it just seemed way less brittle than hard-coding a position in the fixture data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#issuecomment-742108420
Oh, whoops, I didn't see an addition to `CHANGELOG.md` -- @ocket8888 mind opening a follow-up PR to add one?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535728877
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
+GROUP BY deliveryservice_server.deliveryservice
+HAVING COUNT(deliveryservice_server.server) = 1
+`
+
+func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+ log.Debugf("TEST: checking last server #%d in DSes: %v", serverID, dsIDs)
Review comment:
lol no. my bad
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535733339
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
Review comment:
I guess there's not really a good reason. Plus I'm not sure TP can handle that right now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535728593
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
Review comment:
Sure. Coming from doing more C stuff it just always feels weird to me to use things before their declaration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536312601
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1303,13 +1326,51 @@ func Update(w http.ResponseWriter, r *http.Request) {
server.ID = new(int)
*server.ID = inf.IntParams["id"]
+ status, ok, err := dbhelpers.GetStatusByID(*server.StatusID, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting server #%d status (#%d): %v", id, *server.StatusID, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if !ok {
+ log.Warnf("previously existent status #%d not found when fetching later", *server.StatusID)
+ userErr = fmt.Errorf("no such Status: #%d", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil)
+ return
+ }
+ if status.Name == nil {
+ sysErr = fmt.Errorf("status #%d had no name", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
+ for _, dsID := range dsIDs {
+ alert := tc.Alert{
+ Level: tc.ErrorLevel.String(),
+ Text: fmt.Sprintf("setting server status to '%s' would leave Delivery Service #%d with no 'ONLINE' or 'REPORTED' servers", *status.Name, dsID),
+ }
+ alerts = append(alerts, alert)
+ }
+ api.WriteAlerts(w, r, http.StatusConflict, tc.Alerts{Alerts: alerts})
+ return
+ }
+ }
+ server.ID = &id
Review comment:
it is not
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536193214
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
+ CommonServerProperties: tc.CommonServerProperties{
+ CachegroupID: &cacheGroupID,
+ CDNID: &cdnID,
+ DomainName: &xmlid,
+ HostName: &xmlid,
+ HTTPSPort: util.IntPtr(443),
+ PhysLocationID: &physLoc.ID,
+ ProfileID: &profile.ID,
+ RevalPending: new(bool),
+ StatusID: &statusID,
+ TCPPort: util.IntPtr(80),
+ TypeID: &serverTypeID,
+ UpdPending: new(bool),
+ },
+ Interfaces: []tc.ServerInterfaceInfo{
+ {
+ Name: "eth0",
+ IPAddresses: []tc.ServerIPAddress{
+ {
+ Address: "198.51.100.1",
+ ServiceAddress: true,
+ },
+ },
+ },
+ },
+ }
+ alerts, _, err = TOSession.CreateServerWithHdr(server, nil)
+ if err != nil {
+ t.Fatalf("Failed to create server: %v - alerts: %s", err, strings.Join(alerts.ToStrings(), ", "))
+ }
+ params := url.Values{}
+ params.Set("hostName", *server.HostName)
+ servers, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch server after creation: %v", err)
+ }
+ if len(servers.Response) != 1 {
+ t.Fatalf("Expected exactly 1 server with hostname '%s'; got: %d", *server.HostName, len(servers.Response))
+ }
+ server = servers.Response[0]
+ if server.ID == nil {
+ t.Fatal("Server had null/undefined ID after creation")
+ }
Review comment:
You don't have to depend on a particular index in tc-fixtures.json. I agree that is bad practice and should probably never be done. We should generally always be free to _add_ data to tc-fixtures.json, and any test that prevents that must be fixed. However, I think it is perfectly acceptable to add new data to tc-fixtures.json and reference primary keys (e.g. xml_id) in the test, because generally people should not _change_ existing objects in tc-fixtures.json (at least not significantly). It is usually safer to try to create the test from existing data (if possible, without changing it), or just add new data to tc-fixtures.json.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535706219
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
Review comment:
+1
It also has the benefit of not breaking if the internal embedded-type structure of `tc.DeliveryServiceNullableV30` changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp merged pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536314563
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1346,26 +1407,34 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
- if userErr, sysErr, errCode = deleteInterfaces(inf.IntParams["id"], tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = deleteInterfaces(id, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
- if userErr, sysErr, errCode = createInterfaces(inf.IntParams["id"], interfaces, tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = createInterfaces(id, server.Interfaces, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
if inf.Version.Major >= 3 {
- if userErr, sysErr, errCode = updateStatusLastUpdatedTime(inf.IntParams["id"], &statusLastUpdatedTime, tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = updateStatusLastUpdatedTime(id, &statusLastUpdatedTime, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", tc.ServerNullable{CommonServerProperties: server.CommonServerProperties, Interfaces: interfaces, StatusLastUpdated: &statusLastUpdatedTime})
- } else if inf.Version.Major == 1 {
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server.ServerNullableV11)
- } else {
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server)
+ } else {
+ v2Server, err := server.ToServerV2()
+ if err != nil {
+ sysErr = fmt.Errorf("converting valid v3 server to a v2 structure: %v", err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if inf.Version.Major <= 1 {
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", v2Server.ServerNullableV11)
+ } else {
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server)
Review comment:
yes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r535731055
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -23,11 +23,12 @@ import (
"database/sql"
"errors"
"fmt"
- "github.com/jmoiron/sqlx"
"net/http"
"strconv"
"strings"
+ "github.com/jmoiron/sqlx"
Review comment:
y-yeah, it's my IDE's fault. That thing keeps putting bugs in my code too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536196863
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
Review comment:
Yeah, that was my other concern. I kind of imagined getting 100 alerts back and having to scroll down past 100 red alert panes (not that I know it does that or not).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536396240
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
Review comment:
No longer relevant
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
Review comment:
No longer relevant
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r536195820
##########
File path: traffic_ops/v3-client/deliveryservice.go
##########
@@ -206,11 +206,13 @@ func (to *Session) GetDeliveryServicesByCDNID(cdnID int) ([]tc.DeliveryServiceNu
return to.GetDeliveryServicesByCDNIDWithHdr(cdnID, nil)
}
-func (to *Session) GetDeliveryServiceNullableWithHdr(id string, header http.Header) (*tc.DeliveryServiceNullable, ReqInf, error) {
+// GetDeliveryServiceNullableWithHdr fetches the Delivery Service with the given ID.
+func (to *Session) GetDeliveryServiceNullableWithHdr(id int, header http.Header) (*tc.DeliveryServiceNullableV30, ReqInf, error) {
Review comment:
Yeah, we should probably stop backporting new features into 5.0.x. It's confusing as a developer to not know what release a change is going into, when there are only certain things you can do or have to do around a release boundary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5328: Delivery Service/server assignment safeties
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5328:
URL: https://github.com/apache/trafficcontrol/pull/5328#discussion_r534550945
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
Review comment:
We should use declarations like this:
```
ds := tc.DeliveryServiceNullableV30{}
ds.Active = new(bool)
...
(etc)
```
Then you don't need all the unnecessary nesting.
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
Review comment:
Do you mind moving this function below `func TestDeliveryServiceServers` since that is basically the "entrypoint" of the test which is usually always the first function in the test file?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -493,12 +530,26 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
}
// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
-func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo) (error, error, int) {
+func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo, replace bool) (error, error, int) {
userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
if userErr != nil || sysErr != nil {
return userErr, sysErr, status
}
+ if ds.Active && replace {
+ ids := make([]int, 0, len(serverInfos))
+ for _, inf := range serverInfos {
+ ids = append(ids, inf.ID)
+ }
+ ok, err := verifyStatuses(ids, tx)
+ if err != nil {
+ return nil, fmt.Errorf("verifying statuses: %v", err), http.StatusInternalServerError
+ }
+ if !ok {
+ return fmt.Errorf("this server assignment leaves Delivery Service #%d without any 'ONLINE' or 'REPORTED' servers", ds.ID), nil, http.StatusConflict
Review comment:
This error message should specify that it's an _active_ delivery service as well.
Also, it could use `tc.CacheStatusOnline` and `tc.CacheStatusReported`.
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
+ CommonServerProperties: tc.CommonServerProperties{
+ CachegroupID: &cacheGroupID,
+ CDNID: &cdnID,
+ DomainName: &xmlid,
+ HostName: &xmlid,
+ HTTPSPort: util.IntPtr(443),
+ PhysLocationID: &physLoc.ID,
+ ProfileID: &profile.ID,
+ RevalPending: new(bool),
+ StatusID: &statusID,
+ TCPPort: util.IntPtr(80),
+ TypeID: &serverTypeID,
+ UpdPending: new(bool),
+ },
+ Interfaces: []tc.ServerInterfaceInfo{
+ {
+ Name: "eth0",
+ IPAddresses: []tc.ServerIPAddress{
+ {
+ Address: "198.51.100.1",
+ ServiceAddress: true,
+ },
+ },
+ },
+ },
+ }
+ alerts, _, err = TOSession.CreateServerWithHdr(server, nil)
+ if err != nil {
+ t.Fatalf("Failed to create server: %v - alerts: %s", err, strings.Join(alerts.ToStrings(), ", "))
+ }
+ params := url.Values{}
+ params.Set("hostName", *server.HostName)
+ servers, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch server after creation: %v", err)
+ }
+ if len(servers.Response) != 1 {
+ t.Fatalf("Expected exactly 1 server with hostname '%s'; got: %d", *server.HostName, len(servers.Response))
+ }
+ server = servers.Response[0]
+ if server.ID == nil {
+ t.Fatal("Server had null/undefined ID after creation")
+ }
+
+ _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID, []int{*server.ID}, true)
+ if err != nil {
+ t.Fatalf("Failed to assign server to Delivery Service: %v", err)
+ }
+
+ _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID, []int{}, true)
+ if err == nil {
+ t.Error("Didn't get expected error trying to remove the only server assigned to a Delivery Service")
+ } else {
+ t.Logf("Got expected error trying to remove the only server assigned to a Delivery Service: %v", err)
+ }
+
+ _, _, err = TOSession.DeleteDeliveryServiceServer(*ds.ID, *server.ID)
+ if err == nil {
Review comment:
this `if` is redundant
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1184,78 +1190,93 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
- //Get original xmppid
- origSer, _, userErr, sysErr, _, _ := getServers(r.Header, inf.Params, inf.Tx, inf.User, false, *version)
+ id := inf.IntParams["id"]
+
+ // Get original server
+ originals, _, userErr, sysErr, errCode, _ := getServers(r.Header, inf.Params, inf.Tx, inf.User, false, *version)
if userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
- if len(origSer) == 0 {
+ if len(originals) < 1 {
api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("the server doesn't exist, cannot update"), nil)
return
}
- origServer := origSer[0]
- originalXMPPID := ""
- originalStatusID := 0
- changeXMPPID := false
- if origServer.XMPPID != nil {
- originalXMPPID = *origServer.XMPPID
+ if len(originals) > 1 {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("too many servers by ID %d: %d", id, len(originals)))
Review comment:
missing `return`
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
Review comment:
Why N alerts instead of one alert with N delivery services? I'd think most of our APIs only return a single alert unless they have a good reason.
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
Review comment:
`tc.CacheStatusOnline` and `tc.CacheStatusReported`
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -23,11 +23,12 @@ import (
"database/sql"
"errors"
"fmt"
- "github.com/jmoiron/sqlx"
"net/http"
"strconv"
"strings"
+ "github.com/jmoiron/sqlx"
Review comment:
I know this was probably your IDE, but if we're moving imports this belongs down w/ `lib/pq`.
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
Review comment:
Here as well, it would probably be better to do:
```
server := tc.ServerV30{}
server.CachegroupID = &cacheGroupID
...
(etc)
```
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -337,6 +337,43 @@ type DSServerIds struct {
type TODSServerIds DSServerIds
+const verifyStatusesQuery = `
+SELECT status.name = 'ONLINE' OR status.name = 'REPORTED'
+FROM server
+INNER JOIN status ON server.status = status.id
+WHERE server.id = ANY($1::BIGINT[])
+`
+
+func verifyStatuses(ids []int, tx *sql.Tx) (bool, error) {
Review comment:
Would a better name be `verifyAtLeastOneAvailableServer`?
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -17,21 +17,339 @@ package v3
import (
"errors"
+ "fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
+ "time"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
)
+func TryToRemoveLastServerInDeliveryService(t *testing.T) {
+ cdns, _, err := TOSession.GetCDNsWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch CDNs: %v", err)
+ }
+ if len(cdns) < 1 {
+ t.Fatal("Need at least one CDN to test removing the last server from a Delivery Service")
+ }
+
+ cdnID := cdns[0].ID
+
+ user, _, err := TOSession.GetUserCurrentWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch current user: %v", err)
+ }
+ if user == nil {
+ t.Fatalf("Current user is null")
+ }
+ if user.TenantID == nil {
+ t.Fatalf("Current user has null Tenant ID")
+ }
+
+ types, _, err := TOSession.GetTypesWithHdr(nil, "deliveryservice")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'deliveryservice' to create a Delivery Service")
+ }
+ dsTypeID := types[0].ID
+
+ xmlid := fmt.Sprintf("test-%d", time.Now().Unix())
+ ds := tc.DeliveryServiceNullableV30{
+ DeliveryServiceNullableV15: tc.DeliveryServiceNullableV15{
+ DeliveryServiceNullableV14: tc.DeliveryServiceNullableV14{
+ DeliveryServiceNullableV13: tc.DeliveryServiceNullableV13{
+ DeliveryServiceNullableV12: tc.DeliveryServiceNullableV12{
+ DeliveryServiceNullableV11: tc.DeliveryServiceNullableV11{
+ Active: new(bool),
+ AnonymousBlockingEnabled: new(bool),
+ CDNID: &cdnID,
+ DisplayName: util.StrPtr("test"),
+ DSCP: new(int),
+ GeoLimit: new(int),
+ GeoProvider: new(int),
+ InitialDispersion: new(int),
+ IPV6RoutingEnabled: new(bool),
+ LogsEnabled: new(bool),
+ MissLat: new(float64),
+ MissLong: new(float64),
+ OrgServerFQDN: util.StrPtr("https://example.com"),
+ Protocol: new(int),
+ QStringIgnore: new(int),
+ RangeRequestHandling: new(int),
+ RegionalGeoBlocking: new(bool),
+ Signed: false,
+ TenantID: user.TenantID,
+ TypeID: &dsTypeID,
+ XMLID: &xmlid,
+ },
+ },
+ },
+ },
+ },
+ }
+ *ds.InitialDispersion = 1
+ *ds.Active = true
+ ds, _, err = TOSession.CreateDeliveryServiceV30(ds)
+ if err != nil {
+ t.Fatalf("Failed to create Delivery Service: %v", err)
+ }
+ if ds.ID == nil {
+ t.Fatal("Delivery Service had null/undefined ID after creation")
+ }
+
+ cacheGroups, _, err := TOSession.GetCacheGroupsNullableWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Cache Groups: %v", err)
+ }
+ if len(cacheGroups) < 1 {
+ t.Fatal("Need at least one Cache Group for testing")
+ }
+ if cacheGroups[0].ID == nil {
+ t.Fatal("Got a Cache Group for testing with null or undefined ID")
+ }
+ cacheGroupID := *cacheGroups[0].ID
+
+ profile := tc.Profile{
+ CDNID: cdnID,
+ Description: "test",
+ Name: xmlid,
+ Type: "ATS_PROFILE",
+ }
+ alerts, _, err := TOSession.CreateProfile(profile)
+ t.Logf("Alerts from creating Profile: %s", strings.Join(alerts.ToStrings(), ", "))
+ if err != nil {
+ t.Fatalf("Failed to create new Profile in CDN #%d: %v", cdnID, err)
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profile.Name, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Profile '%s' after creation: %v", profile.Name, err)
+ }
+ if len(profiles) != 1 {
+ t.Fatalf("Expected exactly one Profile named '%s'; got: %d", profile.Name, len(profiles))
+ }
+ profile = profiles[0]
+
+ physLocs, _, err := TOSession.GetPhysLocationsWithHdr(nil, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Physical Locations: %v", err)
+ }
+ if len(physLocs) < 1 {
+ t.Fatal("Need at least one Physical Location")
+ }
+ physLoc := physLocs[0]
+
+ statuses, _, err := TOSession.GetStatusesWithHdr(nil)
+ if err != nil {
+ t.Fatalf("Could not fetch Statuses: %v", err)
+ }
+ if len(statuses) < 1 {
+ t.Fatal("Need at least one Status")
+ }
+
+ var statusID int
+ var badStatusID int
+ found := false
+ foundBad := false
+ for _, status := range statuses {
+ if status.Name == "ONLINE" || status.Name == "REPORTED" {
+ statusID = status.ID
+ found = true
+ if foundBad {
+ break
+ }
+ } else {
+ badStatusID = status.ID
+ foundBad = true
+ if found {
+ break
+ }
+ }
+ }
+ if !found || !foundBad {
+ t.Fatal("Need at least one status with the name 'ONLINE' or 'REPORTED' and at least one status with neither of those names")
+ }
+
+ types, _, err = TOSession.GetTypesWithHdr(nil, "server")
+ if err != nil {
+ t.Fatalf("Could not fetch Types: %v", err)
+ }
+ if len(types) < 1 {
+ t.Fatal("Need at least one Type with 'useInTable' of 'server' to create a server")
+ }
+ serverTypeID := types[0].ID
+
+ server := tc.ServerV30{
+ CommonServerProperties: tc.CommonServerProperties{
+ CachegroupID: &cacheGroupID,
+ CDNID: &cdnID,
+ DomainName: &xmlid,
+ HostName: &xmlid,
+ HTTPSPort: util.IntPtr(443),
+ PhysLocationID: &physLoc.ID,
+ ProfileID: &profile.ID,
+ RevalPending: new(bool),
+ StatusID: &statusID,
+ TCPPort: util.IntPtr(80),
+ TypeID: &serverTypeID,
+ UpdPending: new(bool),
+ },
+ Interfaces: []tc.ServerInterfaceInfo{
+ {
+ Name: "eth0",
+ IPAddresses: []tc.ServerIPAddress{
+ {
+ Address: "198.51.100.1",
+ ServiceAddress: true,
+ },
+ },
+ },
+ },
+ }
+ alerts, _, err = TOSession.CreateServerWithHdr(server, nil)
+ if err != nil {
+ t.Fatalf("Failed to create server: %v - alerts: %s", err, strings.Join(alerts.ToStrings(), ", "))
+ }
+ params := url.Values{}
+ params.Set("hostName", *server.HostName)
+ servers, _, err := TOSession.GetServersWithHdr(¶ms, nil)
+ if err != nil {
+ t.Fatalf("Could not fetch server after creation: %v", err)
+ }
+ if len(servers.Response) != 1 {
+ t.Fatalf("Expected exactly 1 server with hostname '%s'; got: %d", *server.HostName, len(servers.Response))
+ }
+ server = servers.Response[0]
+ if server.ID == nil {
+ t.Fatal("Server had null/undefined ID after creation")
+ }
Review comment:
Rather than spending 200+ lines creating a delivery service and server in the test, why not just define them in `tc-fixtures.json` and have them built and cleaned up for free? All of that kind of detracts from the real tests at hand.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
##########
@@ -36,54 +38,129 @@ func Delete(w http.ResponseWriter, r *http.Request) {
delete(w, r, false)
}
-// Delete deprecatation handler for deleting the association between a Delivery Service and a Server
+// DeleteDeprecated is the deprecatation handler for deleting the association
Review comment:
sp: deprecatation
##########
File path: traffic_ops/v3-client/deliveryservice.go
##########
@@ -206,11 +206,13 @@ func (to *Session) GetDeliveryServicesByCDNID(cdnID int) ([]tc.DeliveryServiceNu
return to.GetDeliveryServicesByCDNIDWithHdr(cdnID, nil)
}
-func (to *Session) GetDeliveryServiceNullableWithHdr(id string, header http.Header) (*tc.DeliveryServiceNullable, ReqInf, error) {
+// GetDeliveryServiceNullableWithHdr fetches the Delivery Service with the given ID.
+func (to *Session) GetDeliveryServiceNullableWithHdr(id int, header http.Header) (*tc.DeliveryServiceNullableV30, ReqInf, error) {
Review comment:
Changing the signature here should be ok since this method is currently unreleased, right? But 5.0 is leaving the station...
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -337,6 +337,43 @@ type DSServerIds struct {
type TODSServerIds DSServerIds
+const verifyStatusesQuery = `
+SELECT status.name = 'ONLINE' OR status.name = 'REPORTED'
Review comment:
we could use `tc.CacheStatusOnline` and `tc.CacheStatusReported` here
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
Review comment:
we could use `tc.CacheStatusOnline` and `tc.CacheStatusReported` here
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1303,13 +1326,51 @@ func Update(w http.ResponseWriter, r *http.Request) {
server.ID = new(int)
*server.ID = inf.IntParams["id"]
+ status, ok, err := dbhelpers.GetStatusByID(*server.StatusID, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting server #%d status (#%d): %v", id, *server.StatusID, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if !ok {
+ log.Warnf("previously existent status #%d not found when fetching later", *server.StatusID)
+ userErr = fmt.Errorf("no such Status: #%d", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil)
+ return
+ }
+ if status.Name == nil {
+ sysErr = fmt.Errorf("status #%d had no name", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
+ for _, dsID := range dsIDs {
+ alert := tc.Alert{
+ Level: tc.ErrorLevel.String(),
+ Text: fmt.Sprintf("setting server status to '%s' would leave Delivery Service #%d with no 'ONLINE' or 'REPORTED' servers", *status.Name, dsID),
Review comment:
This should specify _active_ delivery service. Also, ditto my previous comment about N alerts.
##########
File path: traffic_ops/traffic_ops_golang/server/put_status.go
##########
@@ -24,69 +24,94 @@ import (
"encoding/json"
"errors"
"fmt"
- "github.com/apache/trafficcontrol/lib/go-log"
"net/http"
"strings"
"time"
+ "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
)
+// UpdateStatusHandler is the handler for PUT requests to the /servers/{{ID}}/status API endpoint.
func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
reqObj := tc.ServerPutStatus{}
if err := json.NewDecoder(r.Body).Decode(&reqObj); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("malformed JSON: "+err.Error()), nil)
return
}
- serverInfo, exists, err := dbhelpers.GetServerInfo(inf.IntParams["id"], inf.Tx.Tx)
+ id := inf.IntParams["id"]
+ serverInfo, exists, err := dbhelpers.GetServerInfo(id, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !exists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", inf.IntParams["id"]), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("server ID %d not found", id), nil)
return
}
status := tc.StatusNullable{}
statusExists := false
if reqObj.Status.Name != nil {
- status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByName(*reqObj.Status.Name, tx)
} else if reqObj.Status.ID != nil {
- status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, inf.Tx.Tx)
+ status, statusExists, err = dbhelpers.GetStatusByID(*reqObj.Status.ID, tx)
} else {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("status is required"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("status is required"), nil)
return
}
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err)
return
}
if !statusExists {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("invalid status (does not exist)"), nil)
return
}
if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
- if err := updateServerStatusAndOfflineReason(inf.IntParams["id"], *status.ID, reqObj.OfflineReason, inf.Tx.Tx); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
+
+ existingStatus, existingStatusUpdatedTime := checkExistingStatusInfo(id, tx)
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" && *status.ID != existingStatus {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
+ for _, dsID := range dsIDs {
+ alert := tc.Alert{
+ Level: tc.ErrorLevel.String(),
+ Text: fmt.Sprintf("setting server status to '%s' would leave Delivery Service #%d with no 'ONLINE' or 'REPORTED' servers", *status.Name, dsID),
Review comment:
this error message should specify _active_ delivery service as well, and can use `tc.CacheStatusOnline` and `tc.CacheStatusReported`
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1303,13 +1326,51 @@ func Update(w http.ResponseWriter, r *http.Request) {
server.ID = new(int)
*server.ID = inf.IntParams["id"]
+ status, ok, err := dbhelpers.GetStatusByID(*server.StatusID, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting server #%d status (#%d): %v", id, *server.StatusID, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if !ok {
+ log.Warnf("previously existent status #%d not found when fetching later", *server.StatusID)
+ userErr = fmt.Errorf("no such Status: #%d", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil)
+ return
+ }
+ if status.Name == nil {
+ sysErr = fmt.Errorf("status #%d had no name", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" {
Review comment:
`tc.CacheStatusOnline` and `tc.CacheStatusReported`
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1303,13 +1326,51 @@ func Update(w http.ResponseWriter, r *http.Request) {
server.ID = new(int)
*server.ID = inf.IntParams["id"]
+ status, ok, err := dbhelpers.GetStatusByID(*server.StatusID, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting server #%d status (#%d): %v", id, *server.StatusID, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if !ok {
+ log.Warnf("previously existent status #%d not found when fetching later", *server.StatusID)
+ userErr = fmt.Errorf("no such Status: #%d", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil)
+ return
+ }
+ if status.Name == nil {
+ sysErr = fmt.Errorf("status #%d had no name", *server.StatusID)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if *status.Name != "ONLINE" && *status.Name != "REPORTED" {
+ dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
+ for _, dsID := range dsIDs {
+ alert := tc.Alert{
+ Level: tc.ErrorLevel.String(),
+ Text: fmt.Sprintf("setting server status to '%s' would leave Delivery Service #%d with no 'ONLINE' or 'REPORTED' servers", *status.Name, dsID),
+ }
+ alerts = append(alerts, alert)
+ }
+ api.WriteAlerts(w, r, http.StatusConflict, tc.Alerts{Alerts: alerts})
+ return
+ }
+ }
+ server.ID = &id
Review comment:
Is this re-assignment necessary? L1328 already seems to do this.
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
+GROUP BY deliveryservice_server.deliveryservice
+HAVING COUNT(deliveryservice_server.server) = 1
+`
+
+func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+ log.Debugf("TEST: checking last server #%d in DSes: %v", serverID, dsIDs)
+ violations := []int{}
+ rows, err := tx.Query(lastServerInActiveDeliveryServicesQuery, serverID, pq.Array(dsIDs))
+ if err != nil {
+ return violations, fmt.Errorf("querying: %v", err)
+ }
+ defer rows.Close()
+
+ for rows.Next() {
+ var violation int
+ if err = rows.Scan(&violation); err != nil {
+ return violations, fmt.Errorf("scanning: %v", err)
+ }
+ violations = append(violations, violation)
+ }
+
+ log.Debugf("TEST: violations: %v", violations)
Review comment:
Is this debug log still necessary?
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1619,6 +1728,23 @@ func Delete(w http.ResponseWriter, r *http.Request) {
id := inf.IntParams["id"]
+ if dsIDs, err := getDeliveryServicesThatOnlyHaveThisServerAssigned(id, tx); err != nil {
+ sysErr = fmt.Errorf("checking if server #%d is the last server assigned to any Delivery Services: %v", id, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ } else if len(dsIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(dsIDs))
Review comment:
ditto earlier comment about N alerts
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1601,6 +1671,45 @@ func Create(w http.ResponseWriter, r *http.Request) {
}
}
+const lastServerOfDSesQuery = `
+SELECT "deliveryservice"
+FROM deliveryservice_server
+WHERE "deliveryservice" IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice
+ ON deliveryservice_server.deliveryservice = deliveryservice.id
+ WHERE deliveryservice_server."server" = $1
+ AND deliveryservice.active
+)
+GROUP BY "deliveryservice"
+HAVING COUNT("server") = 1;
+`
+
+func getDeliveryServicesThatOnlyHaveThisServerAssigned(id int, tx *sql.Tx) ([]int, error) {
+ var ids []int
+ if tx == nil {
+ return ids, errors.New("nil transaction")
+ }
+
+ rows, err := tx.Query(lastServerOfDSesQuery, id)
+ if err != nil {
+ return ids, fmt.Errorf("querying: %v", err)
+ }
+
Review comment:
don't forget to `defer log.Close(rows, "...")`
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1601,6 +1671,45 @@ func Create(w http.ResponseWriter, r *http.Request) {
}
}
+const lastServerOfDSesQuery = `
+SELECT "deliveryservice"
+FROM deliveryservice_server
+WHERE "deliveryservice" IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice
+ ON deliveryservice_server.deliveryservice = deliveryservice.id
+ WHERE deliveryservice_server."server" = $1
+ AND deliveryservice.active
+)
+GROUP BY "deliveryservice"
+HAVING COUNT("server") = 1;
+`
+
+func getDeliveryServicesThatOnlyHaveThisServerAssigned(id int, tx *sql.Tx) ([]int, error) {
Review comment:
this name should probably specify _active_
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
+GROUP BY deliveryservice_server.deliveryservice
+HAVING COUNT(deliveryservice_server.server) = 1
+`
+
+func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+ log.Debugf("TEST: checking last server #%d in DSes: %v", serverID, dsIDs)
Review comment:
Is this debug log still necessary?
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1346,26 +1407,34 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
- if userErr, sysErr, errCode = deleteInterfaces(inf.IntParams["id"], tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = deleteInterfaces(id, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
- if userErr, sysErr, errCode = createInterfaces(inf.IntParams["id"], interfaces, tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = createInterfaces(id, server.Interfaces, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
if inf.Version.Major >= 3 {
- if userErr, sysErr, errCode = updateStatusLastUpdatedTime(inf.IntParams["id"], &statusLastUpdatedTime, tx); userErr != nil || sysErr != nil {
+ if userErr, sysErr, errCode = updateStatusLastUpdatedTime(id, &statusLastUpdatedTime, tx); userErr != nil || sysErr != nil {
api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", tc.ServerNullable{CommonServerProperties: server.CommonServerProperties, Interfaces: interfaces, StatusLastUpdated: &statusLastUpdatedTime})
- } else if inf.Version.Major == 1 {
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server.ServerNullableV11)
- } else {
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server)
+ } else {
+ v2Server, err := server.ToServerV2()
+ if err != nil {
+ sysErr = fmt.Errorf("converting valid v3 server to a v2 structure: %v", err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if inf.Version.Major <= 1 {
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", v2Server.ServerNullableV11)
+ } else {
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated", server)
Review comment:
Shouldn't this be `v2Server` instead `server`?
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
+GROUP BY deliveryservice_server.deliveryservice
+HAVING COUNT(deliveryservice_server.server) = 1
+`
+
+func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+ log.Debugf("TEST: checking last server #%d in DSes: %v", serverID, dsIDs)
+ violations := []int{}
+ rows, err := tx.Query(lastServerInActiveDeliveryServicesQuery, serverID, pq.Array(dsIDs))
+ if err != nil {
+ return violations, fmt.Errorf("querying: %v", err)
+ }
+ defer rows.Close()
+
+ for rows.Next() {
+ var violation int
+ if err = rows.Scan(&violation); err != nil {
+ return violations, fmt.Errorf("scanning: %v", err)
+ }
+ violations = append(violations, violation)
+ }
+
+ log.Debugf("TEST: violations: %v", violations)
+
+ return violations, nil
+}
+
+// AssignDeliveryServicesToServerHandler is the handler for POST requests to /servers/{{ID}}/deliveryservices.
func AssignDeliveryServicesToServerHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
dsList := []int{}
if err := json.NewDecoder(r.Body).Decode(&dsList); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("payload must be a list of integers representing delivery service ids"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("payload must be a list of integers representing delivery service ids"), nil)
return
}
replaceQueryParameter := inf.Params["replace"]
replace, err := strconv.ParseBool(replaceQueryParameter) //accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False. for replace url parameter documentation
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, err, nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
return
}
server := inf.IntParams["id"]
- serverInfo, ok, err := dbhelpers.GetServerInfo(server, inf.Tx.Tx)
+ serverInfo, ok, err := dbhelpers.GetServerInfo(server, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("getting server name from ID: "+err.Error()))
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, errors.New("getting server name from ID: "+err.Error()))
return
} else if !ok {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, errors.New("no server with that ID found"), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("no server with that ID found"), nil)
return
}
if !strings.HasPrefix(serverInfo.Type, tc.OriginTypeName) {
- usrErr, sysErr, status := ValidateDSCapabilities(dsList, serverInfo.HostName, inf.Tx.Tx)
+ usrErr, sysErr, status := ValidateDSCapabilities(dsList, serverInfo.HostName, tx)
if usrErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, status, usrErr, sysErr)
+ api.HandleErr(w, r, tx, status, usrErr, sysErr)
return
}
}
// We already know the CDN exists because that's part of the serverInfo query above
- serverCDN, _, err := dbhelpers.GetCDNNameFromID(inf.Tx.Tx, int64(serverInfo.CDNID))
+ serverCDN, _, err := dbhelpers.GetCDNNameFromID(tx, int64(serverInfo.CDNID))
if err != nil {
sysErr = fmt.Errorf("Failed to get CDN name from ID: %v", err)
errCode = http.StatusInternalServerError
- api.HandleErr(w, r, inf.Tx.Tx, errCode, nil, sysErr)
+ api.HandleErr(w, r, tx, errCode, nil, sysErr)
return
}
if len(dsList) > 0 {
- if errCode, userErr, sysErr = checkTenancyAndCDN(inf.Tx.Tx, string(serverCDN), server, serverInfo, dsList, inf.User); userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ if errCode, userErr, sysErr = checkTenancyAndCDN(tx, string(serverCDN), server, serverInfo, dsList, inf.User); userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
if strings.HasPrefix(serverInfo.Type, tc.OriginTypeName) {
- if userErr, sysErr, status := checkOriginInTopologies(inf.Tx.Tx, serverInfo.Cachegroup, dsList); userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, status, userErr, sysErr)
+ if userErr, sysErr, status := checkOriginInTopologies(tx, serverInfo.Cachegroup, dsList); userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, status, userErr, sysErr)
return
}
}
}
- assignedDSes, err := assignDeliveryServicesToServer(server, dsList, replace, inf.Tx.Tx)
+ if replace && (serverInfo.Status == tc.CacheStatusOnline.String() || serverInfo.Status == tc.CacheStatusReported.String()) {
+ currentDSIDs, err := checkForLastServerInActiveDeliveryServices(server, dsList, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("checking for deliveryservices to which server #%d is the last assigned: %v", server, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(currentDSIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(currentDSIDs))
Review comment:
ditto earlier comment about N alerts
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -62,77 +62,141 @@ func getConfigFile(prefix string, xmlId string) string {
return prefix + xmlId + configSuffix
}
+const lastServerInActiveDeliveryServicesQuery = `
+SELECT deliveryservice_server.deliveryservice
+FROM deliveryservice_server
+INNER JOIN server ON server.id = deliveryservice_server.server
+INNER JOIN status ON status.id = server.status
+WHERE deliveryservice IN (
+ SELECT deliveryservice_server.deliveryservice
+ FROM deliveryservice_server
+ INNER JOIN deliveryservice ON deliveryservice.id = deliveryservice_server.deliveryservice
+ WHERE deliveryservice_server.server=$1
+ AND deliveryservice.active IS TRUE
+)
+AND NOT (deliveryservice_server.deliveryservice = ANY($2::BIGINT[]))
+AND (status.name = 'ONLINE' OR status.name = 'REPORTED')
+GROUP BY deliveryservice_server.deliveryservice
+HAVING COUNT(deliveryservice_server.server) = 1
+`
+
+func checkForLastServerInActiveDeliveryServices(serverID int, dsIDs []int, tx *sql.Tx) ([]int, error) {
+ log.Debugf("TEST: checking last server #%d in DSes: %v", serverID, dsIDs)
+ violations := []int{}
+ rows, err := tx.Query(lastServerInActiveDeliveryServicesQuery, serverID, pq.Array(dsIDs))
+ if err != nil {
+ return violations, fmt.Errorf("querying: %v", err)
+ }
+ defer rows.Close()
+
+ for rows.Next() {
+ var violation int
+ if err = rows.Scan(&violation); err != nil {
+ return violations, fmt.Errorf("scanning: %v", err)
+ }
+ violations = append(violations, violation)
+ }
+
+ log.Debugf("TEST: violations: %v", violations)
+
+ return violations, nil
+}
+
+// AssignDeliveryServicesToServerHandler is the handler for POST requests to /servers/{{ID}}/deliveryservices.
func AssignDeliveryServicesToServerHandler(w http.ResponseWriter, r *http.Request) {
inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"})
+ tx := inf.Tx.Tx
if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
defer inf.Close()
dsList := []int{}
if err := json.NewDecoder(r.Body).Decode(&dsList); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("payload must be a list of integers representing delivery service ids"), nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("payload must be a list of integers representing delivery service ids"), nil)
return
}
replaceQueryParameter := inf.Params["replace"]
replace, err := strconv.ParseBool(replaceQueryParameter) //accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False. for replace url parameter documentation
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, err, nil)
+ api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
return
}
server := inf.IntParams["id"]
- serverInfo, ok, err := dbhelpers.GetServerInfo(server, inf.Tx.Tx)
+ serverInfo, ok, err := dbhelpers.GetServerInfo(server, tx)
if err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("getting server name from ID: "+err.Error()))
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, errors.New("getting server name from ID: "+err.Error()))
return
} else if !ok {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, errors.New("no server with that ID found"), nil)
+ api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("no server with that ID found"), nil)
return
}
if !strings.HasPrefix(serverInfo.Type, tc.OriginTypeName) {
- usrErr, sysErr, status := ValidateDSCapabilities(dsList, serverInfo.HostName, inf.Tx.Tx)
+ usrErr, sysErr, status := ValidateDSCapabilities(dsList, serverInfo.HostName, tx)
if usrErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, status, usrErr, sysErr)
+ api.HandleErr(w, r, tx, status, usrErr, sysErr)
return
}
}
// We already know the CDN exists because that's part of the serverInfo query above
- serverCDN, _, err := dbhelpers.GetCDNNameFromID(inf.Tx.Tx, int64(serverInfo.CDNID))
+ serverCDN, _, err := dbhelpers.GetCDNNameFromID(tx, int64(serverInfo.CDNID))
if err != nil {
sysErr = fmt.Errorf("Failed to get CDN name from ID: %v", err)
errCode = http.StatusInternalServerError
- api.HandleErr(w, r, inf.Tx.Tx, errCode, nil, sysErr)
+ api.HandleErr(w, r, tx, errCode, nil, sysErr)
return
}
if len(dsList) > 0 {
- if errCode, userErr, sysErr = checkTenancyAndCDN(inf.Tx.Tx, string(serverCDN), server, serverInfo, dsList, inf.User); userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ if errCode, userErr, sysErr = checkTenancyAndCDN(tx, string(serverCDN), server, serverInfo, dsList, inf.User); userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
return
}
if strings.HasPrefix(serverInfo.Type, tc.OriginTypeName) {
- if userErr, sysErr, status := checkOriginInTopologies(inf.Tx.Tx, serverInfo.Cachegroup, dsList); userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, status, userErr, sysErr)
+ if userErr, sysErr, status := checkOriginInTopologies(tx, serverInfo.Cachegroup, dsList); userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, status, userErr, sysErr)
return
}
}
}
- assignedDSes, err := assignDeliveryServicesToServer(server, dsList, replace, inf.Tx.Tx)
+ if replace && (serverInfo.Status == tc.CacheStatusOnline.String() || serverInfo.Status == tc.CacheStatusReported.String()) {
+ currentDSIDs, err := checkForLastServerInActiveDeliveryServices(server, dsList, tx)
+ if err != nil {
+ sysErr = fmt.Errorf("checking for deliveryservices to which server #%d is the last assigned: %v", server, err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr)
+ return
+ }
+ if len(currentDSIDs) > 0 {
+ alerts := make([]tc.Alert, 0, len(currentDSIDs))
+ for _, dsID := range currentDSIDs {
+ alert := tc.Alert{
+ Level: tc.ErrorLevel.String(),
+ Text: fmt.Sprintf("Delivery Service assignment would leave Delivery Service #%d without any assigned ONLINE or REPORTED servers", dsID),
Review comment:
error message should specify _active_ delivery service, also: `tc.CacheStatusOnline` and `tc.CacheStatusReported`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org