Add collector to get pci device data #320

brett060102 · 2025-07-17T13:22:26Z

Add collection of lsmod and PCI data to the hwdata infor block

This done by adding a collector that will invoke
lspci -s .0 and lsmod

Requires adding pciutils to required package for suseconnect

The size of the additional payload in the heartbeat depends on how many pci devices are on the system and
number of modules loaded into the kernel. See:
https://confluence.suse.com/display/publiccloud/Design+-+PCI+Data+Collection
For more information.

mssola

In a more general comment, please use goimports/gofmt. As in, your Go environment does not align at all with Go's coding conventions. This is something that should've been detected via our CI, but we never bothered to do so 😅 Hence, take a look at that, and run make check-format to ensure that your code is up to Go's coding conventions.

cmd/suseconnect/suseconnect.go

internal/collectors/lsmod.go

internal/collectors/pci.go

brett060102 · 2025-08-11T19:12:40Z

Thank you for "gofmt" reference. I did not know about it. Does make things easier.

build/packaging/suseconnect-ng.spec

mssola · 2025-08-12T05:47:47Z

By the way, does the WIP label still apply for this PR?

brett060102 · 2025-08-12T19:49:13Z

yes, this should stay WIP for now. We don't want this to land until we know how it would effect RMT's and SCC.

mssola · 2025-08-13T07:59:54Z

Ok, then I will not review this further. Ping me again when you need another review.

brett060102 · 2025-08-13T14:45:59Z

@mssola Thank you very much for all of your comments and inputs on this. It has helped a lot.

internal/connect/wrapper.go

cmd/suseconnect/suseconnect.go

internal/collectors/lsmod.go

internal/collectors/pci.go

internal/connect/collectors.go

internal/util/profiles.go

mssola · 2025-10-17T12:53:57Z

cmd/suseconnect/suseconnect.go

 		os.Exit(0)
 	} else if deRegister {
+		// Need to clear ProfileCache on deregister
+		// even if dereg does not succeed.


Nit: didn't this comment fit into a single line?

mssola · 2025-10-17T12:54:04Z

cmd/suseconnect/suseconnect.go

 			exitOnError(errors.New("cannot use the json option with the 'cleanup' command"), api, opts)
 		}
+		// Need to clear ProfileCache on cleanup
+		// even if cleanup does not succeed.


mssola · 2025-10-17T12:54:22Z

cmd/suseconnect/suseconnect.go


 			err := connect.Register(api, opts)
 			if err != nil {
+				// clear profile cache on registration errors


Nit: capitalize first word.

mssola · 2025-10-17T12:54:59Z

cmd/suseconnect/suseconnect.go

 }

 func exitOnError(err error, api connect.WrappedAPI, opts *connect.Options) {
+	util.Debug.Println("exitOnError err: ", err)


Isn't this handled on each if block so it's more specific?

mssola · 2025-10-17T12:56:55Z

internal/collectors/collectors.go

 )

-type Result = map[string]interface{}
+type Result = util.Result


This Result could probably go into pkg/, as it's used in different places. But also bear in mind my comments on profiles 😉

mssola · 2025-10-17T13:27:22Z

internal/util/profiles.go

@@ -0,0 +1,124 @@
+package util


All of this probably belongs to pkg/, as users of this library might be interested on this very topic as well. Note that for some time we also wanted to export the collectors into this public library, but we ended up too busy with other stuff 😄

As for SUSEConnect-specific things, maybe they can be brought in as parameters? Note that you are already passing arguments such us cacheFile.

mssola · 2025-10-17T13:29:58Z

internal/util/profiles.go

