diff --git a/frontend/src/api/recommendations.ts b/frontend/src/api/recommendations.ts index 810c48056..6b8f6ac26 100644 --- a/frontend/src/api/recommendations.ts +++ b/frontend/src/api/recommendations.ts @@ -85,6 +85,8 @@ export interface PartialProviderSweep { /** Recommendations the provider dropped as invalid (GCP: non-USD, malformed, unknown term). */ skipped_recommendations?: number; reasons: Record; + /** Platform account IDs whose rows are held through a pricing outage (reason `pricing_unavailable`). */ + held_account_ids?: string[]; } export interface RecommendationsFreshness { diff --git a/go.mod b/go.mod index a260648d2..a98b657df 100644 --- a/go.mod +++ b/go.mod @@ -34,9 +34,9 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.15.0 github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1 // indirect @@ -88,10 +88,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.5.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.5.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353 github.com/aws/aws-lambda-go v1.55.1 github.com/aws/aws-sdk-go-v2/service/kms v1.62.1 github.com/aws/aws-sdk-go-v2/service/lambda v1.113.4 @@ -113,20 +113,19 @@ require ( require ( cel.dev/expr v0.25.3 // indirect - cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a // indirect + cloud.google.com/go/auth v0.24.1 // indirect cloud.google.com/go/auth/oauth2adapt v0.3.0 // indirect cloud.google.com/go/monitoring v1.30.0 // indirect cloud.google.com/go/redis v1.24.0 // indirect - cloud.google.com/go/storage v1.62.3 // indirect - cloud.google.com/go/trace v1.11.7 // indirect + cloud.google.com/go/storage v1.70.0 // indirect dario.cat/mergo v1.0.2 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch v1.4.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.2.0 // indirect github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0 // indirect - github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0 // indirect - github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0 // indirect + github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0 // indirect + github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect github.com/agext/levenshtein v1.2.1 // indirect github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect @@ -154,6 +153,7 @@ require ( github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/klauspost/compress v1.18.7 // indirect + github.com/kr/text v0.2.0 // indirect github.com/lufia/plan9stats v0.0.0-20260330125221-c963978e514e // indirect github.com/magiconair/properties v1.8.10 // indirect github.com/mitchellh/go-wordwrap v1.0.1 // indirect diff --git a/go.sum b/go.sum index 9db63995f..20ad857f5 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ cel.dev/expr v0.25.3 h1:A2jO8jwOugrrovveCWfj0KEZOfqiLgAcwjpHPhzIGw0= cel.dev/expr v0.25.3/go.mod h1:hrXvqGP6G6gyx8UAHSHJ5RGk//1Oj5nXQ2NI02Nrsg4= cloud.google.com/go v0.123.0 h1:2NAUJwPR47q+E35uaJeYoNhuNEM9kM8SjgRgdeOJUSE= cloud.google.com/go v0.123.0/go.mod h1:xBoMV08QcqUGuPW65Qfm1o9Y4zKZBpGS+7bImXLTAZU= -cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a h1:jW1ZLFy9oa5ai8LOI40z6s+DL1cO/L1KSm9hVGnibn8= -cloud.google.com/go/auth v0.24.1-0.20261001053825-dbc26066f70a/go.mod h1:1dmBXa9xfBvBr39ZnDUDkaaA91ccq9CcqENDgJA4d7c= +cloud.google.com/go/auth v0.24.1 h1:NTmd2iJb4DsGIav0KiqFMYI88nMshA2wnFMBatRmoCs= +cloud.google.com/go/auth v0.24.1/go.mod h1:1dmBXa9xfBvBr39ZnDUDkaaA91ccq9CcqENDgJA4d7c= cloud.google.com/go/auth/oauth2adapt v0.3.0 h1:FY8oSZpCYoUNv6QxVODuMjQz4IlSOVeiQtZ08vLPz88= cloud.google.com/go/auth/oauth2adapt v0.3.0/go.mod h1:7+2uCm7++XFO+/lN06c2HXpDXb/NMNn2/UwyBPbTnkk= cloud.google.com/go/compute v1.74.0 h1:GxtfiSN4hoA/938OAruNaZ746F6rUtZNyDQvXcXutrA= @@ -28,10 +28,10 @@ cloud.google.com/go/resourcemanager v1.16.0 h1:aDf2RuuQ1Z+kbHHDzHyJNMHPGRFK+cMYf cloud.google.com/go/resourcemanager v1.16.0/go.mod h1:Hn4HPkLRnTuiUhFEFJg736Brt7BwlS84xYU06sc3STc= cloud.google.com/go/secretmanager v1.23.0 h1:tUmPWfKapcrCsLXdJQIokbJamK5leTGAyxVhPXG1x90= cloud.google.com/go/secretmanager v1.23.0/go.mod h1:xDP/NNLKgQGGKcOenNnflIPW49MBj5w1EcakXjRLZHg= -cloud.google.com/go/storage v1.62.3 h1:SZq1t23NCI+e96dH77Dg3PEfsNNEjqO8zE5AnD8gVD0= -cloud.google.com/go/storage v1.62.3/go.mod h1:cpYz/kRVZ+UQAF1uHeea10/9ewcRbxGoGNKsS9daSXA= -cloud.google.com/go/trace v1.11.7 h1:kDNDX8JkaAG3R2nq1lIdkb7FCSi1rCmsEtKVsty7p+U= -cloud.google.com/go/trace v1.11.7/go.mod h1:TNn9d5V3fQVf6s4SCveVMIBS2LJUqo73GACmq/Tky0s= +cloud.google.com/go/storage v1.70.0 h1:LQNHn0rRF94jVFfF3e416IzdQqIufa5GfF6HilQxb80= +cloud.google.com/go/storage v1.70.0/go.mod h1:W84cNa070TZIegvTW67CBY+f5rxux8EJMb7YyiHdrls= +cloud.google.com/go/trace v1.16.0 h1:GmQovzFc5F0CNfl0VLgL64aoTtu7xsM0YajW2GlG9+E= +cloud.google.com/go/trace v1.16.0/go.mod h1:r+bdAn16dKLSV1G2D5v3e58IlQlizfxWrUfjx7kM7X0= dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6 h1:He8afgbRMd7mFxO99hRNu+6tazq8nFF9lIwo9JFroBk= @@ -48,20 +48,18 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0/go.mod h1:oZ73p8dR7aZI+TJo5Ul92oCoVubMYPBo39eTsWa0AiQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0 h1:4JHm1qT4VKYZ8aGP1dJBg+wk/WgN/KitLNtQs6jFNfA= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0/go.mod h1:GBFVpPKOTooUG7VM93ip046AqLf4qubcIEsU3js4v3g= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 h1:QfV5XZt6iNa2aWMAt96CZEbfJ7kgG/qYIpq465Shr5E= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0/go.mod h1:uYt4CfhkJA9o0FN7jfE5minm/i4nUE4MjGUJkzB6Zs8= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 h1:pTIng5JZfGKPA4WT8QjEPGOD5KK2CoCBkecWgtq3Cuc= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0/go.mod h1:0vCBR1wgGwZeGmloJ+eCWIZF2S47grTXRzj2mftg2Nk= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 h1:LkHbJbgF3YyvC53aqYGR+wWQDn2Rdp9AQdGndf9QvY4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0/go.mod h1:QyiQdW4f4/BIfB8ZutZ2s+28RAgfa/pT+zS++ZHyM1I= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0 h1:TAbicMLAaCP73UAoRwAoVh0DVuyzdWT/psQr4pG1vHY= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.2.0/go.mod h1:a1Pzix6xp1+Y9/hzJUAsx81QcUOHWMLgbcRtYTbdFuw= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0 h1:mTrlTrd4rdq32sUpDZhKJw8pfHaAqaEhZTuGH4WMfDQ= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2 v2.7.0/go.mod h1:M7VOO9cI4UMIkZGo+a5RS9HcsQeQPRQ104Py9Vug3KU= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal v1.1.2 h1:mLY+pNLjCUeKhgnAJWAKhEUQM+RJQo2H1fuGSw1Ky1E= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal v1.1.2/go.mod h1:FbdwsQ2EzwvXxOPcMFYO8ogEc9uMMIj3YkmCdXdAFmk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0 h1:2qsIIvxVT+uE6yrNldntJKlLRgxGbZ85kgtz5SNBhMw= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0/go.mod h1:AW8VEadnhw9xox+VaVd9sP7NjzOAnaZBLRH6Tq3cJ38= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 h1:zp+znRAHKLSewbw+WWKIMgCaFNxEXt9AwjxmW5fCnck= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0/go.mod h1:nEvLUni7GO5ukfEYtmrUfz08Puqd2FP9d8sCZazm5W4= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0 h1:EkL5dmUoy1OlzVfsbkcHayOvOJgheyRYL3wM/RHizzg= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.3.0/go.mod h1:DiazWkJHUUKUZGpIdV7JhDTjebBxdfsJ386dE5w7G3o= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 h1:0OO/3K+SKt45gXiOU4gHRILOLeNOUZdqeNO47Mq6iN8= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0/go.mod h1:2FDnHkGwh1BFK1ZQt+iDJU47Cg9Y28F0W3FSI389NOA= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= @@ -86,20 +84,20 @@ github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1 h1:0O6j18nQDo github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1/go.mod h1:xdYAf5bjkOpsd3auA8riiJW4vneBubt9caavL7626jQ= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0 h1:bN1gA3of5bXtbnLsRPrwfmbbe7A5UWFlcTHseujLnpc= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.35.0/go.mod h1:Yj5vHEz/aAepZGliRJsA6uvHAVAQyEwajq9ORCHPxzM= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0 h1:UnDZ/zFfG1JhH/DqxIZYU/1CUAlTUScoXD/LcM2Ykk8= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.55.0/go.mod h1:IA1C1U7jO/ENqm/vhi7V9YYpBsp+IMyqNrEN94N7tVc= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.55.0 h1:7t/qx5Ost0s0wbA/VDrByOooURhp+ikYwv20i9Y07TQ= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.55.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0 h1:0s6TxfCu2KHkkZPnBfsQ2y5qia0jl3MMrmBhu3nCOYk= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043 h1:aNxMUAEefkT973eGTUbVppd1X99zLKWRUYmJWfO1BUI= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026123645-88b95b64b043/go.mod h1:rEJErEsL40Np/ttf0RDoNNC2XQsN1GmWyFf34y0JLD8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043 h1:UjP0mxD+iv/ZIZg6hIm6dWHILzNX+0T/WCgiDUk5c+8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026123645-88b95b64b043/go.mod h1:yVHEL5ceNS/CsDatzLH0+Qmwtoi7yY4tIHvqyvhdtig= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043 h1:uhHCm5B8G6sGGoRCOyUp41tbza4W3pkh2EeUwT8UVKo= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026123645-88b95b64b043/go.mod h1:zA51HKXtBvHHMYd+owRDmn4p8ialZrLFzI9I052uUmo= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043 h1:ryqPNyihinNu9065Ysi4c9sWP90yX9jeBP7fDOrO6jw= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026123645-88b95b64b043/go.mod h1:/HxBqXBb5v69+EHLsibHnekkEYRP8TvyCmT6d9QRxt0= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0 h1:jLdiS1vO+XJFyDSWRHBx56r4s/NNtcl5J6KyCcWUX/w= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.57.0/go.mod h1:8lmpHY+1VRoteiOwyrQMDt1YGXOrFKCz+1wJW7n3ODY= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.57.0 h1:cSjUzZ7KU8hicTgzaSv9NmSyM9fTVK3y5lsBUl3wOis= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.57.0/go.mod h1:dzcEjy1WJ0Q4u9twNR3LcLhNoYMRCrMCMafpxa0TjPQ= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0 h1:RoO5+d7uCmDqovLrHCr2/BuViUXvdcrNxyNM1pN9dDQ= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0/go.mod h1:YqwkQPrWSC7+byyc1VlKbWLBF5JsW5IoL6xUkemYSXk= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353 h1:PpC722KByQTXdsdUIXuBzRXpklEu3+SLKfMipfPOtPg= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261103222717-61111428a353/go.mod h1:pseCFRgm1rzBKomktjw6E0ayxmVE7XfHHiOmew4ka+g= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353 h1:91krJUF0e9AC0Y0mc6nAVi4MsMtGiZVDlSgbZ/VT+9E= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261103222717-61111428a353/go.mod h1:NjNxaaNeYU3CTAcWUCOxaN+NeY7aveSyI/jNXEKz7/c= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353 h1:jCiXQIXO42bkvQPEaZ0IIQBzxGz3BPptB74cIumdiCo= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261103222717-61111428a353/go.mod h1:X+73EdStvYV6pqEVZZFyMfLG1frORlO9B1IsuK0LfOs= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353 h1:faFzHnGyaYgdN3rU0Ukkq6hHXjpOEcaX6W82Mz+DfnM= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261103222717-61111428a353/go.mod h1:aj4d0YEmBg1A46rIXKG+GkTFc1d/xwmveJ97kDD1oPs= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/agext/levenshtein v1.2.1 h1:QmvMAjj2aEICytGiWzmxoE0x2KZvE0fvmqMOfy2tjT8= @@ -186,6 +184,7 @@ github.com/coreos/go-oidc/v3 v3.21.0 h1:wZo4Q9Pum8dYEj0eMUPrqR+kvuGkeUplbLpNCkBq github.com/coreos/go-oidc/v3 v3.21.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= +github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -349,8 +348,8 @@ go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.69.0 h1:8tvICD4 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.69.0/go.mod h1:z9+yiacE0IHRqM4qFfkbt/JYlmYXgss8GY/jXoNuPJI= go.opentelemetry.io/otel v1.47.0 h1:j7ALJ/zgkS7Z6aeJW09p8VC9804bC+PpeTfCD4XPnOM= go.opentelemetry.io/otel v1.47.0/go.mod h1:8wS9O2qfXrYrzp6hIF/HOYJJf/wIhFPhR2xLuP+iXQU= -go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.43.0 h1:TC+BewnDpeiAmcscXbGMfxkO+mwYUwE/VySwvw88PfA= -go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.43.0/go.mod h1:J/ZyF4vfPwsSr9xJSPyQ4LqtcTPULFR64KwTikGLe+A= +go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.45.0 h1:dm9iyzn6tioYZtwqaiBSU0TSI8Yu/8dTIbfG0+B49DY= +go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.45.0/go.mod h1:xAvxYjYK28qvt+yu4BYZ/zMmAjwMXINXD6JiMyeB8iI= go.opentelemetry.io/otel/log v1.47.0 h1:cOTS1CcLbSQeZKanGJ+0JpF/+t4PELi3O3bbl2lqCcI= go.opentelemetry.io/otel/log v1.47.0/go.mod h1:9byitSQ5pLC6PpqwGXjqdMKya6ZTswHRZh2vvXT33nw= go.opentelemetry.io/otel/metric v1.47.0 h1:4PptaldXx3Eat1XjMZ68pPJEs5wrhlemctZE9a3UdWY= diff --git a/internal/api/freshness_held_scope_test.go b/internal/api/freshness_held_scope_test.go new file mode 100644 index 000000000..d9ef2c7e8 --- /dev/null +++ b/internal/api/freshness_held_scope_test.go @@ -0,0 +1,88 @@ +package api + +import ( + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// GET /api/recommendations/freshness is AuthUser and the state row is global, +// so held_account_ids must only name accounts the caller may see. + +const ( + heldInScope = "11111111-1111-1111-1111-111111111111" + heldOutScope = "22222222-2222-2222-2222-222222222222" +) + +func heldFreshness(ids ...string) *config.RecommendationsFreshness { + return &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(ids), HeldAccountIDs: ids, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}, + }} +} + +func heldIDsFor(t *testing.T, accounts []string, stored *config.RecommendationsFreshness) []string { + t.Helper() + f := newRefreshFixture(t, viewRecommendationsOnly(), accounts) + f.store.ExpectedCalls = nil + f.store.On("GetRecommendationsFreshness", mock.Anything).Return(stored, nil) + res, err := NewRouter(f.handler).Route(t.Context(), "GET", "/api/recommendations/freshness", scopedRequest("")) + require.NoError(t, err) + got, ok := res.(*config.RecommendationsFreshness) + require.True(t, ok, "got %T", res) + require.NotNil(t, got.LastCollectionPartial) + return got.LastCollectionPartial.Providers[0].HeldAccountIDs +} + +func TestFreshness_HeldAccountsScopedToCaller(t *testing.T) { + stored := func() *config.RecommendationsFreshness { return heldFreshness(heldInScope, heldOutScope) } + + t.Run("scoped caller sees only its own held account", func(t *testing.T) { + assert.Equal(t, []string{heldInScope}, heldIDsFor(t, []string{heldInScope}, stored())) + }) + t.Run("unrestricted caller sees all", func(t *testing.T) { + assert.Equal(t, []string{heldInScope, heldOutScope}, heldIDsFor(t, nil, stored())) + }) + t.Run("caller with no matching accounts sees none", func(t *testing.T) { + assert.Empty(t, heldIDsFor(t, []string{"33333333-3333-3333-3333-333333333333"}, stored())) + }) + t.Run("the stored value is not mutated", func(t *testing.T) { + s := stored() + heldIDsFor(t, []string{heldInScope}, s) + assert.Equal(t, []string{heldInScope, heldOutScope}, s.LastCollectionPartial.Providers[0].HeldAccountIDs) + }) +} + +// A scope that cannot be established must fail closed: an error, never the +// unfiltered held IDs. +func TestFreshness_ScopeLookupFailureFailsClosed(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() + mockAuth.grantPermissionsScoped(viewRecommendationsOnly(), []string{heldInScope}) + kept := mockAuth.ExpectedCalls[:0] + for _, c := range mockAuth.ExpectedCalls { + if c.Method != "GetAllowedAccountsAPI" { + kept = append(kept, c) + } + } + mockAuth.ExpectedCalls = kept + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, errors.New("auth store unavailable")) + f := newRefreshFixtureWithAuth(t, mockAuth) + f.store.ExpectedCalls = nil + f.store.On("GetRecommendationsFreshness", mock.Anything).Return(heldFreshness(heldInScope, heldOutScope), nil) + + res, err := NewRouter(f.handler).Route(t.Context(), "GET", "/api/recommendations/freshness", scopedRequest("")) + + require.Error(t, err, "a failed scope lookup must not produce a payload: got %v", res) + assert.Nil(t, res, "nothing, filtered or not, may be returned") + assert.NotContains(t, err.Error(), heldOutScope) + assert.NotContains(t, err.Error(), heldInScope) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a lookup failure is a server error, not a client one") +} diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 3a55527ef..8f8b5060d 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -4,6 +4,7 @@ package api import ( "context" "fmt" + "slices" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -338,22 +339,52 @@ func stampComputeCapacity(rec *config.RecommendationRecord) { // null when unknown; issue #756), computed from the schedule rather than // stored. // -// Gated by view:recommendations permission inside the handler. The route -// itself inherits AuthAdmin (the router default) so the permission check -// is currently defense-in-depth, matching the pattern used by other -// scoped handlers in this package. +// The route is AuthUser: any caller holding view:recommendations reaches it, +// so the held account IDs in the payload are narrowed to the caller's allowed +// accounts (scopeHeldAccounts). func (h *Handler) getRecommendationsFreshness(ctx context.Context, req *events.LambdaFunctionURLRequest) (*config.RecommendationsFreshness, error) { - if _, err := h.requirePermission(ctx, req, "view", "recommendations"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "recommendations") + if err != nil { return nil, err } freshness, err := h.config.GetRecommendationsFreshness(ctx) if err != nil { return nil, fmt.Errorf("failed to get recommendations freshness: %w", err) } - if freshness != nil { - freshness.NextCollectionAt = h.NextCollectionAt(h.clock()) + if freshness == nil { + return nil, nil + } + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + scoped := scopeHeldAccounts(freshness, scope) + scoped.NextCollectionAt = h.NextCollectionAt(h.clock()) + return scoped, nil +} + +// scopeHeldAccounts returns a copy of f whose held_account_ids name only +// accounts the scope allows. The state row is global, so naming an account the +// caller cannot see would leak that it exists. An unrestricted scope sees all +// of it; an account-less (ambient) hold has no IDs to show a restricted caller. +func scopeHeldAccounts(f *config.RecommendationsFreshness, scope auth.AccountScope) *config.RecommendationsFreshness { + out := *f + if scope.AllowsAll() || f.LastCollectionPartial == nil { + return &out + } + partial := *f.LastCollectionPartial + partial.Providers = slices.Clone(partial.Providers) + for i := range partial.Providers { + var allowed []string + for _, id := range partial.Providers[i].HeldAccountIDs { + if scope.Allows(id, "") { + allowed = append(allowed, id) + } + } + partial.Providers[i].HeldAccountIDs = allowed } - return freshness, nil + out.LastCollectionPartial = &partial + return &out } // getRecommendationDetail returns the per-id drill-down payload backing diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ef4e29b36..cb971c99e 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -6985,8 +6985,11 @@ components: nullable: true description: > Non-null when the most recent persisted collection kept rows from - at least one incomplete sweep. Cleared by the next collection whose - sweeps are all complete. Carries counts and reason categories only. + at least one incomplete sweep, or while a pricing hold carries + forward from an earlier collection. Cleared by the next collection + whose sweeps are all complete and that leaves no hold. Carries counts + and reason categories, plus held_account_ids filtered to the + caller's account scope. required: [providers] properties: providers: @@ -7026,12 +7029,25 @@ components: description: > Failure count per reason category. Keys are drawn from a closed set: permission_denied, throttled, timeout, canceled, not_found, - provider_unavailable, other. + provider_unavailable, pricing_unavailable, other. additionalProperties: type: integer example: permission_denied: 2 throttled: 1 + held_account_ids: + type: array + description: > + Platform cloud-account IDs (not cloud account numbers) whose + previous-cycle Azure Advisor rows are held because the price + catalog was unreachable (reason pricing_unavailable). Their + reservations cannot be purchased until a collection re-prices them. + The nil UUID (00000000-0000-0000-0000-000000000000) stands for the + ambient identity, which has no registered account. It is returned + only to callers with an unrestricted account scope. + items: + type: string + format: uuid # -- Plans -------------------------------------------------------------- PlansResponse: diff --git a/internal/api/purchase_currency_test.go b/internal/api/purchase_currency_test.go index a0e6c161c..170cb7765 100644 --- a/internal/api/purchase_currency_test.go +++ b/internal/api/purchase_currency_test.go @@ -151,3 +151,36 @@ func TestEnforcePurchaseConstraints_NonUSDBackendErrorPropagates(t *testing.T) { assert.False(t, isClient) mockAuth.AssertNumberOfCalls(t, "HasPermissionForConstraintsAPI", 1) } + +// A priced Azure Advisor reservation (go#663) carries USD, so the cap applies +// to its total commitment (upfront plus monthly over the term), while an +// unpriced Savings Plan row with no currency is still refused under a cap. +func TestEnforcePurchaseConstraints_AzureAdvisorPricedUSDCapAppliesUnpricedSPRefused(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + monthly := 50.0 + priced := config.RecommendationRecord{ + ID: "adv-vm", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, MonthlyCost: &monthly, CommitmentCurrency: "USD", + } + require.InDelta(t, 700, priced.TotalCommitment(), 0.001) + unpriced := config.RecommendationRecord{ + ID: "adv-sp", Provider: "azure", Service: "savingsplans", Region: "westeurope", + Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 900, + } + + require.NoError(t, handlerWithCap(auth.ActionExecute, 1000).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{priced})) + + err := handlerWithCap(auth.ActionExecute, 600).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{priced}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "exceeds the constraints", "the cap is compared against the 700 USD total, not upfront alone") + assert.NotContains(t, ce.Error(), "USD spend cap") + + requireCurrencyRefusal(t, handlerWithCap(auth.ActionExecute, 1000).enforcePurchaseConstraints(ctx, session, auth.ActionExecute, + []config.RecommendationRecord{unpriced}), "unknown") +} diff --git a/internal/api/ri_exchange_refusals.go b/internal/api/ri_exchange_refusals.go index 537802c17..364c10658 100644 --- a/internal/api/ri_exchange_refusals.go +++ b/internal/api/ri_exchange_refusals.go @@ -3,6 +3,7 @@ package api import ( "errors" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" ) @@ -17,12 +18,16 @@ import ( // aliased in provider_wiring.go): a source reservation's applied scope // cannot be carried onto the purchased reservation, or the sources mix // scopes. Returned by CalculateExchange before any Azure call. +// - common.ErrCurrencyNotAccepted: the billing currency a provider +// reported is missing or not USD (Azure CalculateExchange, wrapped with +// %w), so no session is returned and nothing can be executed. // -// Both describe the request or the account it targets, not a server fault, -// and both prove the exchange did not happen. +// These describe the request or the account it targets, not a server fault, +// and all prove the exchange did not happen. var exchangeRefusals = []error{ exchange.ErrQuoteNotUSD, errAzureUnsupportedAppliedScope, + common.ErrCurrencyNotAccepted, } // exchangeRefusalError returns a 422 carrying err's message when err wraps diff --git a/internal/api/ri_exchange_refusals_test.go b/internal/api/ri_exchange_refusals_test.go index 14f5d1949..c59982f1a 100644 --- a/internal/api/ri_exchange_refusals_test.go +++ b/internal/api/ri_exchange_refusals_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -77,17 +78,20 @@ func TestExecuteExchange_NonUSDQuoteRefusedAs422(t *testing.T) { func TestMapExchangeErrors_Refusals(t *testing.T) { notUSD := fmt.Errorf("aborting exchange at accept time: %w", fmt.Errorf("%w: quote is in %q", exchange.ErrQuoteNotUSD, "EUR")) scope := fmt.Errorf("azure: CalculateExchange: sources[0]: %w", azurecompute.ErrUnsupportedAppliedScope) + currency := fmt.Errorf("azure: CalculateExchange: calculateExchange bills in EUR but the expected currency is USD; refusing to exchange: %w", common.ErrCurrencyNotAccepted) mappers := map[string]func(string, error) error{ "aws": mapAWSExchangeError, "azure": mapAzureExchangeError, } for name, mapErr := range mappers { - for _, refusal := range []error{notUSD, scope} { + for _, refusal := range []error{notUSD, scope, currency} { t.Run(name+"/"+refusal.Error(), func(t *testing.T) { ce, ok := IsClientError(mapErr("op failed", refusal)) require.True(t, ok) assert.Equal(t, 422, ce.code) assert.Equal(t, refusal.Error(), ce.message) + assert.Equal(t, config.RIExchangeDirectStatusFailed, directExchangeFailureStatus(ce), + "a refusal happens before the provider commit call, so the audit row is failed, not unknown") }) } t.Run(name+"/other", func(t *testing.T) { diff --git a/internal/config/azure_hold_carry.go b/internal/config/azure_hold_carry.go new file mode 100644 index 000000000..077a8c3ad --- /dev/null +++ b/internal/config/azure_hold_carry.go @@ -0,0 +1,116 @@ +package config + +import ( + "slices" + + "github.com/google/uuid" +) + +// ambientAccountKey is the account_key of rows with no registered account. +var ambientAccountKey = uuid.Nil.String() + +// carryAzureHold keeps the Azure pricing hold alive for as long as the held +// rows are. Every upsert replaces last_collection_partial, but an account's +// Advisor rows stay at their old price until a later cycle re-prices them, so +// the previous hold (prev, read under the state row lock) is merged into this +// cycle's partial: accounts re-priced now (in collects) drop out, newly held +// ones are added, the rest carry over. That covers another outage, an outright +// Azure failure and a run that skips Azure. +// +// A hold lasts only while stored Azure rows it protects exist: stored holds the +// account_key of every provider=azure row, read in the same transaction. A +// deleted account's rows cascade and pruned rows disappear, which ends the +// hold; a disabled account keeps its rows, so it stays held. A hold with no +// account IDs (an unresolved ambient identity) covers the ambient rows, keyed +// by the nil UUID, and carries until an ambient Azure collection succeeds or +// none remain. Once the hold also names accounts, the ambient part is recorded +// as the nil UUID in the held IDs, so it outlives the named accounts' re-pricing. +func carryAzureHold(prev, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect, stored map[string]bool) *RecommendationsCollectionPartial { + held := previousAzureHold(prev) + if held == nil { + return partial + } + carried, unresolved := stillHeld(held, collects, stored) + if len(carried) == 0 && !unresolved { + return partial + } + if unresolved { + carried = append(carried, ambientAccountKey) + } + return mergeCarriedHold(partial, carried) +} + +// mergeCarriedHold adds the carried account IDs and the pricing_unavailable +// reason to partial's Azure entry, creating the entry when this cycle had none. +// An entry that already records the pricing reason with no account IDs is this +// cycle's ambient outage; it gains the nil UUID so it survives a named hold. +func mergeCarriedHold(partial *RecommendationsCollectionPartial, carried []string) *RecommendationsCollectionPartial { + if partial == nil { + partial = &RecommendationsCollectionPartial{} + } + idx := slices.IndexFunc(partial.Providers, func(p PartialProviderSweep) bool { return p.Provider == "azure" }) + if idx < 0 { + partial.Providers = append(partial.Providers, PartialProviderSweep{Provider: "azure", Reasons: map[string]int{}}) + idx = len(partial.Providers) - 1 + } + sweep := &partial.Providers[idx] + ownAmbient := false + if sweep.Reasons == nil { + sweep.Reasons = map[string]int{} + } + if sweep.Reasons[PartialReasonPricingUnavailable] == 0 { + sweep.Reasons[PartialReasonPricingUnavailable] = 1 + } else if len(sweep.HeldAccountIDs) == 0 { + carried = append(carried, ambientAccountKey) + ownAmbient = true + } + before := len(sweep.HeldAccountIDs) + held := slices.Concat(sweep.HeldAccountIDs, carried) + slices.Sort(held) + sweep.HeldAccountIDs = slices.Compact(held) + // Each newly held account adds to the incomplete count; this cycle's own + // ambient outage was already counted. + sweep.IncompleteAccounts += len(sweep.HeldAccountIDs) - before + if ownAmbient { + sweep.IncompleteAccounts-- + } + return partial +} + +// previousAzureHold returns the Azure entry that records a pricing hold, or nil. +func previousAzureHold(partial *RecommendationsCollectionPartial) *PartialProviderSweep { + if partial == nil { + return nil + } + for i := range partial.Providers { + sweep := &partial.Providers[i] + if sweep.Provider == "azure" && sweep.Reasons[PartialReasonPricingUnavailable] > 0 { + return sweep + } + } + return nil +} + +// stillHeld returns the previously held accounts this cycle did not re-price +// that still have stored Azure rows, and whether an account-less (ambient) hold is still +// unresolved. +func stillHeld(prev *PartialProviderSweep, collects []SuccessfulCollect, stored map[string]bool) (carried []string, unresolved bool) { + repriced := map[string]bool{} + ambientRepriced := false + for _, sc := range collects { + switch { + case sc.Provider != "azure": + case sc.CloudAccountID == nil: + ambientRepriced = true + repriced[ambientAccountKey] = true + default: + repriced[*sc.CloudAccountID] = true + } + } + for _, id := range prev.HeldAccountIDs { + if !repriced[id] && stored[id] { + carried = append(carried, id) + } + } + return carried, len(prev.HeldAccountIDs) == 0 && !ambientRepriced && stored[ambientAccountKey] +} diff --git a/internal/config/azure_hold_carry_test.go b/internal/config/azure_hold_carry_test.go new file mode 100644 index 000000000..8ad2f1cb5 --- /dev/null +++ b/internal/config/azure_hold_carry_test.go @@ -0,0 +1,111 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func holdOf(ids ...string) *RecommendationsCollectionPartial { + return &RecommendationsCollectionPartial{Providers: []PartialProviderSweep{{ + Provider: "azure", HeldAccountIDs: ids, Reasons: map[string]int{PartialReasonPricingUnavailable: 1}, + }}} +} + +func azureCollect(id *string) []SuccessfulCollect { + return []SuccessfulCollect{{Provider: "azure", CloudAccountID: id}} +} + +func TestCarryAzureHold(t *testing.T) { + a, b, c := "a", "b", "c" + stored := map[string]bool{"a": true, "b": true, "c": true} + ambient := map[string]bool{ambientAccountKey: true} + for name, tc := range map[string]struct { + prev *RecommendationsCollectionPartial + collects []SuccessfulCollect + partial *RecommendationsCollectionPartial + stored map[string]bool + want []string + wantHold bool + }{ + "no previous hold": {nil, nil, nil, stored, nil, false}, + "nothing re-priced": {holdOf("a", "b"), nil, nil, stored, []string{"a", "b"}, true}, + "one re-priced": {holdOf("a", "b"), azureCollect(&a), nil, stored, []string{"b"}, true}, + "both re-priced": {holdOf("a", "b"), append(azureCollect(&a), azureCollect(&b)...), nil, stored, nil, false}, + "other provider ignored": {holdOf("a"), []SuccessfulCollect{{Provider: "gcp", CloudAccountID: &a}}, nil, stored, []string{"a"}, true}, + "new hold is merged": {holdOf("a", "b"), nil, holdOf("c"), stored, []string{"a", "b", "c"}, true}, + "re-held account dedupes": {holdOf("a", "b"), nil, holdOf("a"), stored, []string{"a", "b"}, true}, + "account without rows drops": {holdOf("a", "b"), nil, nil, map[string]bool{"b": true}, []string{"b"}, true}, + "all rows gone drops": {holdOf("a"), nil, nil, map[string]bool{"c": true}, nil, false}, + "ambient carries while rows exist": {holdOf(), nil, nil, ambient, []string{ambientAccountKey}, true}, + "ambient survives account rows too": {holdOf(), azureCollect(&c), nil, map[string]bool{ambientAccountKey: true, "c": true}, []string{ambientAccountKey}, true}, + "ambient re-priced": {holdOf(), azureCollect(nil), nil, ambient, nil, false}, + "ambient drops with no ambient rows": {holdOf(), nil, nil, stored, nil, false}, + } { + t.Run(name, func(t *testing.T) { + got := carryAzureHold(tc.prev, tc.partial, tc.collects, tc.stored) + if !tc.wantHold { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + require.Len(t, got.Providers, 1) + assert.ElementsMatch(t, tc.want, got.Providers[0].HeldAccountIDs) + assert.Positive(t, got.Providers[0].Reasons[PartialReasonPricingUnavailable]) + }) + } +} + +// An ambient hold merged with a named account's hold must stay an ambient hold +// after the named account re-prices: the stale ambient rows are still there. +func TestCarryAzureHold_AmbientSurvivesNamedReprice(t *testing.T) { + c := "c" + stored := map[string]bool{ambientAccountKey: true, "c": true} + + // Cycle 1: the ambient hold carries while this cycle holds account c. + merged := carryAzureHold(holdOf(), holdOf("c"), nil, stored) + require.NotNil(t, merged) + + // Cycle 2: c re-prices, the ambient identity does not. + got := carryAzureHold(merged, nil, azureCollect(&c), stored) + require.NotNil(t, got, "ambient rows still exist, so the hold must stay") + assert.Contains(t, got.Providers[0].HeldAccountIDs, ambientAccountKey) + + // Cycle 3: the ambient identity re-prices, which ends the hold. + assert.Nil(t, carryAzureHold(got, nil, azureCollect(nil), stored)) +} + +// An ambient pricing outage in this cycle plus a carried named hold keeps both. +func TestCarryAzureHold_ThisCycleAmbientKeptWithCarriedNamed(t *testing.T) { + stored := map[string]bool{ambientAccountKey: true, "a": true} + got := carryAzureHold(holdOf("a"), holdOf(), nil, stored) + require.NotNil(t, got) + assert.ElementsMatch(t, []string{"a", ambientAccountKey}, got.Providers[0].HeldAccountIDs) +} + +// Carried accounts add to the incomplete count instead of being absorbed into a +// larger count that came from other failures this cycle. +func TestCarryAzureHold_IncompleteAccountsCountsEachCarriedAccount(t *testing.T) { + stored := map[string]bool{"a": true, "b": true, "x": true, ambientAccountKey: true} + for name, tc := range map[string]struct { + prev, partial *RecommendationsCollectionPartial + want int + }{ + "carried on top of other failures": {holdOf("a", "b"), withIncomplete(holdOf("x"), 3), 5}, + "own ambient outage counted once": {holdOf("a"), withIncomplete(holdOf(), 1), 2}, + "nothing this cycle": {holdOf("a", "b"), nil, 2}, + "ambient carried": {holdOf(), nil, 1}, + } { + t.Run(name, func(t *testing.T) { + got := carryAzureHold(tc.prev, tc.partial, nil, stored) + require.NotNil(t, got) + assert.Equal(t, tc.want, got.Providers[0].IncompleteAccounts) + }) + } +} + +func withIncomplete(p *RecommendationsCollectionPartial, n int) *RecommendationsCollectionPartial { + p.Providers[0].IncompleteAccounts = n + return p +} diff --git a/internal/config/azure_hold_concurrent_integration_test.go b/internal/config/azure_hold_concurrent_integration_test.go new file mode 100644 index 000000000..b1fe838d4 --- /dev/null +++ b/internal/config/azure_hold_concurrent_integration_test.go @@ -0,0 +1,82 @@ +//go:build integration +// +build integration + +package config_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// Two overlapping collections: one commits a pricing hold on X, the other +// (which started earlier and only re-prices Y) commits later. The later commit +// must not drop X's hold. The holder transaction stands in for the first +// collection mid-flight; the second upsert is provably queued behind it before +// the first commits. +func TestUpsertRecommendationsWithPartial_ConcurrentCollectionKeepsHold(t *testing.T) { + ctx := context.Background() + db := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB + store := config.NewPostgresStore(db) + acctX, acctY := uuid.NewString(), uuid.NewString() + seedRecommendationCloudAccount(ctx, t, store, acctX, "azure", "sub-hold-x") + seedRecommendationCloudAccount(ctx, t, store, acctY, "azure", "sub-hold-y") + + // The hold protects X's stored rows, which must exist for it to carry. + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().Add(-time.Hour), + []config.RecommendationRecord{azureAccountRec("row-x", acctX, "D2")}, nil)) + + holder, err := db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = holder.Rollback(ctx) }) + _, err = holder.Exec(ctx, `SELECT 1 FROM recommendations_state WHERE id = 1 FOR UPDATE`) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { + done <- store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, + []config.SuccessfulCollect{{Provider: "azure", CloudAccountID: &acctY}}, nil) + }() + _, finished := testhelpers.AwaitBlockedBy(ctx, t, db, testhelpers.BackendPID(ctx, t, holder), done) + require.False(t, finished, "the second collection must queue behind the first") + + hold, err := json.Marshal(&config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: 1, HeldAccountIDs: []string{acctX}, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}}) + require.NoError(t, err) + _, err = holder.Exec(ctx, `UPDATE recommendations_state SET last_collection_partial = $1::jsonb WHERE id = 1`, hold) + require.NoError(t, err) + require.NoError(t, holder.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial, "the later commit dropped the other run's hold") + require.Equal(t, []string{acctX}, fresh.LastCollectionPartial.Providers[0].HeldAccountIDs) +} + +// The stored-rows read must be limited to Azure: an ambient Azure hold ends when +// no Azure ambient rows remain, even if another provider's ambient rows exist. +func TestUpsertRecommendationsWithPartial_AmbientHoldIgnoresOtherProviderRows(t *testing.T) { + ctx := context.Background() + store := config.NewPostgresStore(testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB) + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().Add(-time.Hour), + []config.RecommendationRecord{ambientRec("aws-ambient", "aws", "m5.large")}, nil)) + ambientHold := &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: 1, Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}} + require.NoError(t, store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, nil, ambientHold)) + + require.NoError(t, store.UpsertRecommendations(ctx, time.Now().UTC(), nil, nil)) + + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial, "AWS ambient rows must not keep an Azure hold alive") +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 958c6227a..482b5a42f 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -692,6 +692,10 @@ type StoreInterface interface { // SuccessfulCollect for the per-row semantics. There is deliberately no // unscoped "replace everything" method (issue #159). UpsertRecommendations(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect) error + // UpsertRecommendationsWithPartial is UpsertRecommendations that also + // records the collection's incomplete-sweep summary (nil clears it) in the + // same transaction, so the rows and the marker commit or roll back together. + UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error // PruneStaleRecommendations deletes the rows of every (provider, // account) pair with no row collected in the last retentionDays days, // leaving every pair that collected inside the window untouched, and @@ -712,11 +716,6 @@ type StoreInterface interface { PageStoredRecommendations(ctx context.Context, q RecommendationPageQuery) (RecommendationPage, error) GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error) SetRecommendationsCollectionError(ctx context.Context, errMsg string) error - // SetRecommendationsCollectionPartial records (or, with nil, clears) the - // most recent collection's incomplete-sweep summary. - // UpsertRecommendations clears it, so a later complete collection drops - // the marker. - SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error // MarkCollectionStarted atomically sets last_collection_started_at = now // only when no in-flight collection is running (last_collection_started_at IS NULL // OR older than 5 minutes), and stamps a freshly generated owner token diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 5987c3fb4..cfc297312 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -45,6 +45,15 @@ const azurePricingMigrationVersion = 106 // insert. This collapses ambient and registered identities consistently // on both sides of the join. func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect) error { + return s.UpsertRecommendationsWithPartial(ctx, collectedAt, recs, successfulCollects, nil) +} + +// UpsertRecommendationsWithPartial is UpsertRecommendations that records the +// collection's incomplete-sweep summary in the same transaction. The previous +// Azure pricing hold is read under a row lock and merged into partial (see +// carryAzureHold), so overlapping collections cannot drop each other's hold +// and a failed read or write rolls the rows back with the marker. +func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error { tx, err := s.db.Begin(ctx) if err != nil { return fmt.Errorf("failed to begin tx: %w", err) @@ -72,6 +81,13 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t } } + // The state row lock is taken last, after the row writes, so the lock order + // is unchanged from the plain UPDATE this replaced. + partialJSON, markerErr := lockedMarkerJSON(ctx, tx, partial, successfulCollects) + if markerErr != nil { + return markerErr + } + // last_collected_at only moves forward (issue #111). Two collections can // overlap (cron and a user refresh), and the one that started first can // commit last; an unconditional SET would then move freshness backwards @@ -83,9 +99,9 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t UPDATE recommendations_state SET last_collected_at = GREATEST(last_collected_at, $1), last_collection_error = NULL, - last_collection_partial = NULL + last_collection_partial = $2::jsonb WHERE id = 1 - `, collectedAt); err != nil { + `, collectedAt, partialJSON); err != nil { return fmt.Errorf("failed to update state: %w", err) } @@ -95,6 +111,60 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t return nil } +// encodeCollectionPartial marshals the marker for the JSONB column; nil stays +// nil so the column is cleared. +func encodeCollectionPartial(partial *RecommendationsCollectionPartial) ([]byte, error) { + if partial == nil { + return nil, nil + } + encoded, err := json.Marshal(partial) + if err != nil { + return nil, fmt.Errorf("failed to encode collection partial: %w", err) + } + return encoded, nil +} + +// lockedMarkerJSON returns the encoded marker to write: partial with the +// previous Azure hold merged in under the state row lock. +func lockedMarkerJSON(ctx context.Context, tx pgx.Tx, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect) ([]byte, error) { + merged, err := carryHoldInTx(ctx, tx, partial, collects) + if err != nil { + return nil, err + } + return encodeCollectionPartial(merged) +} + +// carryHoldInTx locks the state row, then merges the previous Azure pricing +// hold into partial. The lock serializes this read-merge-write against every +// concurrent collection. An unreadable row or account list fails the tx. +func carryHoldInTx(ctx context.Context, tx pgx.Tx, partial *RecommendationsCollectionPartial, collects []SuccessfulCollect) (*RecommendationsCollectionPartial, error) { + var raw []byte + if err := tx.QueryRow(ctx, `SELECT last_collection_partial FROM recommendations_state WHERE id = 1 FOR UPDATE`).Scan(&raw); err != nil { + return nil, fmt.Errorf("failed to lock recommendations_state: %w", err) + } + prev := decodeCollectionPartial(raw) + if previousAzureHold(prev) == nil { + return partial, nil + } + rows, err := tx.Query(ctx, `SELECT DISTINCT account_key::text FROM recommendations WHERE provider = 'azure'`) + if err != nil { + return nil, fmt.Errorf("failed to list azure accounts with stored rows: %w", err) + } + defer rows.Close() + stored := map[string]bool{} + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + return nil, fmt.Errorf("failed to scan azure account key: %w", err) + } + stored[id] = true + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to list azure accounts with stored rows: %w", err) + } + return carryAzureHold(prev, partial, collects, stored), nil +} + // successfulCollectArrays converts the SuccessfulCollect slice into the two // parallel arrays the eviction unnest() expects. nil CloudAccountID becomes // uuid.Nil so the join matches the generated account_key column for @@ -434,7 +504,7 @@ func (s *PostgresStore) GetRecommendationsFreshness(ctx context.Context) (*Recom // decodeCollectionPartial decodes the last_collection_partial JSONB column. // NULL decodes to nil (the last collection was complete). The column is only -// ever written by SetRecommendationsCollectionPartial via json.Marshal, so an +// ever written by UpsertRecommendationsWithPartial via json.Marshal, so an // undecodable value means the row was edited by hand; it is logged and read // as nil rather than failing the whole freshness read, which the scheduler's // cache-TTL check and every dashboard load depend on. @@ -476,32 +546,6 @@ func (s *PostgresStore) SetRecommendationsCollectionError(ctx context.Context, e return nil } -// SetRecommendationsCollectionPartial records the most recent collection's -// incomplete-sweep summary (issue #87), or clears it when partial is nil. -// Like SetRecommendationsCollectionError it is called by the scheduler AFTER -// UpsertRecommendations has reset the column to NULL, so a complete -// collection leaves it cleared and only a partial one re-sets it. It touches -// no other column; in particular it must not clear last_collection_started_at -// (see SetRecommendationsCollectionError, issue #261). -func (s *PostgresStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error { - var raw []byte - if partial != nil { - encoded, err := json.Marshal(partial) - if err != nil { - return fmt.Errorf("failed to encode collection partial: %w", err) - } - raw = encoded - } - if _, err := s.db.Exec(ctx, ` - UPDATE recommendations_state - SET last_collection_partial = $1::jsonb - WHERE id = 1 - `, raw); err != nil { - return fmt.Errorf("failed to set collection partial: %w", err) - } - return nil -} - // MarkCollectionStarted atomically sets last_collection_started_at = NOW() // only when no in-flight collection is currently running. The WHERE clause // treats a started_at older than 5 minutes as stale (the scheduler Lambda diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 493da93aa..c7c4c74d3 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -225,7 +225,7 @@ func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) { } recordPartial := func() { t.Helper() - require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, partial)) + require.NoError(t, store.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), nil, nil, partial)) got, getErr := store.GetRecommendationsFreshness(ctx) require.NoError(t, getErr) require.Equal(t, partial, got.LastCollectionPartial) @@ -243,12 +243,6 @@ func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) { require.NoError(t, err) assert.Nil(t, fr.LastCollectionPartial, "the next persisted collection must clear the marker") assert.Nil(t, fr.LastCollectionError) - - recordPartial() - require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, nil)) - fr, err = store.GetRecommendationsFreshness(ctx) - require.NoError(t, err) - assert.Nil(t, fr.LastCollectionPartial, "a nil partial clears the column") } // TestPostgresStore_UpsertRecommendations_StoresAllTermVariants pins diff --git a/internal/config/types.go b/internal/config/types.go index 44a8cb09a..7401e528f 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -949,6 +949,11 @@ const ( PartialReasonNotFound = "not_found" PartialReasonProviderUnavailable = "provider_unavailable" PartialReasonOther = "other" + // PartialReasonPricingUnavailable marks accounts whose previous-cycle + // Advisor rows were kept because the Retail Prices catalog could not be + // reached. Those rows may carry an outdated price and must not be + // purchased until a good collection re-prices them. + PartialReasonPricingUnavailable = "pricing_unavailable" ) // PartialProviderSweep summarizes one provider's incomplete sweeps in a @@ -973,6 +978,12 @@ type PartialProviderSweep struct { SkippedRecommendations int `json:"skipped_recommendations"` // Reasons counts the failure causes by PartialReason* category. Reasons map[string]int `json:"reasons"` + // HeldAccountIDs lists the platform cloud-account UUIDs (not cloud + // account numbers) whose previous-cycle rows are held through a pricing + // outage; the purchase path refuses their recommendations. It can contain + // the nil UUID (all zeros) for the ambient identity, which has no account; + // the API shows it only to callers with an unrestricted account scope. + HeldAccountIDs []string `json:"held_account_ids,omitempty"` } // SuccessfulCollect identifies a (provider, account) pair whose collection diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index afcc07417..0c9db1eb9 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1876,6 +1876,15 @@ func (m *MockConfigStore) UpsertRecommendations(ctx context.Context, collectedAt return args.Error(0) } +func (m *MockConfigStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, partial *config.RecommendationsCollectionPartial) error { + m.record("UpsertRecommendationsWithPartial", ctx, collectedAt, recs, successfulCollects, partial) + if !isExpected(&m.Mock, "UpsertRecommendationsWithPartial") { + return nil + } + args := m.Called(ctx, collectedAt, recs, successfulCollects, partial) + return args.Error(0) +} + func (m *MockConfigStore) PruneStaleRecommendations(ctx context.Context, retentionDays int) (int64, error) { m.record("PruneStaleRecommendations", ctx, retentionDays) if !isExpected(&m.Mock, "PruneStaleRecommendations") { @@ -1974,15 +1983,6 @@ func (m *MockConfigStore) SetRecommendationsCollectionError(ctx context.Context, return args.Error(0) } -func (m *MockConfigStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *config.RecommendationsCollectionPartial) error { - m.record("SetRecommendationsCollectionPartial", ctx, partial) - if !isExpected(&m.Mock, "SetRecommendationsCollectionPartial") { - return nil - } - args := m.Called(ctx, partial) - return args.Error(0) -} - func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey) (*config.RIUtilizationCacheEntry, error) { m.record("GetRIUtilizationCache", ctx, key) if !isExpected(&m.Mock, "GetRIUtilizationCache") { diff --git a/internal/purchase/azure_pricing.go b/internal/purchase/azure_pricing.go index b328ff35c..c5b828b30 100644 --- a/internal/purchase/azure_pricing.go +++ b/internal/purchase/azure_pricing.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "slices" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -48,9 +49,44 @@ func (m *Manager) staleAzurePricingRefusal(ctx context.Context, exec *config.Pur return fmt.Errorf("%w: Azure recommendation %q no longer has the approved price", ErrStaleAzurePricing, rec.ID) } } + return m.heldAzurePricingRefusal(ctx, azure) +} + +// heldAzurePricingRefusal refuses Azure recs whose account's stored rows are +// held through a Retail Prices outage. Those rows are the previous cycle's, so +// they match the approved execution while their price may be days old. +// A rec without an account (an unresolved ambient identity) cannot be named in +// the hold, so any current Azure hold refuses it. Fails closed when the +// collection state cannot be read. +func (m *Manager) heldAzurePricingRefusal(ctx context.Context, azure []config.RecommendationRecord) error { + fresh, err := m.config.GetRecommendationsFreshness(ctx) + if err != nil { + return fmt.Errorf("%w: cannot read the latest collection state to confirm Azure prices are current: %w", ErrStaleAzurePricing, err) + } + if fresh == nil { + return fmt.Errorf("%w: the latest collection state is missing, so Azure prices cannot be confirmed current", ErrStaleAzurePricing) + } + if fresh.LastCollectionPartial == nil { + return nil + } + for _, sweep := range fresh.LastCollectionPartial.Providers { + if sweep.Provider != "azure" || sweep.Reasons[config.PartialReasonPricingUnavailable] == 0 { + continue + } + for i := range azure { + if azureAccountHeld(azure[i].CloudAccountID, sweep.HeldAccountIDs) { + return fmt.Errorf("%w: Azure prices for recommendation %q could not be refreshed (price catalog unavailable); "+ + "refresh recommendations after prices recover and submit a new purchase", ErrStaleAzurePricing, azure[i].ID) + } + } + } return nil } +func azureAccountHeld(accountID *string, held []string) bool { + return accountID == nil || *accountID == "" || slices.Contains(held, *accountID) +} + func azurePricingMatches(rec, stored config.RecommendationRecord) bool { if rec.Count <= 0 || stored.Count <= 0 { return false diff --git a/internal/purchase/azure_pricing_hold_test.go b/internal/purchase/azure_pricing_hold_test.go new file mode 100644 index 000000000..a9c4567b0 --- /dev/null +++ b/internal/purchase/azure_pricing_hold_test.go @@ -0,0 +1,75 @@ +package purchase + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func heldFreshness(heldIDs ...string) *config.RecommendationsFreshness { + return &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(heldIDs), HeldAccountIDs: heldIDs, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}, + }} +} + +// An Advisor row held through a pricing outage still matches the approved +// execution, because both carry the same old price. The collection state is +// what tells the purchase path the price may be days old. +func TestAzurePricingHold(t *testing.T) { + acct := "11111111-1111-1111-1111-111111111111" + other := "22222222-2222-2222-2222-222222222222" + cases := []struct { + name string + execAcct *string + fresh *config.RecommendationsFreshness + freshErr error + wantStale bool + }{ + {"held account refused", &acct, heldFreshness(acct), nil, true}, + {"hold cleared by a good cycle", &acct, &config.RecommendationsFreshness{}, nil, false}, + {"unheld account unaffected", &other, heldFreshness(acct), nil, false}, + {"unreadable hold state refuses", &acct, nil, errors.New("database unavailable"), true}, + {"missing collection state refuses", &acct, nil, nil, true}, + // The reason gate: only a pricing_unavailable sweep holds rows. A sweep that + // is partial for another reason (here throttling) must not refuse a purchase. + {"partial sweep without the pricing reason does not refuse", &acct, &config.RecommendationsFreshness{LastCollectionPartial: &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{Provider: "azure", HeldAccountIDs: []string{acct}, Reasons: map[string]int{config.PartialReasonThrottled: 1}}}, + }}, nil, false}, + {"unresolved ambient rec refused during an outage", nil, heldFreshness(), nil, true}, + // Fail-closed by design: a rec without an account cannot be matched to the + // hold list, so any Azure hold refuses it. + {"accountless rec refused while other accounts are held", nil, heldFreshness(acct), nil, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + base := config.RecommendationRecord{ID: "azure-held", Provider: "azure", Service: "compute", Region: "westeurope", + ResourceType: "Standard_D2s_v3", Count: 2, Term: 3, UpfrontCost: 200, Selected: true, CloudAccountID: tc.execAcct} + exec := armedExecution("azure", "compute", 0, "approved") + exec.Recommendations = []config.RecommendationRecord{base} + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}). + Return([]config.RecommendationRecord{base}, nil).Maybe() + store.On("GetRecommendationsFreshness", mock.Anything).Return(tc.fresh, tc.freshErr).Maybe() + + err := mgr.executeAndFinalize(context.Background(), exec) + + if tc.wantStale { + require.ErrorIs(t, err, ErrStaleAzurePricing) + assert.Zero(t, recorder.count()) + assert.Equal(t, "failed", exec.Status) + return + } + require.NoError(t, err) + assert.Equal(t, 1, recorder.count()) + }) + } +} diff --git a/internal/scheduler/advisor_pricing_outage_integration_test.go b/internal/scheduler/advisor_pricing_outage_integration_test.go new file mode 100644 index 000000000..d0e925a36 --- /dev/null +++ b/internal/scheduler/advisor_pricing_outage_integration_test.go @@ -0,0 +1,131 @@ +//go:build integration + +package scheduler + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Skip reasons as the go#663 Advisor pricing records them (see pricingOutageReasons). +var advisorOutageSkips = []string{ + `advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, +} + +// collectAzureAmbientCycle runs one ambient Azure collection against store +// and persists it the way a scheduled cycle does. +func collectAzureAmbientCycle(t *testing.T, store config.StoreInterface, recs []common.Recommendation, sweepErr error) []config.RecommendationRecord { + t.Helper() + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") + ctx := context.Background() + + recClient := new(MockRecommendationsClient) + recClient.On("GetAllRecommendations", mock.Anything).Return(recs, sweepErr).Once() + prov := new(MockProvider) + prov.On("GetRecommendationsClient", mock.Anything).Return(recClient, nil).Once() + factory := new(MockProviderFactory) + factory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.Anything).Return(prov, nil).Once() + s := &Scheduler{config: store, providerFactory: factory} + + got, ids, gap, err := s.collectAzureRecommendations(ctx, &config.GlobalConfig{}) + require.NoError(t, err) + s.persistCollection(ctx, got, expandSuccessfulCollects("azure", ids), nil, appendPartialProvider(nil, "azure", gap)) + return got +} + +func storedAzureIDs(t *testing.T, store config.StoreInterface) []string { + t.Helper() + rows, err := store.ListStoredRecommendations(context.Background(), config.RecommendationFilter{}) + require.NoError(t, err) + ids := make([]string, 0, len(rows)) + for _, row := range rows { + ids = append(ids, row.ID) + } + return ids +} + +func seedAdvisorRow(t *testing.T, store config.StoreInterface) { + t.Helper() + require.NoError(t, store.UpsertRecommendations(context.Background(), time.Now().Add(-time.Hour), []config.RecommendationRecord{ + {ID: "prev-advisor-row", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Savings: 40, Count: 1, Term: 12, Payment: "upfront"}, + }, nil)) +} + +// An Advisor pricing outage skips every row; the previous cycle's rows must +// survive it, a later good cycle replaces them, and a genuine empty cycle +// still evicts. +func TestAzureAdvisorPricingOutage_HoldsEviction(t *testing.T) { + ctx := context.Background() + newStore := func(t *testing.T) config.StoreInterface { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + seedAdvisorRow(t, store) + return store + } + + t.Run("outage keeps prior rows", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + require.Contains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) + + t.Run("later good cycle replaces them", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + got := collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + require.NotEmpty(t, got) + ids := storedAzureIDs(t, store) + require.NotContains(t, ids, "prev-advisor-row") + require.Len(t, ids, len(got)) + }) + + t.Run("genuine empty cycle still evicts", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, nil) + require.NotContains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) + + t.Run("unrelated skip reasons still evict", func(t *testing.T) { + store := newStore(t) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: []string{"advisor: row is in EUR, not USD"}}) + require.NotContains(t, storedAzureIDs(t, store), "prev-advisor-row") + }) +} + +// The freshness payload names the account whose Advisor rows are held through +// a pricing outage, with the closed pricing_unavailable reason, and a good +// cycle clears it. The purchase path reads this to refuse the held rows. +func TestAzureAdvisorPricingOutage_FreshnessListsHeldAccount(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + acct := config.CloudAccount{ID: uuid.NewString(), Name: "host", Provider: "azure", ExternalID: "sub-a", AzureSubscriptionID: "sub-a", Enabled: false} + require.NoError(t, store.CreateCloudAccount(ctx, &acct)) + + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + require.Len(t, fresh.LastCollectionPartial.Providers, 1) + sweep := fresh.LastCollectionPartial.Providers[0] + require.Equal(t, "azure", sweep.Provider) + require.Equal(t, []string{acct.ID}, sweep.HeldAccountIDs) + require.Equal(t, 1, sweep.IncompleteAccounts) + require.Positive(t, sweep.Reasons[config.PartialReasonPricingUnavailable]) + + collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + fresh, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial, "a good cycle clears the hold") +} diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go new file mode 100644 index 000000000..bf34803ef --- /dev/null +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -0,0 +1,209 @@ +//go:build integration + +package scheduler + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func heldPartial(ids ...string) *config.RecommendationsCollectionPartial { + return &config.RecommendationsCollectionPartial{Providers: []config.PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: len(ids), HeldAccountIDs: ids, + Reasons: map[string]int{config.PartialReasonPricingUnavailable: 1}, + }}} +} + +func heldAzureAccounts(t *testing.T, store config.StoreInterface) []string { + t.Helper() + fresh, err := store.GetRecommendationsFreshness(context.Background()) + require.NoError(t, err) + if fresh.LastCollectionPartial == nil { + return nil + } + for _, sweep := range fresh.LastCollectionPartial.Providers { + if sweep.Provider == "azure" && sweep.Reasons[config.PartialReasonPricingUnavailable] > 0 { + return sweep.HeldAccountIDs + } + } + return nil +} + +func registerAzure(t *testing.T, store config.StoreInterface) string { + t.Helper() + acct := &config.CloudAccount{Name: "azure-" + uuid.NewString(), Provider: "azure", ExternalID: uuid.NewString(), Enabled: true} + require.NoError(t, store.CreateCloudAccount(context.Background(), acct)) + return acct.ID +} + +// seedAzureAccountRows stores an old Azure row for each account, the rows a +// hold protects. +func seedAzureAccountRows(t *testing.T, store config.StoreInterface, accounts ...string) { + t.Helper() + recs := make([]config.RecommendationRecord, 0, len(accounts)) + for _, id := range accounts { + recs = append(recs, config.RecommendationRecord{ID: "row-" + id, CloudAccountID: &id, Provider: "azure", Service: "compute", + Region: "westeurope", ResourceType: "Standard_D2s_v3", Savings: 40, Count: 1, Term: 12, Payment: "upfront"}) + } + require.NoError(t, store.UpsertRecommendations(context.Background(), time.Now().Add(-time.Hour), recs, nil)) +} + +// The hold marker lasts as long as the held rows: it survives every cycle that +// does not re-price the account, and is dropped only by a cycle that does. +func TestAzureHoldCarriedAcrossCycles(t *testing.T) { + ctx := context.Background() + var acctA, acctB string + newScheduler := func(t *testing.T) (*Scheduler, config.StoreInterface) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + acctA, acctB = registerAzure(t, store), registerAzure(t, store) + seedAzureAccountRows(t, store, acctA, acctB) + s := &Scheduler{config: store} + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + return s, store + } + azureOK := func(id string) []config.SuccessfulCollect { return expandSuccessfulCollects("azure", []string{id}) } + + t.Run("outright Azure failure keeps the hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("run without Azure keeps the hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, expandSuccessfulCollects("aws", []string{""}), nil, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("an outage on another account holds both", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctB)) + require.ElementsMatch(t, []string{acctA, acctB}, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle for another account keeps this hold", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle for the account clears it", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, azureOK(acctA), nil, nil) + require.Empty(t, heldAzureAccounts(t, store)) + }) + + t.Run("a good cycle re-prices one of two held accounts", func(t *testing.T) { + s, store := newScheduler(t) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctB)) + s.persistCollection(ctx, nil, azureOK(acctA), nil, nil) + require.Equal(t, []string{acctB}, heldAzureAccounts(t, store)) + }) + + t.Run("a deleted account's hold drops", func(t *testing.T) { + s, store := newScheduler(t) + require.NoError(t, store.DeleteCloudAccount(ctx, acctA)) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Empty(t, heldAzureAccounts(t, store)) + }) + + t.Run("a disabled account with rows stays held", func(t *testing.T) { + s, store := newScheduler(t) + acct, err := store.GetCloudAccount(ctx, acctA) + require.NoError(t, err) + acct.Enabled = false + require.NoError(t, store.UpdateCloudAccount(ctx, acct)) + s.persistCollection(ctx, nil, azureOK(acctB), nil, nil) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + }) + + ambientHold := func(t *testing.T, withRows bool) (*Scheduler, config.StoreInterface) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + if withRows { + seedAdvisorRow(t, store) + } + s := &Scheduler{config: store} + s.persistCollection(ctx, nil, nil, nil, heldPartial()) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + return s, store + } + + t.Run("an ambient hold stays while ambient rows exist, even once accounts are registered", func(t *testing.T) { + s, store := ambientHold(t, true) + s.persistCollection(ctx, nil, azureOK(registerAzure(t, store)), nil, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial) + }) + + t.Run("an ambient hold drops once no ambient rows remain", func(t *testing.T) { + s, store := ambientHold(t, false) + s.persistCollection(ctx, nil, azureOK(registerAzure(t, store)), nil, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial) + }) + + t.Run("an unresolved ambient hold survives an outright failure", func(t *testing.T) { + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + seedAdvisorRow(t, store) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) + fresh, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.NotNil(t, fresh.LastCollectionPartial, "no registered account to name, but the hold must remain") + collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) + fresh, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + require.Nil(t, fresh.LastCollectionPartial) + }) +} + +// The rows and the hold marker are written in one transaction: when writing the +// new marker fails, the upsert rolls back too, so the old rows and the old hold +// both stay. Before, the upsert committed and reset the marker first, and a +// failing second write left the held rows purchasable. +func TestPersistCollection_FailedMarkerWriteRollsBackUpsert(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(container.DB) + s := &Scheduler{config: store} + acctA := registerAzure(t, store) + seedAdvisorRow(t, store) + s.persistCollection(ctx, nil, nil, nil, heldPartial(acctA)) + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store)) + + _, err := container.DB.Exec(ctx, ` + CREATE FUNCTION fail_poisoned_marker() RETURNS trigger AS $$ + BEGIN + IF NEW.last_collection_partial::text LIKE '%poison%' THEN + RAISE EXCEPTION 'injected marker write failure'; + END IF; + RETURN NEW; + END $$ LANGUAGE plpgsql; + CREATE TRIGGER fail_poisoned_marker BEFORE UPDATE ON recommendations_state + FOR EACH ROW EXECUTE FUNCTION fail_poisoned_marker();`) + require.NoError(t, err) + + newRow := config.RecommendationRecord{ID: "new-row", Provider: "azure", Service: "compute", Region: "eastus", ResourceType: "Standard_D4s_v3", + Savings: 10, Count: 1, Term: 12, Payment: "upfront"} + s.persistCollection(ctx, []config.RecommendationRecord{newRow}, nil, nil, heldPartial("poison")) + + require.Equal(t, []string{"prev-advisor-row"}, storedAzureIDs(t, store), "the upsert must roll back with the marker") + require.Equal(t, []string{acctA}, heldAzureAccounts(t, store), "the old hold must survive a failed marker write") +} diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index ad947df78..e56da1b4d 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,10 +17,14 @@ import ( type skipErr struct { msg string partial bool + count int + reasons []string } -func (e *skipErr) Error() string { return e.msg } -func (e *skipErr) Partial() bool { return e.partial } +func (e *skipErr) Error() string { return e.msg } +func (e *skipErr) Partial() bool { return e.partial } +func (e *skipErr) SkippedCount() int { return e.count } +func (e *skipErr) SkipReasons() []string { return e.reasons } const skipMsg = "gcp recommendations: 3 recommendation(s) skipped: computeengine us-east1: non-USD; cloudsql us-east1: bad plan" @@ -29,7 +34,7 @@ func gcpAmbient(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGa func TestGCPPartialResult_KeepsRecsAndRecordsSkips(t *testing.T) { t.Setenv("GCP_PROJECT_ID", "proj-a") - s, _ := newPartialSweepScheduler(t, "gcp", fmt.Errorf("collect: %w", &skipErr{msg: skipMsg, partial: true})) + s, _ := newPartialSweepScheduler(t, "gcp", fmt.Errorf("collect: %w", &skipErr{msg: skipMsg, partial: true, count: 3})) recs, roster, gap, err := gcpAmbient(s) @@ -65,7 +70,7 @@ func TestGCPPartialResult_NonPartialErrorsStayHard(t *testing.T) { func TestGCPPartialResult_FanOutKeepsAccountEvictable(t *testing.T) { accounts := []config.CloudAccount{{ID: "a1", Name: "a1", ExternalID: "p1"}, {ID: "a2", Name: "a2", ExternalID: "p2"}} fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { - gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: skipMsg, partial: true}) + gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: skipMsg, partial: true, count: 3}) if acct.ID == "a2" { // A lost project in the same account hold back eviction as before. gap = mergeSweepGaps(gap, gapFromPartialSubscriptions(partialSweepErr("p-x"))) @@ -83,9 +88,95 @@ func TestGCPPartialResult_FanOutKeepsAccountEvictable(t *testing.T) { assert.Equal(t, 1, outcome.Gap.incompleteAccounts) } -func TestGCPPartialResult_UnparsableCountStillRecorded(t *testing.T) { - gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: "something skipped", partial: true}) +// The count comes from SkippedCount(), never from the message text. +func TestGCPPartialResult_CountComesFromTypedMethod(t *testing.T) { + gap, err := tolerateIncompleteSweep("gcp", &skipErr{msg: "something skipped", partial: true, count: 5}) + require.NoError(t, err) + require.NotNil(t, gap) + assert.Equal(t, 5, gap.skippedRecommendations) +} + +// A mixed Azure sweep (lost subscriptions plus recommendations dropped on the +// reachable ones) reports the skipped count for the user warning, but the lost +// subscriptions still hold stale-row eviction back. +func TestAzureMixedSweep_RecordsSkipsAndHoldsEviction(t *testing.T) { + failure := azureprovider.NewPartialSubscriptionFailure(2, + []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("throttled")}}, + []string{"sub-a: non-USD", "sub-a: bad term"}) + + gap, err := tolerateIncompleteSweep("azure", failure) + + require.NoError(t, err) + require.NotNil(t, gap) + assert.Equal(t, 2, gap.skippedRecommendations) + assert.Equal(t, 2, gap.toProviderSweep("azure").SkippedRecommendations) + assert.True(t, gap.holdsEviction(), "lost subscriptions still hold stale-row eviction") + assert.Equal(t, 1, gap.forAccount().incompleteAccounts) +} + +func TestAzureFailureWithoutSkips_RecordsNoSkippedCount(t *testing.T) { + failure := azureprovider.NewPartialSubscriptionFailure(2, + []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("throttled")}}, nil) + + gap, err := tolerateIncompleteSweep("azure", failure) + require.NoError(t, err) require.NotNil(t, gap) assert.Zero(t, gap.skippedRecommendations) + assert.True(t, gap.holdsEviction()) +} + +func TestHasPricingOutageSkip(t *testing.T) { + for name, tc := range map[string]struct { + reasons []string + want bool + }{ + "first failed row": {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true}, + "later rows": {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true}, + "invalid recommendation": {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false}, + "mid-string mention": {[]string{"computeengine: price lookup unavailable: x"}, false}, + "truncated walk is not an outage": {[]string{"advisor: azure advisor: recommendation list is incomplete"}, false}, + "truncated walk plus outage": {[]string{"advisor: azure advisor: recommendation list is incomplete", "advisor: azure advisor: get: retail prices fetch failed"}, true}, + "prefix mid-string": {[]string{"sub-1: advisor: price lookup unavailable: x"}, false}, + "none": {nil, false}, + } { + t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, hasPricingOutageSkip(tc.reasons)) }) + } +} + +// A pricing outage holds eviction; ordinary invalid-row skips do not. +func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) { + gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, + reasons: []string{"advisor: price lookup unavailable: x"}}) + require.NoError(t, err) + assert.True(t, gap.holdsEviction()) + assert.Equal(t, 1, gap.skippedRecommendations) + + gap, err = tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{"advisor: non-USD"}}) + require.NoError(t, err) + assert.False(t, gap.holdsEviction()) +} + +// The held account's platform ID travels with the gap through the fan-out; +// accounts whose skips are ordinary invalid rows are not held. +func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) { + accounts := []config.CloudAccount{{ID: "held", Name: "held"}, {ID: "plain", Name: "plain"}} + fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { + reason := "advisor: non-USD" + if acct.ID == "held" { + reason = "advisor: price lookup unavailable: x" + } + gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{reason}}) + return []config.RecommendationRecord{{ID: acct.ID}}, gap, err + } + + _, outcome := fanOutPerAccount(context.Background(), "Azure", accounts, fn) + + require.NotNil(t, outcome.Gap) + sweep := outcome.Gap.toProviderSweep("azure") + assert.Equal(t, []string{"held"}, sweep.HeldAccountIDs) + assert.Equal(t, 1, sweep.IncompleteAccounts) + assert.Equal(t, 1, sweep.Reasons[config.PartialReasonPricingUnavailable]) + assert.Equal(t, []string{"held"}, outcome.IncompleteAccountIDs) + assert.Equal(t, []string{"plain"}, outcome.SucceededAccountIDs) } diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index 43cfcfd2b..a510d0bbc 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -4,8 +4,7 @@ import ( "context" "errors" "net/http" - "regexp" - "strconv" + "slices" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -41,6 +40,12 @@ type sweepGap struct { // but invalid recommendations, so the account's sweep itself was complete. skippedOnly bool reasons map[string]int + // pricingUnavailable is set when a skip meant the price could not be + // fetched, so the previous cycle's rows are held (see hasPricingOutageSkip). + pricingUnavailable bool + // heldAccountIDs are the platform account UUIDs whose rows are held for + // that reason; the collector that knows the account fills them in. + heldAccountIDs []string } // holdsEviction reports whether the sweep must stay out of the succeeded- @@ -52,12 +57,14 @@ func (g *sweepGap) holdsEviction() bool { return g != nil && !g.skippedOnly } -var skippedCountRE = regexp.MustCompile(`(\d+) recommendation\(s\) skipped`) - // partialResult is implemented by provider errors that accompany usable // results (GCP's skipped recommendations). Matching the method, not the // provider's internal type, keeps this provider-agnostic. -type partialResult interface{ Partial() bool } +type partialResult interface { + Partial() bool + SkippedCount() int + SkipReasons() []string +} // tolerateSkippedRecommendations keeps the recommendations returned alongside // a Partial() error and records how many were skipped. Any other error is @@ -68,15 +75,89 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, return nil, err } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) - // The count is parsed from the message because the contract exposes only - // Partial(); an unparsable message still yields a gap, with count 0. + g := &sweepGap{reasons: map[string]int{}} + g.recordSkips(p.SkippedCount(), p.SkipReasons()) + g.skippedOnly = !g.pricingUnavailable + return g, nil +} + +// trimSubscriptionPrefixes drops the ": " that the Azure +// multi-subscription client puts before each skip it aggregates +// (recommendations_multi_subscription.go:399 at 61111428), so the reasons match +// the same prefixes as a single-subscription sweep. Only a leading segment +// followed by an Advisor skip is removed, so unprefixed reasons pass through. +func trimSubscriptionPrefixes(reasons []string) []string { + out := make([]string, len(reasons)) + for i, reason := range reasons { + out[i] = reason + if sub, rest, ok := strings.Cut(reason, ": "); ok && sub != "" && !strings.ContainsAny(sub, " :") && strings.HasPrefix(rest, "advisor: ") { + out[i] = rest + } + } + return out +} + +// recordSkips adds dropped recommendations to the gap. A skip that means "the +// price could not be fetched" says nothing about the row being invalid, so its +// previous-cycle row must survive: it marks the gap pricingUnavailable. +func (g *sweepGap) recordSkips(count int, reasons []string) { + g.skippedRecommendations += count + if outage := countPricingOutageSkips(trimSubscriptionPrefixes(reasons)); outage > 0 { + g.pricingUnavailable = true + g.reasons[config.PartialReasonPricingUnavailable] += outage + } +} + +// withHeld records accountID as held when the gap is a pricing outage. An +// empty accountID (an unresolved ambient identity) cannot be named. +func (g *sweepGap) withHeld(accountID string) *sweepGap { + if g == nil || !g.pricingUnavailable || accountID == "" { + return g + } + out := mergeSweepGaps(nil, g) + out.heldAccountIDs = append(out.heldAccountIDs, accountID) + return out +} + +// Skip-reason prefixes cloud-commitments-go records when Azure Advisor rows +// cannot be priced because the Retail Prices catalog is unreachable +// (providers/azure/recommendations_advisor_pricing.go, priceAdvisorReservations, +// go#663). The library exports no constant for them: RecordSkip stores +// ": " strings. The first failed row is recorded with the +// collection-failure wrapper "azure advisor: "; every later row of the call +// gets "price lookup unavailable: ". +var pricingOutageSkipPrefixes = []string{ + "advisor: price lookup unavailable: ", + "advisor: azure advisor: ", +} + +// advisorTruncatedSkipPrefix starts the skip recorded for an Advisor walk that +// hit the page cap or a page error (collectAdvisorOrSkip, +// providers/azure/recommendations.go:471 at 61111428). It shares the +// "advisor: azure advisor: " prefix but is not a pricing outage: a walk over +// the page cap is always truncated, so holding eviction would keep that +// account's stale rows forever. +const advisorTruncatedSkipPrefix = "advisor: azure advisor: recommendation list is incomplete" + +// hasPricingOutageSkip reports whether any skip reason is a pricing outage. +func hasPricingOutageSkip(reasons []string) bool { + return countPricingOutageSkips(reasons) > 0 +} + +func countPricingOutageSkips(reasons []string) int { n := 0 - if m := skippedCountRE.FindStringSubmatch(err.Error()); m != nil { - if v, convErr := strconv.Atoi(m[1]); convErr == nil { - n = v + for _, reason := range reasons { + if strings.HasPrefix(reason, advisorTruncatedSkipPrefix) { + continue + } + for _, prefix := range pricingOutageSkipPrefixes { + if strings.HasPrefix(reason, prefix) { + n++ + break + } } } - return &sweepGap{skippedRecommendations: n, skippedOnly: true, reasons: map[string]int{}}, nil + return n } // gapFromAWSIncomplete summarizes an AWS IncompleteRecommendationsError. @@ -102,6 +183,12 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro failedScopes: max(p.Attempted-p.Succeeded, len(p.Failed)), reasons: map[string]int{}, } + // A mixed sweep also lost invalid recommendations. The count is for the + // user warning only and does not affect eviction (skippedOnly stays false), + // but a pricing outage among them holds the account like a skips-only one. + if sk := p.Skips(); sk != nil { + g.recordSkips(sk.SkippedCount(), sk.SkipReasons()) + } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ } @@ -134,6 +221,8 @@ func mergeSweepGaps(a, b *sweepGap) *sweepGap { continue } out.skippedOnly = out.skippedOnly && g.skippedOnly + out.pricingUnavailable = out.pricingUnavailable || g.pricingUnavailable + out.heldAccountIDs = append(out.heldAccountIDs, g.heldAccountIDs...) out.skippedRecommendations += g.skippedRecommendations out.incompleteAccounts += g.incompleteAccounts out.attemptedScopes += g.attemptedScopes @@ -160,6 +249,7 @@ func (g *sweepGap) toProviderSweep(providerName string) config.PartialProviderSw FailedDetails: g.failedDetails, SkippedRecommendations: g.skippedRecommendations, Reasons: reasons, + HeldAccountIDs: slices.Sorted(slices.Values(g.heldAccountIDs)), } } diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index 4448a52a9..62d48be62 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -189,15 +189,17 @@ func expectScheduledCollection(store *MockConfigStore, providers ...string) (*[] store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { upserted = args.Get(2).([]config.RecommendationRecord) - }).Return(nil).Once() + }).Return(nil).Maybe() - // Registered as Maybe so an unwanted call is RECORDED: MockConfigStore - // short-circuits unexpected methods before mock.Called, which would - // make AssertNotCalled pass vacuously. + // A partial collection writes its marker in the same transaction as the + // rows. Registered as Maybe so an unwanted call is RECORDED: + // MockConfigStore short-circuits unexpected methods before mock.Called, + // which would make AssertNotCalled pass vacuously. var recorded *config.RecommendationsCollectionPartial - store.On("SetRecommendationsCollectionPartial", mock.Anything, mock.Anything). + store.On("UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Run(func(args mock.Arguments) { - recorded = args.Get(1).(*config.RecommendationsCollectionPartial) + upserted = args.Get(2).([]config.RecommendationRecord) + recorded = args.Get(4).(*config.RecommendationsCollectionPartial) }).Return(nil).Maybe() return &upserted, &recorded } @@ -241,7 +243,7 @@ func TestCollectRecommendations_CompleteSweepLeavesPartialCleared(t *testing.T) require.NoError(t, err) assert.Len(t, *upserted, 1) - store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestCollectRecommendations_TotalFailureUnchanged: a hard provider failure @@ -260,7 +262,7 @@ func TestCollectRecommendations_TotalFailureUnchanged(t *testing.T) { require.NoError(t, err) assert.Contains(t, result.FailedProviders, "azure") assert.Contains(t, recordedErr, "azure: ") - store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) + store.AssertNotCalled(t, "UpsertRecommendationsWithPartial", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // A partial provider and a complete one: only the partial provider appears. @@ -272,3 +274,49 @@ func TestAppendPartialProvider(t *testing.T) { assert.Equal(t, "azure", got.Providers[0].Provider) assert.NotNil(t, got.Providers[0].Reasons, "reasons must encode as {} rather than null") } + +// A mixed Azure sweep (a lost subscription plus Advisor pricing-outage skips on +// a reachable one) must hold the account like a skips-only outage does, or its +// rows stay purchasable at the previous cycle's price. +func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T) { + outage := []string{ + `advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + } + failed := []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("403 authorization failed")}} + + // The multi-subscription client Provider.GetRecommendationsClient returns + // when scope is ambiguous builds each skip as ": " + // (recommendations_multi_subscription.go:399 at 61111428). + prefixed := make([]string, len(outage)) + for i, r := range outage { + prefixed[i] = "0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: " + r + } + gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, prefixed)) + require.NoError(t, err) + + held := gap.forAccount().withHeld("acct-1") + assert.True(t, held.pricingUnavailable) + assert.Equal(t, []string{"acct-1"}, held.heldAccountIDs) + assert.True(t, held.holdsEviction(), "the lost subscription still holds eviction") + sweep := held.toProviderSweep("azure") + assert.Positive(t, sweep.Reasons[config.PartialReasonPricingUnavailable]) + assert.Equal(t, []string{"acct-1"}, sweep.HeldAccountIDs) +} + +// A skips-only multi-subscription sweep returns a plain PartialResultError +// (recommendations_multi_subscription.go:421 at 61111428) whose skips carry the +// same ": " prefix; its outage must hold the account too. +func TestTolerateIncompleteSweep_PrefixedPricingOutageSkipsHoldAccount(t *testing.T) { + skips := []string{ + `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, + } + gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips}) + require.NoError(t, err) + + held := gap.forAccount().withHeld("acct-1") + assert.True(t, held.pricingUnavailable) + assert.True(t, held.holdsEviction(), "the held rows must survive eviction") + assert.Equal(t, []string{"acct-1"}, held.heldAccountIDs) +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index de8718730..2956fe24f 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -433,11 +433,10 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT // recorded in recommendations_state so the frontend banner renders while // the user still sees the valid rows we managed to upsert. // -// UpsertRecommendations also resets last_collection_partial, so a complete -// collection clears the previous run's partial marker; a non-nil partial -// re-records it straight after (issue #87). The two writes mirror the -// existing error-column pattern and are not atomic: if the second write -// fails, the run is logged and reads as complete until the next collection. +// The rows and last_collection_partial commit in one transaction: a complete +// collection clears the previous run's marker, a non-nil partial replaces it +// (issue #87). A failed marker write rolls the rows back too, so the Azure +// pricing hold cannot be lost while the held rows stay. // // After a successful write it prunes the rows of (provider, account) pairs // that have not collected for CUDLY_STALE_RECOMMENDATION_RETENTION_DAYS @@ -445,7 +444,7 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT // stops showing instead of staying forever (issue #120). A failed write skips // the prune. func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { - if err := s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects); err != nil { + if err := s.upsertWithMarker(ctx, recs, successfulCollects, partial); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { logging.Errorf("Failed to record write error: %v", setErr) @@ -459,15 +458,19 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme } } - if partial != nil { - if err := s.config.SetRecommendationsCollectionPartial(ctx, partial); err != nil { - logging.Errorf("Failed to record partial collection: %v", err) - } - } - s.pruneStaleRecommendationsBestEffort(ctx) } +// upsertWithMarker writes the rows and the collection's partial marker in one +// transaction. Both store methods merge the previous Azure pricing hold under +// the state row lock, so a complete collection (nil partial) keeps it too. +func (s *Scheduler) upsertWithMarker(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, partial *config.RecommendationsCollectionPartial) error { + if partial == nil { + return s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects) + } + return s.config.UpsertRecommendationsWithPartial(ctx, time.Now().UTC(), recs, successfulCollects, partial) +} + // joinProviderErrors formats a "provider: err; provider: err" string for // the state table's last_collection_error column. func joinProviderErrors(failed map[string]string) string { @@ -567,7 +570,7 @@ func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *co // in those cases. if acctID := s.resolveAmbientHostAccountID(ctx); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } @@ -678,7 +681,7 @@ func fanOutPerAccount( } else { outcome.SucceededAccountIDs = append(outcome.SucceededAccountIDs, acct.ID) } - outcome.Gap = mergeSweepGaps(outcome.Gap, gap.forAccount()) + outcome.Gap = mergeSweepGaps(outcome.Gap, gap.forAccount().withHeld(acct.ID)) mu.Unlock() return nil }) @@ -918,7 +921,7 @@ func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.G } if acctID := s.resolveAmbientAccountID(ctx, "azure", subscriptionID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } @@ -993,7 +996,7 @@ func (s *Scheduler) collectGCPRecommendations(ctx context.Context, _ *config.Glo } if acctID := s.resolveAmbientAccountID(ctx, "gcp", projectID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount(), nil + return recs, ambientEvictionRoster(!gap.holdsEviction(), acctID), gap.forAccount().withHeld(acctID), nil } return recs, ambientEvictionRoster(!gap.holdsEviction(), ""), gap.forAccount(), nil } @@ -1114,11 +1117,15 @@ func tolerateIncompleteSweep(providerName string, err error) (*sweepGap, error) if partial == nil { return tolerateSkippedRecommendations(providerName, err) } + skipped := 0 + if sk := partial.Skips(); sk != nil { + skipped = sk.SkippedCount() + } logging.Warnf( - "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s (keeping the %d that did, "+ - "and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", + "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s; %d recommendation(s) skipped "+ + "(keeping the %d that did, and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", providerName, partial.Succeeded, partial.Attempted, - strings.Join(partial.FailedSubscriptionIDs(), ", "), partial.Succeeded) + strings.Join(partial.FailedSubscriptionIDs(), ", "), skipped, partial.Succeeded) return gapFromPartialSubscriptions(partial), nil } diff --git a/internal/scheduler/stale_recommendations_test.go b/internal/scheduler/stale_recommendations_test.go index 11dd09a23..5f43903ee 100644 --- a/internal/scheduler/stale_recommendations_test.go +++ b/internal/scheduler/stale_recommendations_test.go @@ -79,8 +79,7 @@ func TestScheduler_persistCollection_PruneErrorIsBestEffort(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) partial := &config.RecommendationsCollectionPartial{} - mockStore.On("UpsertRecommendations", ctx, mock.Anything, mock.Anything, mock.Anything).Return(nil) - mockStore.On("SetRecommendationsCollectionPartial", ctx, partial).Return(nil) + mockStore.On("UpsertRecommendationsWithPartial", ctx, mock.Anything, mock.Anything, mock.Anything, partial).Return(nil) mockStore.On("PruneStaleRecommendations", ctx, config.DefaultStaleRecommendationRetentionDays). Return(int64(0), errors.New("lock timeout"))