fix metadata updates (#182)

Author: DoctorVin

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

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))

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"
 		}
 	}
 

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) {

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)

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 {

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,20 +44,17 @@ 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
 	}
 
 	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)