+	ProfileData any    `json:"profileData"` // Data associated with ProfileI
+}
+
+func UpdateCache(updateCache bool, tag string, cacheFile string, dataBlock any) (Result, error) {


Exported function needs documentation. Other than that:

To me it feels weird to have a function called UpdateCache which has as a first parameter a boolean on whether you want to update the cache or not. This feels off.

Maybe instead of tag, identifier, or profileName?

In the documentation, please specify whether cacheFile is just the name, the full path, etc.

mssola · 2025-10-17T13:34:14Z

internal/util/profiles.go

+//
+// ProfileData: can be any data such array of strings, a single large string
+type Profile struct {
+	ProfileID   string `json:"profileId"`   // Sha256 of ProfileData


Don't document members inline. But other than that, the naming feels redundant. As in, if the struct is named Profile, there's no need to include Profile* in the member's names. Moreover, this is not a profile "id", but a checksum, as you clearly document. Hence, why not simply:

type Profile struct { Checksum string `json:"checksum"` Data string `json:"data"` }

This way documentation is not even needed as it pretty clear what each thing refers to, and anyways the documentation of the type you have provided is more than enough.

Also note that I turned Data into a string, as that's good enough. Users of the Profile struct should do the marshaling themselves, but looking at the code, everything is a string anyways, so there's no need for marshaling nor to be cryptic about the type.

mssola · 2025-10-17T13:36:54Z

internal/util/profiles.go

+func UpdateCache(updateCache bool, tag string, cacheFile string, dataBlock any) (Result, error) {
+
+	var hashInput = ""
+	switch v := dataBlock.(type) {


As detailed before, let it be a string and avoid type casting shenanigans.

mssola · 2025-10-17T13:37:26Z

internal/util/profiles.go

+func SetProfileFilePath(newPath string) {
+	profileFilePath = newPath
+	if profileFilePath[len(profileFilePath)-1] != '/' {
+		profileFilePath = profileFilePath + "/"


Use filepath.Join instead.

and some debug to wrapper

brett060102 added the wip Work in progress label Jul 17, 2025

brett060102 mentioned this pull request Jul 17, 2025

Add collector to get pci device data #319

Closed

brett060102 force-pushed the pci-collector branch from 292be7c to 1ece975 Compare August 11, 2025 13:52

mssola requested changes Aug 11, 2025

View reviewed changes

cmd/suseconnect/suseconnect.go Outdated Show resolved Hide resolved

internal/collectors/lsmod.go Outdated Show resolved Hide resolved

internal/collectors/lsmod.go Outdated Show resolved Hide resolved

internal/collectors/pci.go Outdated Show resolved Hide resolved

brett060102 force-pushed the pci-collector branch from 1ece975 to 82b34b2 Compare August 11, 2025 20:36

brett060102 changed the base branch from main to next August 11, 2025 20:38

mssola reviewed Aug 12, 2025

View reviewed changes

build/packaging/suseconnect-ng.spec Show resolved Hide resolved

brett060102 force-pushed the pci-collector branch from 82b34b2 to 5585dd7 Compare August 12, 2025 22:52

brett060102 force-pushed the pci-collector branch 2 times, most recently from b8e8271 to 06e5d40 Compare August 13, 2025 16:49

mssola force-pushed the next branch 2 times, most recently from ba2362b to 3b96875 Compare August 19, 2025 10:58

brett060102 force-pushed the pci-collector branch 2 times, most recently from 677ec83 to a82952b Compare August 27, 2025 06:19

felixsch force-pushed the next branch from 3b96875 to ce22198 Compare September 2, 2025 09:53

Base automatically changed from next to main September 2, 2025 10:15

brett060102 force-pushed the pci-collector branch 7 times, most recently from 3cc85cd to ff353d9 Compare September 9, 2025 19:23

mssola reviewed Sep 10, 2025

View reviewed changes

brett060102 force-pushed the pci-collector branch from ff353d9 to 94faef2 Compare September 10, 2025 14:10

brett060102 force-pushed the pci-collector branch 9 times, most recently from 16cf9bc to 4dcd889 Compare September 16, 2025 15:56

brett060102 force-pushed the pci-collector branch 4 times, most recently from ce9ae58 to 92c1d95 Compare October 1, 2025 17:23

rtamalin reviewed Oct 1, 2025

View reviewed changes

internal/util/profiles.go Outdated Show resolved Hide resolved

rtamalin reviewed Oct 1, 2025

View reviewed changes

internal/util/profiles.go Outdated Show resolved Hide resolved

rtamalin reviewed Oct 1, 2025

View reviewed changes

internal/util/profiles.go Outdated Show resolved Hide resolved

rtamalin reviewed Oct 1, 2025

View reviewed changes

internal/util/profiles.go Outdated Show resolved Hide resolved

brett060102 force-pushed the pci-collector branch from 92c1d95 to 0e16f52 Compare October 1, 2025 20:38

brett060102 force-pushed the pci-collector branch 2 times, most recently from e213f15 to 8e70bb4 Compare October 16, 2025 18:33

mssola requested changes Oct 17, 2025

View reviewed changes

brett060102 force-pushed the pci-collector branch 3 times, most recently from 9c801a3 to bb90fb7 Compare October 17, 2025 20:22

Add collector to get pci and lsmod data

45dc5e0

and some debug to wrapper

brett060102 force-pushed the pci-collector branch from bb90fb7 to 45dc5e0 Compare October 17, 2025 20:44

Add collector to get pci device data #320

Are you sure you want to change the base?

Add collector to get pci device data #320

Uh oh!

Conversation

brett060102 commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brett060102 commented Aug 11, 2025

Uh oh!

Uh oh!

mssola commented Aug 12, 2025

Uh oh!

brett060102 commented Aug 12, 2025

Uh oh!

mssola commented Aug 13, 2025

Uh oh!

brett060102 commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brett060102 commented Jul 17, 2025 •

edited

Loading