From 35db8dd8f0f1afb355c8efc68d831d0800129eed Mon Sep 17 00:00:00 2001 From: Doctor Vince Date: Tue, 16 Jan 2024 13:18:56 -0500 Subject: [PATCH] fix metadata updates (#182) --- internal/collector/collector.go | 4 -- internal/collector/collector_test.go | 1 + internal/store/serverservice/attributes.go | 20 +++++---- .../store/serverservice/attributes_test.go | 41 +++++++++++-------- .../store/serverservice/components_test.go | 2 +- internal/store/serverservice/helpers.go | 22 +++------- internal/store/serverservice/serverservice.go | 26 ++---------- 7 files changed, 46 insertions(+), 70 deletions(-) diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 92349f1c..2a37cab4 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -25,10 +25,6 @@ var ( ErrInventoryCollect = errors.New("error collecting inventory data") ) -func init() { - -} - // DeviceCollector holds attributes to collect inventory, bios configuration data from a single device. type DeviceCollector struct { queryor device.Queryor diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index aec68ffa..3cd93c5a 100644 --- a/internal/collector/collector_test.go +++ b/internal/collector/collector_test.go @@ -13,6 +13,7 @@ import ( "go.uber.org/goleak" ) +// XXX: Warning, this test might be flaky. func Test_Collect_Concurrent(t *testing.T) { ignorefunc := "go.opencensus.io/stats/view.(*worker).start" defer goleak.VerifyNone(t, goleak.IgnoreTopFunction(ignorefunc)) diff --git a/internal/store/serverservice/attributes.go b/internal/store/serverservice/attributes.go index 3bb71865..497b5bf6 100644 --- a/internal/store/serverservice/attributes.go +++ b/internal/store/serverservice/attributes.go @@ -18,7 +18,8 @@ import ( ) const ( - uefiVariablesKey = "uefi-variables" + uefiVariablesKey = "uefi-variables" + ssMetadataAttributeFound = "__ss_found" ) // createUpdateServerAttributes creates/updates the server serial, vendor, model attributes @@ -174,11 +175,7 @@ func mustFilterAssetMetadata(inventory map[string]string) json.RawMessage { func (r *Store) createUpdateServerMetadataAttributes(ctx context.Context, serverID uuid.UUID, asset *model.Asset) error { // no metadata reported in inventory from device if asset.Inventory == nil || len(asset.Inventory.Metadata) == 0 { - return nil - } - - // update when metadata differs - if helpers.MapsAreEqual(asset.Metadata, asset.Inventory.Metadata) { + // XXX: should delete the metadata on the server-service record! return nil } @@ -190,12 +187,15 @@ func (r *Store) createUpdateServerMetadataAttributes(ctx context.Context, server Data: metadata, } - // current asset metadata has no attributes set, create - if len(asset.Metadata) == 0 { + // XXX: This would be much easier if serverservice/fleetdb supported upsert + // current asset metadata has no attributes set and no metadata attribute, create one + if _, ok := asset.Metadata[ssMetadataAttributeFound]; !ok { + r.logger.WithField("server.id", serverID.String()).Debug("creating metadata attributes") _, err := r.CreateAttributes(ctx, serverID, attribute) return err } + r.logger.WithField("server.id", serverID.String()).Debug("updating metadata attributes") // update vendor, model attributes _, err := r.UpdateAttributes(ctx, serverID, serverMetadataAttributeNS, metadata) @@ -498,6 +498,10 @@ func serverMetadataAttributes(attributes []serverserviceapi.Attributes) (map[str if err := json.Unmarshal(attribute.Data, &metadata); err != nil { return nil, errors.Wrap(ErrServerServiceObject, "server metadata attribute: "+err.Error()) } + // XXX: it is possible for there to be a metadata attribute with an empty Data field + // Add an entry here so that when we test for doing a create vs. an update we make the + // right decision. + metadata[ssMetadataAttributeFound] = "true" } } diff --git a/internal/store/serverservice/attributes_test.go b/internal/store/serverservice/attributes_test.go index 8ada9a51..e989f722 100644 --- a/internal/store/serverservice/attributes_test.go +++ b/internal/store/serverservice/attributes_test.go @@ -15,7 +15,6 @@ import ( "github.com/bmc-toolbox/common" "github.com/google/uuid" "github.com/hashicorp/go-retryablehttp" - "github.com/metal-toolbox/alloy/internal/app" "github.com/metal-toolbox/alloy/internal/fixtures" "github.com/metal-toolbox/alloy/internal/model" "github.com/sirupsen/logrus" @@ -44,7 +43,7 @@ func testStoreInstance(t *testing.T, mockURL string) *Store { } return &Store{ - logger: app.NewLogrusEntryFromLogger(logrus.Fields{"component": "publisher"}, logrus.New()), + logger: logrus.New(), slugs: fixtures.ServerServiceSlugMap(), Client: mockClient, attributeNS: serverComponentAttributeNS(model.AppKindOutOfBand), @@ -676,11 +675,17 @@ func Test_ServerService_CreateUpdateServerMetadataAttributes_Update(t *testing.T serverID, _ := uuid.Parse(fixtures.TestserverID_Dell_fc167440) device := &model.Asset{ - Metadata: map[string]string{"foo": "bar"}, - Vendor: "foobar", + Metadata: map[string]string{ + "foo": "bar", + "__ss_found": "true", + }, + Vendor: "foobar", Inventory: &common.Device{ Common: common.Common{ - Metadata: map[string]string{"foo": "bar", "test": "lala"}, + Metadata: map[string]string{ + "foo": "bar", + "test": "lala", + }, }, }, } @@ -693,23 +698,19 @@ func Test_ServerService_CreateUpdateServerMetadataAttributes_Update(t *testing.T switch r.Method { case http.MethodPut: b, err := io.ReadAll(r.Body) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // unpack attributes posted by method attributes := &serverserviceapi.Attributes{} - if err = json.Unmarshal(b, attributes); err != nil { - t.Fatal(err) - } + err = json.Unmarshal(b, attributes) + require.NoError(t, err) // unpack attributes data data := map[string]string{} - if err = json.Unmarshal(attributes.Data, &data); err != nil { - t.Fatal(err) - } + err = json.Unmarshal(attributes.Data, &data) + require.NoError(t, err) - assert.Equal(t, device.Inventory.Metadata, data) + require.Equal(t, device.Inventory.Metadata, data) w.Header().Set("Content-Type", "application/json") _, _ = w.Write(fixtures.ServerServiceR6515Components_fc167440_JSON()) @@ -719,13 +720,17 @@ func Test_ServerService_CreateUpdateServerMetadataAttributes_Update(t *testing.T }, ) + handler.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + t.Logf("unhandled URL: %s", r.URL) + w.WriteHeader(404) + }) + mock := httptest.NewServer(handler) + t.Logf("mock URL: %s", mock.URL) p := testStoreInstance(t, mock.URL) err := p.createUpdateServerMetadataAttributes(context.TODO(), serverID, device) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) } func Test_ServerService_CreateUpdateServerBMCErrorAttributes_NoErrorsNoChanges(t *testing.T) { diff --git a/internal/store/serverservice/components_test.go b/internal/store/serverservice/components_test.go index 06238b25..02ed6da7 100644 --- a/internal/store/serverservice/components_test.go +++ b/internal/store/serverservice/components_test.go @@ -41,7 +41,7 @@ func Test_ToComponentSlice(t *testing.T) { mock := httptest.NewServer(handler) p := testStoreInstance(t, mock.URL) - p.logger = logrus.NewEntry(logrus.New()) + p.logger = logrus.New() p.slugs = fixtures.ServerServiceSlugMap() p.attributeNS = serverComponentAttributeNS(model.AppKindOutOfBand) p.firmwareVersionedAttributeNS = serverComponentFirmwareNS(model.AppKindOutOfBand) diff --git a/internal/store/serverservice/helpers.go b/internal/store/serverservice/helpers.go index 597e9eb5..6db14cc6 100644 --- a/internal/store/serverservice/helpers.go +++ b/internal/store/serverservice/helpers.go @@ -30,7 +30,7 @@ var ( // TODO move this under an interface // NewServerServiceClient instantiates and returns a serverService client -func NewServerServiceClient(ctx context.Context, cfg *app.ServerserviceOptions, logger *logrus.Entry) (*serverserviceapi.Client, error) { +func NewServerServiceClient(ctx context.Context, cfg *app.ServerserviceOptions, logger *logrus.Logger) (*serverserviceapi.Client, error) { if cfg == nil { return nil, errors.Wrap(ErrConfig, "configuration is nil") } @@ -43,7 +43,7 @@ func NewServerServiceClient(ctx context.Context, cfg *app.ServerserviceOptions, } // returns a serverservice retryable client with Otel -func newServerserviceClientWithOtel(cfg *app.ServerserviceOptions, endpoint string, logger *logrus.Entry) (*serverserviceapi.Client, error) { +func newServerserviceClientWithOtel(cfg *app.ServerserviceOptions, endpoint string, logger *logrus.Logger) (*serverserviceapi.Client, error) { if cfg == nil { return nil, errors.Wrap(ErrConfig, "configuration is nil") } @@ -69,13 +69,6 @@ func newServerserviceClientWithOtel(cfg *app.ServerserviceOptions, endpoint stri // set retryable HTTP client to be the otel http client to collect telemetry retryableClient.HTTPClient = otelhttp.DefaultClient - // disable default debug logging on the retryable client - if logger.Level < logrus.DebugLevel { - retryableClient.Logger = nil - } else { - retryableClient.Logger = logger - } - // requests taking longer than timeout value should be canceled. client := retryableClient.StandardClient() client.Timeout = timeout @@ -88,24 +81,19 @@ func newServerserviceClientWithOtel(cfg *app.ServerserviceOptions, endpoint stri } // returns a serverservice retryable http client with Otel and Oauth wrapped in -func newServerserviceClientWithOAuthOtel(ctx context.Context, cfg *app.ServerserviceOptions, endpoint string, logger *logrus.Entry) (*serverserviceapi.Client, error) { +func newServerserviceClientWithOAuthOtel(ctx context.Context, cfg *app.ServerserviceOptions, endpoint string, logger *logrus.Logger) (*serverserviceapi.Client, error) { if cfg == nil { return nil, errors.Wrap(ErrConfig, "configuration is nil") } + logger.Info("serverservice client ctor") + // init retryable http client retryableClient := retryablehttp.NewClient() // set retryable HTTP client to be the otel http client to collect telemetry retryableClient.HTTPClient = otelhttp.DefaultClient - // disable default debug logging on the retryable client - if logger.Level < logrus.DebugLevel { - retryableClient.Logger = nil - } else { - retryableClient.Logger = logger - } - // setup oidc provider provider, err := oidc.NewProvider(ctx, cfg.OidcIssuerEndpoint) if err != nil { diff --git a/internal/store/serverservice/serverservice.go b/internal/store/serverservice/serverservice.go index 11371458..46ce34ab 100644 --- a/internal/store/serverservice/serverservice.go +++ b/internal/store/serverservice/serverservice.go @@ -31,7 +31,7 @@ const ( // Store is an asset inventory store type Store struct { *serverserviceapi.Client - logger *logrus.Entry + logger *logrus.Logger config *app.ServerserviceOptions slugs map[string]*serverserviceapi.ServerComponentType firmwares map[string][]*serverserviceapi.ComponentFirmwareVersion @@ -44,12 +44,9 @@ type Store struct { // NewStore returns a serverservice store queryor to lookup and publish assets to, from the store. func New(ctx context.Context, appKind model.AppKind, cfg *app.ServerserviceOptions, logger *logrus.Logger) (*Store, error) { - loggerEntry := app.NewLogrusEntryFromLogger( - logrus.Fields{"component": "store.serverservice"}, - logger, - ) + logger.Info("serverservice store ctor") - apiclient, err := NewServerServiceClient(ctx, cfg, loggerEntry) + apiclient, err := NewServerServiceClient(ctx, cfg, logger) if err != nil { return nil, err } @@ -57,7 +54,7 @@ func New(ctx context.Context, appKind model.AppKind, cfg *app.ServerserviceOptio s := &Store{ Client: apiclient, appKind: appKind, - logger: loggerEntry, + logger: logger, config: cfg, slugs: make(map[string]*serverserviceapi.ServerComponentType), firmwares: make(map[string][]*serverserviceapi.ComponentFirmwareVersion), @@ -347,21 +344,6 @@ func (r *Store) createUpdateServerComponents(ctx context.Context, serverID uuid. return errors.Wrap(model.ErrInventoryQuery, err.Error()) } - // For debugging and to capture test fixtures data. - if os.Getenv(model.EnvVarDumpFixtures) == "true" { - current := serverID.String() + ".current.components.fixture" - r.logger.Info("components current fixture dumped as file: " + current) - - // nolint:gomnd // file permissions are clearer in this form. - _ = os.WriteFile(current, []byte(litter.Sdump(currentInventory)), 0o600) - - newc := serverID.String() + ".new.components.fixture" - r.logger.Info("components new fixture dumped as file: " + newc) - - // nolint:gomnd // file permissions are clearer in this form. - _ = os.WriteFile(newc, []byte(litter.Sdump(newInventory)), 0o600) - } - // convert to a pointer slice since this data is passed around currentInventoryPtrSlice := componentPtrSlice(currentInventory)