From af30f9568b5cb5f0495a5e9f7a25988df150bddf Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Tue, 25 Apr 2023 11:32:07 +0200 Subject: [PATCH 1/4] [WIP] Implement cleanup of broken partner records --- .../odoo/odoo8/client/model/partner.go | 4 +++ .../billing/odoostorage/odoo/odoo8/odoo8.go | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go b/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go index 39942491..83741efe 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go @@ -142,3 +142,7 @@ func (o Odoo) CreatePartner(ctx context.Context, p Partner) (id int, err error) func (o Odoo) UpdateRawPartner(ctx context.Context, ids []int, raw any) error { return o.querier.UpdateGenericModel(ctx, PartnerModel, ids, raw) } + +func (o Odoo) DeletePartner(ctx context.Context, ids []int) error { + return o.querier.DeleteGenericModel(ctx, PartnerModel, ids) +} \ No newline at end of file diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go index eec9144a..8c4f9264 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go @@ -31,6 +31,8 @@ var metaUIDNamespace = uuid.MustParse("51887759-C769-4829-9910-BB9D5F92767D") var roleAccountFilter = []any{"category_id", "in", []int{roleAccountCategory}} var activeFilter = []any{"active", "in", []bool{true}} var notInflightFilter = []any{"x_control_api_inflight", "in", []any{false}} +var mustInflightFilter = []any{"x_control_api_inflight", "in", []any{true}} +var notRecentlyUpdated = []any{"x_control_api_inflight", "in", []any{true}} var ( // There's a ton of fields we don't want to override in Odoo. @@ -274,6 +276,33 @@ func (s *oodo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) return nil } +func (s *oodo8Storage) CleanupIncompleteRecords(ctx context.Context) error { + l := klog.FromContext(ctx) + + session, err := s.sessionCreator(ctx) + if err != nil { + return err + } + o := model.NewOdoo(session) + + inflightRecords, err := o.SearchPartners(ctx, []client.Filter{ + mustInflightFilter, + notRecentlyUpdated, + }) + if err != nil { + return err + } + + ids := []int{} + + for _, record := range(inflightRecords) { + ids = append(ids, record.ID) + l.Info("Preparing to delete inflight partner record", "record", record) + } + + return o.DeletePartner(ctx, ids) +} + func k8sIDToOdooID(id string) (int, error) { if !strings.HasPrefix(id, "be-") { return 0, fmt.Errorf("invalid ID, missing prefix: %s", id) From a515506b21a3f13fbd844c8dd148abd91b7a9d7c Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Wed, 26 Apr 2023 16:58:55 +0200 Subject: [PATCH 2/4] Clean up inflight billing entities after a while --- Makefile | 4 ++ .../odoo/odoo8/client/model/partner.go | 2 +- .../billing/odoostorage/odoo/odoo8/odoo8.go | 41 +++++++------ .../odoostorage/odoo/odoo8/odoo8_test.go | 41 ++++++++++++- cleanup.go | 57 +++++++++++++++++++ main.go | 2 +- 6 files changed, 127 insertions(+), 20 deletions(-) create mode 100644 cleanup.go diff --git a/Makefile b/Makefile index 47fd4b37..6a905a7f 100644 --- a/Makefile +++ b/Makefile @@ -74,6 +74,10 @@ run-controller: build ## Starts control api controller against the current Kuber $(localenv_make) webhook-certs/tls.key $(BIN_FILENAME) controller --username-prefix "appuio#" --webhook-cert-dir=./local-env/webhook-certs --webhook-port=9444 --zap-log-level debug --billingentity-email-cron-interval "@every 1m" +.PHONY: run-cleanup +run-cleanup: build ## Starts cleanup command + $(BIN_FILENAME) cleanup --billing-entity-odoo8-url $(BE_ODOO_URL) + .PHONY: local-env local-env-setup: ## Setup local kind-based dev environment $(localenv_make) setup diff --git a/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go b/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go index 83741efe..a22d91f3 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/client/model/partner.go @@ -145,4 +145,4 @@ func (o Odoo) UpdateRawPartner(ctx context.Context, ids []int, raw any) error { func (o Odoo) DeletePartner(ctx context.Context, ids []int) error { return o.querier.DeleteGenericModel(ctx, PartnerModel, ids) -} \ No newline at end of file +} diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go index 8c4f9264..135252a9 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go @@ -7,6 +7,7 @@ import ( "fmt" "strconv" "strings" + "time" "github.com/google/uuid" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,8 +32,7 @@ var metaUIDNamespace = uuid.MustParse("51887759-C769-4829-9910-BB9D5F92767D") var roleAccountFilter = []any{"category_id", "in", []int{roleAccountCategory}} var activeFilter = []any{"active", "in", []bool{true}} var notInflightFilter = []any{"x_control_api_inflight", "in", []any{false}} -var mustInflightFilter = []any{"x_control_api_inflight", "in", []any{true}} -var notRecentlyUpdated = []any{"x_control_api_inflight", "in", []any{true}} +var mustInflightFilter = []any{"x_control_api_inflight", "not in", []any{false}} var ( // There's a ton of fields we don't want to override in Odoo. @@ -65,8 +65,10 @@ type Config struct { PaymentTermID int } -func NewOdoo8Storage(odooURL string, debugTransport bool, conf Config) odoo.OdooStorage { - return &oodo8Storage{ +var _ odoo.OdooStorage = &Odoo8Storage{} + +func NewOdoo8Storage(odooURL string, debugTransport bool, conf Config) *Odoo8Storage { + return &Odoo8Storage{ config: conf, sessionCreator: func(ctx context.Context) (client.QueryExecutor, error) { return client.Open(ctx, odooURL, client.ClientOptions{UseDebugLogger: debugTransport}) @@ -74,13 +76,13 @@ func NewOdoo8Storage(odooURL string, debugTransport bool, conf Config) odoo.Odoo } } -type oodo8Storage struct { +type Odoo8Storage struct { config Config sessionCreator func(ctx context.Context) (client.QueryExecutor, error) } -func (s *oodo8Storage) Get(ctx context.Context, name string) (*billingv1.BillingEntity, error) { +func (s *Odoo8Storage) Get(ctx context.Context, name string) (*billingv1.BillingEntity, error) { company, accountingContact, err := s.get(ctx, name) if err != nil { return nil, err @@ -90,7 +92,7 @@ func (s *oodo8Storage) Get(ctx context.Context, name string) (*billingv1.Billing return &be, nil } -func (s *oodo8Storage) get(ctx context.Context, name string) (company model.Partner, accountingContact model.Partner, err error) { +func (s *Odoo8Storage) get(ctx context.Context, name string) (company model.Partner, accountingContact model.Partner, err error) { id, err := k8sIDToOdooID(name) if err != nil { return model.Partner{}, model.Partner{}, err @@ -119,7 +121,7 @@ func (s *oodo8Storage) get(ctx context.Context, name string) (company model.Part return company, accountingContact, nil } -func (s *oodo8Storage) List(ctx context.Context) ([]billingv1.BillingEntity, error) { +func (s *Odoo8Storage) List(ctx context.Context) ([]billingv1.BillingEntity, error) { l := klog.FromContext(ctx) session, err := s.sessionCreator(ctx) @@ -175,7 +177,7 @@ func (s *oodo8Storage) List(ctx context.Context) ([]billingv1.BillingEntity, err return bes, nil } -func (s *oodo8Storage) Create(ctx context.Context, be *billingv1.BillingEntity) error { +func (s *Odoo8Storage) Create(ctx context.Context, be *billingv1.BillingEntity) error { l := klog.FromContext(ctx) if be == nil { @@ -227,7 +229,7 @@ func (s *oodo8Storage) Create(ctx context.Context, be *billingv1.BillingEntity) return nil } -func (s *oodo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) error { +func (s *Odoo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) error { l := klog.FromContext(ctx) if be == nil { @@ -276,8 +278,9 @@ func (s *oodo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) return nil } -func (s *oodo8Storage) CleanupIncompleteRecords(ctx context.Context) error { +func (s *Odoo8Storage) CleanupIncompleteRecords(ctx context.Context, minAge time.Duration) error { l := klog.FromContext(ctx) + l.Info("Looking for stale inflight partner records...") session, err := s.sessionCreator(ctx) if err != nil { @@ -287,7 +290,6 @@ func (s *oodo8Storage) CleanupIncompleteRecords(ctx context.Context) error { inflightRecords, err := o.SearchPartners(ctx, []client.Filter{ mustInflightFilter, - notRecentlyUpdated, }) if err != nil { return err @@ -295,12 +297,19 @@ func (s *oodo8Storage) CleanupIncompleteRecords(ctx context.Context) error { ids := []int{} - for _, record := range(inflightRecords) { - ids = append(ids, record.ID) - l.Info("Preparing to delete inflight partner record", "record", record) + for _, record := range inflightRecords { + updateTime := record.CreationTimestamp.ToTime() + + if updateTime.Before(time.Now().Add(-1 * minAge)) { + ids = append(ids, record.ID) + l.Info("Preparing to delete inflight partner record", "name", record.Name, "id", record.ID) + } } - return o.DeletePartner(ctx, ids) + if len(ids) != 0 { + return o.DeletePartner(ctx, ids) + } + return nil } func k8sIDToOdooID(id string) (int, error) { diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go index 23f7c98c..d3c61a11 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go @@ -390,11 +390,11 @@ func Test_CreateUpdate_UnknownCountry(t *testing.T) { require.ErrorContains(t, subject.Update(context.Background(), s), "unknown country") } -func createStorage(t *testing.T) (*gomock.Controller, *clientmock.MockQueryExecutor, *oodo8Storage) { +func createStorage(t *testing.T) (*gomock.Controller, *clientmock.MockQueryExecutor, *Odoo8Storage) { ctrl := gomock.NewController(t) mock := clientmock.NewMockQueryExecutor(ctrl) - return ctrl, mock, &oodo8Storage{ + return ctrl, mock, &Odoo8Storage{ config: Config{ CountryIDs: map[string]int{ "": 0, @@ -410,3 +410,40 @@ func createStorage(t *testing.T) (*gomock.Controller, *clientmock.MockQueryExecu }, } } + +func TestCleanup(t *testing.T) { + ctrl, mock, subject := createStorage(t) + defer ctrl.Finish() + + tn := time.Now() + to := tn.Add(time.Hour * -1) + + gomock.InOrder( + // Fetch stale records + mock.EXPECT().SearchGenericModel(gomock.Any(), gomock.Any(), gomock.Any()).SetArg(2, model.PartnerList{ + Items: []model.Partner{ + { + ID: 702, + Name: "Accounting", + CreationTimestamp: client.Date(tn), + Parent: model.OdooCompositeID{ID: 700, Valid: true}, + EmailRaw: model.NewNullable("accounting@test.com, notifications@test.com"), + Inflight: model.NewNullable("fooo"), + }, + { + ID: 703, + Name: "Accounting", + CreationTimestamp: client.Date(to), + Parent: model.OdooCompositeID{ID: 700, Valid: true}, + EmailRaw: model.NewNullable("accounting@test.com, notifications@test.com"), + Inflight: model.NewNullable("fooo"), + }, + }, + }), + mock.EXPECT().DeleteGenericModel(gomock.Any(), gomock.Any(), gomock.Eq([]int{703})).Return(nil), + ) + + err := subject.CleanupIncompleteRecords(context.Background(), time.Minute) + require.NoError(t, err) + +} diff --git a/cleanup.go b/cleanup.go new file mode 100644 index 00000000..80c74300 --- /dev/null +++ b/cleanup.go @@ -0,0 +1,57 @@ +package main + +import ( + "os" + "time" + + "github.com/spf13/cobra" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + + "github.com/appuio/control-api/apiserver/billing/odoostorage/odoo/odoo8" + "github.com/appuio/control-api/apiserver/billing/odoostorage/odoo/odoo8/countries" +) + +// APICommand creates a new command allowing to start the API server +func CleanupCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "cleanup", + } + + odooUrl := cmd.Flags().String("billing-entity-odoo8-url", "http://localhost:8069", "URL of the Odoo instance to use for billing entities") + debugTransport := cmd.Flags().Bool("billing-entity-odoo8-debug-transport", false, "Enable debug logging for the Odoo transport") + countryList := cmd.Flags().String("billing-entity-odoo8-country-list", "countries.yaml", "Path to the country list file in the format of [{name: \"Germany\", code: \"DE\", id: 81},...]") + accountingContactDisplayName := cmd.Flags().String("billing-entity-odoo8-accounting-contact-display-name", "Accounting", "Display name of the accounting contact") + languagePreference := cmd.Flags().String("billing-entity-odoo8-language-preference", "en_US", "Language preference of the Odoo record") + paymentTerm := cmd.Flags().Int("billing-entity-odoo8-payment-term-id", 2, "Payment term ID of the Odoo record") + minAge := cmd.Flags().Duration("billing-entity-odoo8-cleanup-after", time.Hour, "Clean up only records older than this") + + cmd.Run = func(cmd *cobra.Command, args []string) { + ctx := ctrl.SetupSignalHandler() + l := klog.FromContext(ctx) + countryIDs, err := countries.LoadCountryIDs(*countryList) + if err != nil { + l.Error(err, "Unable to load country list") + os.Exit(1) + } + storage := odoo8.NewOdoo8Storage( + *odooUrl, + *debugTransport, + odoo8.Config{ + AccountingContactDisplayName: *accountingContactDisplayName, + LanguagePreference: *languagePreference, + PaymentTermID: *paymentTerm, + CountryIDs: countryIDs, + }, + ) + + err = storage.CleanupIncompleteRecords(ctx, *minAge) + if err != nil { + l.Error(err, "Unable to clean up incomplete records") + os.Exit(1) + } + l.Info("Cleanup complete!") + } + + return cmd +} diff --git a/main.go b/main.go index 7314ceaa..b2235e7b 100644 --- a/main.go +++ b/main.go @@ -20,7 +20,7 @@ var ( ) func main() { - rootCommand.AddCommand(ControllerCommand(), APICommand()) + rootCommand.AddCommand(ControllerCommand(), APICommand(), CleanupCommand()) if err := rootCommand.Execute(); err != nil { fmt.Fprintln(os.Stderr, err) From 66db92aa8d2483ee75344ba13c89dd7d1d146df7 Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Fri, 28 Apr 2023 10:21:13 +0200 Subject: [PATCH 3/4] Separate cleanup to its own struct in order to remove unneeded parameters --- .../billing/odoostorage/odoo/odoo8/odoo8.go | 20 ++++++++++++++++--- .../odoostorage/odoo/odoo8/odoo8_test.go | 13 +++++++++++- cleanup.go | 20 ++----------------- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go index 135252a9..dd9fe29f 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go @@ -76,12 +76,24 @@ func NewOdoo8Storage(odooURL string, debugTransport bool, conf Config) *Odoo8Sto } } +func NewFailedRecordScrubber(odooURL string, debugTransport bool) *FailedRecordScrubber { + return &FailedRecordScrubber{ + sessionCreator: func(ctx context.Context) (client.QueryExecutor, error) { + return client.Open(ctx, odooURL, client.ClientOptions{UseDebugLogger: debugTransport}) + }, + } +} + type Odoo8Storage struct { config Config sessionCreator func(ctx context.Context) (client.QueryExecutor, error) } +type FailedRecordScrubber struct { + sessionCreator func(ctx context.Context) (client.QueryExecutor, error) +} + func (s *Odoo8Storage) Get(ctx context.Context, name string) (*billingv1.BillingEntity, error) { company, accountingContact, err := s.get(ctx, name) if err != nil { @@ -278,7 +290,9 @@ func (s *Odoo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) return nil } -func (s *Odoo8Storage) CleanupIncompleteRecords(ctx context.Context, minAge time.Duration) error { +func (s *FailedRecordScrubber) CleanupIncompleteRecords(ctx context.Context, minAge time.Duration) error { + // CleanupIncompleteRecords looks for partner records in Odoo that still have the "inflight" flag set despite being older than `minAge`. Those records are then deleted. + // Such records might come into existence due to a partially failed creation request. l := klog.FromContext(ctx) l.Info("Looking for stale inflight partner records...") @@ -298,9 +312,9 @@ func (s *Odoo8Storage) CleanupIncompleteRecords(ctx context.Context, minAge time ids := []int{} for _, record := range inflightRecords { - updateTime := record.CreationTimestamp.ToTime() + createdTime := record.CreationTimestamp.ToTime() - if updateTime.Before(time.Now().Add(-1 * minAge)) { + if createdTime.Before(time.Now().Add(-1 * minAge)) { ids = append(ids, record.ID) l.Info("Preparing to delete inflight partner record", "name", record.Name, "id", record.ID) } diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go index d3c61a11..92e41f10 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8_test.go @@ -411,8 +411,19 @@ func createStorage(t *testing.T) (*gomock.Controller, *clientmock.MockQueryExecu } } +func createFailedRecordScrubber(t *testing.T) (*gomock.Controller, *clientmock.MockQueryExecutor, *FailedRecordScrubber) { + ctrl := gomock.NewController(t) + mock := clientmock.NewMockQueryExecutor(ctrl) + + return ctrl, mock, &FailedRecordScrubber{ + sessionCreator: func(ctx context.Context) (client.QueryExecutor, error) { + return mock, nil + }, + } +} + func TestCleanup(t *testing.T) { - ctrl, mock, subject := createStorage(t) + ctrl, mock, subject := createFailedRecordScrubber(t) defer ctrl.Finish() tn := time.Now() diff --git a/cleanup.go b/cleanup.go index 80c74300..4e11c2ec 100644 --- a/cleanup.go +++ b/cleanup.go @@ -9,7 +9,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "github.com/appuio/control-api/apiserver/billing/odoostorage/odoo/odoo8" - "github.com/appuio/control-api/apiserver/billing/odoostorage/odoo/odoo8/countries" ) // APICommand creates a new command allowing to start the API server @@ -20,32 +19,17 @@ func CleanupCommand() *cobra.Command { odooUrl := cmd.Flags().String("billing-entity-odoo8-url", "http://localhost:8069", "URL of the Odoo instance to use for billing entities") debugTransport := cmd.Flags().Bool("billing-entity-odoo8-debug-transport", false, "Enable debug logging for the Odoo transport") - countryList := cmd.Flags().String("billing-entity-odoo8-country-list", "countries.yaml", "Path to the country list file in the format of [{name: \"Germany\", code: \"DE\", id: 81},...]") - accountingContactDisplayName := cmd.Flags().String("billing-entity-odoo8-accounting-contact-display-name", "Accounting", "Display name of the accounting contact") - languagePreference := cmd.Flags().String("billing-entity-odoo8-language-preference", "en_US", "Language preference of the Odoo record") - paymentTerm := cmd.Flags().Int("billing-entity-odoo8-payment-term-id", 2, "Payment term ID of the Odoo record") minAge := cmd.Flags().Duration("billing-entity-odoo8-cleanup-after", time.Hour, "Clean up only records older than this") cmd.Run = func(cmd *cobra.Command, args []string) { ctx := ctrl.SetupSignalHandler() l := klog.FromContext(ctx) - countryIDs, err := countries.LoadCountryIDs(*countryList) - if err != nil { - l.Error(err, "Unable to load country list") - os.Exit(1) - } - storage := odoo8.NewOdoo8Storage( + scrubber := odoo8.NewFailedRecordScrubber( *odooUrl, *debugTransport, - odoo8.Config{ - AccountingContactDisplayName: *accountingContactDisplayName, - LanguagePreference: *languagePreference, - PaymentTermID: *paymentTerm, - CountryIDs: countryIDs, - }, ) - err = storage.CleanupIncompleteRecords(ctx, *minAge) + err := scrubber.CleanupIncompleteRecords(ctx, *minAge) if err != nil { l.Error(err, "Unable to clean up incomplete records") os.Exit(1) From 99861eac3b29616e17f86e1701bba3ace399acfe Mon Sep 17 00:00:00 2001 From: Aline Abler Date: Fri, 28 Apr 2023 16:20:28 +0200 Subject: [PATCH 4/4] Update apiserver/billing/odoostorage/odoo/odoo8/odoo8.go Co-authored-by: Sebastian Widmer --- apiserver/billing/odoostorage/odoo/odoo8/odoo8.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go index dd9fe29f..1a0ac4b4 100644 --- a/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go +++ b/apiserver/billing/odoostorage/odoo/odoo8/odoo8.go @@ -290,9 +290,9 @@ func (s *Odoo8Storage) Update(ctx context.Context, be *billingv1.BillingEntity) return nil } +// CleanupIncompleteRecords looks for partner records in Odoo that still have the "inflight" flag set despite being older than `minAge`. Those records are then deleted. +// Such records might come into existence due to a partially failed creation request. func (s *FailedRecordScrubber) CleanupIncompleteRecords(ctx context.Context, minAge time.Duration) error { - // CleanupIncompleteRecords looks for partner records in Odoo that still have the "inflight" flag set despite being older than `minAge`. Those records are then deleted. - // Such records might come into existence due to a partially failed creation request. l := klog.FromContext(ctx) l.Info("Looking for stale inflight partner records...")