NO-JIRA: performance profile: internal cleanup (#1359)

* profilecreator: dissolve helpers into subpackages

reorganize the current helper.go dissolving it into
specific subpackages. The goal is tstreamline the
code layout without changes in behavior.

Signed-off-by: Francesco Romani <[email protected]>

* profilecreator: simplify package layout

the `cmd/pkg` layout is awkward. Canonically:
- code used only by a tool should sit in `internal/`
- code which is/can be exported should sit in `pkg`.

However, in case of nontrivial command line utilities,
it's a widespread practice to have `pkg/cmd` (or `pkg/commands`) trees.
This enable third party utilities to easily embed other utilities
maximizing code reuse.

We streamline the code layout with trivial code
movements - no intended changes of behavior.

The net result is now `cmd/performance-profile-creator` is now
minified, and holds only the entry point source code file.

Signed-off-by: Francesco Romani <[email protected]>

* profilecreator: chore: refactor profilecreator.go

extract code which deals with mustgather or ghw in their
own source files. Trivial code movement with no changes in behavior.

Signed-off-by: Francesco Romani <[email protected]>

* profilecreator: improvements in the `info` command

- add pure text output command. By default (and it's very hard to
  change, if we ever should) log messages go on stderr. We want the
  output on stdout. Added flag `--text` to enable it
- internal cleanup and code reorganization. In log mode, log all the
  message in one go, which is easier to manage.

No intended change in behavior.

Signed-off-by: Francesco Romani <[email protected]>

* profilecreator: add and use Alert function

We used the `logrus` package internally, but minimally
and pretty much only to notify processing status to the user.
This code was thus the only remaining consumer of that package,
which we don't really need.

Add a neutral (nor log, nor warning) minimal abstraction `Alert`,
similar in spirit to `ghw`, to be used when we need to notify the
user about processing status.

Make the backend easy to replace to make integration of this code
easier. We don't see this happening, but the extra cost is negligible.

Remove the direct dependency to `logrus`.
Note we still have quite many indirect dependencies pulling it in.

Signed-off-by: Francesco Romani <[email protected]>

---------

Signed-off-by: Francesco Romani <[email protected]>

Author: ffromani

cmd/performance-profile-creator/main.go

@@ -19,7 +19,7 @@ package main
 import (
 	"os"
 
-	"github.com/openshift/cluster-node-tuning-operator/cmd/performance-profile-creator/cmd"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/profilecreator/cmd"
 )
 
 func main() {

go.mod

@@ -26,7 +26,6 @@ require (
 	github.com/openshift/library-go v0.0.0-20240419113445-f1541d628746
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.21.1
-	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/cobra v1.9.1
 	github.com/spf13/pflag v1.0.6
 	gopkg.in/fsnotify.v1 v1.4.7

go.sum

@@ -434,8 +434,6 @@ github.com/rwcarlsen/goexif v0.0.0-20190401172101-9e8deecbddbd/go.mod h1:hPqNNc0
 github.com/sirupsen/logrus v1.4.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.4.1/go.mod h1:ni0Sbl8bgC9z8RoU9G6nDWqqs/fq4eDPysMBDgk/93Q=
 github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE=
-github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ=
-github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
 github.com/spf13/cobra v0.0.3/go.mod h1:1l0Ry5zgKvJasoi3XT1TypsSe7PqH0Sj9dhYf7v3XqQ=
 github.com/spf13/cobra v1.9.1 h1:CXSaggrXdbHK9CF+8ywj8Amf7PBRmPCOJugH954Nnlo=
 github.com/spf13/cobra v1.9.1/go.mod h1:nDyEzZ8ogv936Cinf6g1RU9MRY64Ir93oCnqb9wxYW0=
@@ -686,7 +684,6 @@ golang.org/x/sys v0.0.0-20220310020820-b874c991c1a5/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220422013727-9388b58f7150/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

pkg/performanceprofile/profilecreator/alert.go

@@ -0,0 +1,30 @@
+package profilecreator
+
+import (
+	"log"
+	"os"
+)
+
+var alertSink *log.Logger
+
+func init() {
+	// intentionally no flag for minimal output
+	alertSink = log.New(os.Stderr, "PPC: ", 0)
+}
+
+// Alert raise status messages. `profilecreator` is meant to run interactively,
+// so the default behavior is to bubble up the messages on the user console.
+// It is however possible to reroute the messages to any log.Logger compliant backend
+// see the function SetAlertSink. All Alerts are considered Info messages.
+func Alert(format string, values ...any) {
+	alertSink.Printf(format, values...)
+}
+
+// Call this function before any other else in the `profilecreator` package
+func SetAlertSink(lh *log.Logger) {
+	alertSink = lh
+}
+
+func GetAlertSink() *log.Logger {
+	return alertSink
+}

cmd/performance-profile-creator/cmd/pkg/hypershift/hypershift.go renamed to pkg/performanceprofile/profilecreator/cmd/hypershift/hypershift.go

@@ -5,8 +5,8 @@ import (
 	"fmt"
 	"os"
 	"sort"
+	"strings"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
@@ -17,6 +17,7 @@ const (
 
 type infoOptions struct {
 	jsonOutput bool
+	textOutput bool
 }
 
 // NewInfoCommand return info command, which provides a list of the cluster nodes and their hardware topology.
@@ -31,6 +32,7 @@ func NewInfoCommand(pcArgs *ProfileCreatorArgs) *cobra.Command {
 		},
 	}
 	info.Flags().BoolVar(&opts.jsonOutput, "json", false, "output as JSON")
+	info.Flags().BoolVar(&opts.textOutput, "text", false, "output as plain text")
 	return info
 }
 
@@ -40,12 +42,8 @@ func executeInfoMode(mustGatherDirPath string, createForHypershift bool, infoOpt
 		return fmt.Errorf("failed to parse the cluster data: %w", err)
 	}
 	clusterInfo := makeClusterInfoFromClusterData(clusterData)
-	if infoOpts.jsonOutput {
-		if err := showClusterInfoJSON(clusterInfo); err != nil {
-			return fmt.Errorf("unable to show cluster info %w", err)
-		}
-	} else {
-		showClusterInfoLog(clusterInfo)
+	if err := showClusterInfo(clusterInfo, infoOpts); err != nil {
+		return fmt.Errorf("unable to show cluster info %w", err)
 	}
 	return nil
 }
@@ -96,13 +94,13 @@ func makeClusterInfoFromClusterData(cluster ClusterData) ClusterInfo {
 		for _, handle := range nodeHandlers {
 			topology, err := handle.SortedTopology()
 			if err != nil {
-				log.Infof("%s(Topology discovery error: %v)", handle.Node.GetName(), err)
+				Alert("%s(Topology discovery error: %v)", handle.Node.GetName(), err)
 				continue
 			}
 
 			htEnabled, err := handle.IsHyperthreadingEnabled()
 			if err != nil {
-				log.Infof("%s(HT discovery error: %v)", handle.Node.GetName(), err)
+				Alert("%s(HT discovery error: %v)", handle.Node.GetName(), err)
 			}
 
 			nInfo := NodeInfo{
@@ -128,21 +126,36 @@ func makeClusterInfoFromClusterData(cluster ClusterData) ClusterInfo {
 	return cInfo.Sort()
 }
 
-func showClusterInfoJSON(cInfo ClusterInfo) error {
-	return json.NewEncoder(os.Stdout).Encode(cInfo)
+func showClusterInfo(cInfo ClusterInfo, infoOpts *infoOptions) error {
+	if infoOpts.jsonOutput {
+		return json.NewEncoder(os.Stdout).Encode(cInfo)
+	}
+	textInfo := dumpClusterInfo(cInfo)
+	if infoOpts.textOutput {
+		fmt.Print(textInfo)
+		return nil
+	}
+	Alert("Cluster info:\n%s", textInfo)
+	return nil
 }
 
-func showClusterInfoLog(cInfo ClusterInfo) {
-	log.Infof("Cluster info:")
+func dumpClusterInfo(cInfo ClusterInfo) string {
+	var sb strings.Builder
 	for _, mcpInfo := range cInfo {
-		log.Infof("MCP '%s' nodes:", mcpInfo.Name)
+		fmt.Fprintf(&sb, "MCP '%s' nodes:", mcpInfo.Name)
+		sb.WriteString("\n")
 		for _, nInfo := range mcpInfo.Nodes {
-			log.Infof("Node: %s (NUMA cells: %d, HT: %v)", nInfo.Name, len(nInfo.NUMACells), nInfo.HTEnabled)
+			fmt.Fprintf(&sb, "Node: %s (NUMA cells: %d, HT: %v)", nInfo.Name, len(nInfo.NUMACells), nInfo.HTEnabled)
+			sb.WriteString("\n")
 			for _, cInfo := range nInfo.NUMACells {
-				log.Infof("NUMA cell %d : %v", cInfo.ID, cInfo.CoreList)
+				fmt.Fprintf(&sb, "NUMA cell %d : %v", cInfo.ID, cInfo.CoreList)
+				sb.WriteString("\n")
 			}
-			log.Infof("CPU(s): %d", nInfo.CPUsCount)
+			fmt.Fprintf(&sb, "CPU(s): %d", nInfo.CPUsCount)
+			sb.WriteString("\n")
 		}
-		log.Infof("---")
+		fmt.Fprintf(&sb, "---")
+		sb.WriteString("\n")
 	}
+	return sb.String()
 }

cmd/performance-profile-creator/cmd/info.go renamed to pkg/performanceprofile/profilecreator/cmd/info.go

@@ -18,31 +18,28 @@ package cmd
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
-	"io"
 	"os"
 	"strings"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	serializer "k8s.io/apimachinery/pkg/runtime/serializer/json"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	kubeletconfig "k8s.io/kubelet/config/v1beta1"
 	"k8s.io/utils/ptr"
-	"sigs.k8s.io/yaml"
 
 	machineconfigv1 "github.com/openshift/api/machineconfiguration/v1"
-	"github.com/openshift/cluster-node-tuning-operator/cmd/performance-profile-creator/cmd/pkg/hypershift"
 	performancev2 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/performanceprofile/v2"
 	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/profilecreator"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/profilecreator/cmd/hypershift"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/profilecreator/serialize"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/profilecreator/toleration"
 )
 
 const (
@@ -107,11 +104,10 @@ type ProfileData struct {
 // ClusterData collects the cluster wide information, each mcp points to a list of ghw node handlers
 type ClusterData map[string][]*profilecreator.GHWHandler
 
+// shortcut
+var Alert = profilecreator.Alert
+
 func init() {
-	log.SetOutput(os.Stderr)
-	log.SetFormatter(&log.TextFormatter{
-		DisableTimestamp: true,
-	})
 	utilruntime.Must(performancev2.AddToScheme(scheme))
 }
 
@@ -128,7 +124,7 @@ func NewRootCommand() *cobra.Command {
 		"must-gather-dir-path",
 	}
 
-	tolerations := profilecreator.TolerationSet{}
+	tols := toleration.Set{}
 
 	root := &cobra.Command{
 		Use:   "performance-profile-creator",
@@ -164,7 +160,7 @@ func NewRootCommand() *cobra.Command {
 				return err
 			}
 
-			err = profilecreator.EnsureNodesHaveTheSameHardware(nodesHandlers, tolerations)
+			err = profilecreator.EnsureNodesHaveTheSameHardware(nodesHandlers, tols)
 			if err != nil {
 				return fmt.Errorf("targeted nodes differ: %w", err)
 			}
@@ -175,12 +171,17 @@ func NewRootCommand() *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("failed to make profile data from node handler: %w", err)
 			}
-			tolerations[profilecreator.EnableHardwareTuning] = profileData.enableHardwareTuning
+			tols[toleration.EnableHardwareTuning] = profileData.enableHardwareTuning
 			profile, err := makePerformanceProfileFrom(*profileData)
 			if err != nil {
 				return err
 			}
-			return writeProfile(profile, tolerations)
+			profData, err := serialize.Profile(profile, tols)
+			if err != nil {
+				return err
+			}
+			_, err = fmt.Println(profData)
+			return err
 		},
 	}
 	pcArgs.AddFlags(root.PersistentFlags())
@@ -201,7 +202,7 @@ func makeNodesHandlers(mustGatherDirPath, poolName string, nodes []*corev1.Node)
 		sb.WriteString(node.Name + " ")
 	}
 	// NodePoolName is alias of MCPName
-	log.Infof("Nodes names targeted by %s pool are: %s", poolName, sb.String())
+	Alert("Nodes names targeted by %s pool are: %s", poolName, sb.String())
 	return handlers, nil
 }
 
@@ -311,8 +312,8 @@ func makeProfileDataFrom(nodeHandler *profilecreator.GHWHandler, args *ProfileCr
 	if err != nil {
 		return nil, fmt.Errorf("failed to compute the reserved and isolated CPUs: %v", err)
 	}
-	log.Infof("%d reserved CPUs allocated: %v ", reservedCPUs.Size(), reservedCPUs.String())
-	log.Infof("%d isolated CPUs allocated: %v", isolatedCPUs.Size(), isolatedCPUs.String())
+	Alert("%d reserved CPUs allocated: %v ", reservedCPUs.Size(), reservedCPUs.String())
+	Alert("%d isolated CPUs allocated: %v", isolatedCPUs.Size(), isolatedCPUs.String())
 	kernelArgs := profilecreator.GetAdditionalKernelArgs(args.DisableHT)
 	profileData := &ProfileData{
 		reservedCPUs:              reservedCPUs.String(),
@@ -521,93 +522,6 @@ func makePerformanceProfileFrom(profileData ProfileData) (runtime.Object, error)
 	return profile, nil
 }
 
-func writeProfile(obj runtime.Object, tolerations profilecreator.TolerationSet) error {
-	// write CSV to out dir
-	writer := strings.Builder{}
-	if err := MarshallObject(obj, &writer); err != nil {
-		return err
-	}
-
-	if tolerations[profilecreator.EnableHardwareTuning] {
-		if _, err := writer.Write([]byte(profilecreator.HardwareTuningMessage)); err != nil {
-			return err
-		}
-	}
-
-	if tolerations[profilecreator.DifferentCoreIDs] {
-		if _, err := writer.Write([]byte(profilecreator.DifferentCoreIDsMessage)); err != nil {
-			return err
-		}
-	}
-
-	fmt.Printf("%s", writer.String())
-	return nil
-}
-
-// MarshallObject mashals an object, usually a CSV into YAML
-func MarshallObject(obj interface{}, writer io.Writer) error {
-	jsonBytes, err := json.Marshal(obj)
-	if err != nil {
-		return err
-	}
-
-	var r unstructured.Unstructured
-	if err := json.Unmarshal(jsonBytes, &r.Object); err != nil {
-		return err
-	}
-
-	// remove status and metadata.creationTimestamp
-	unstructured.RemoveNestedField(r.Object, "metadata", "creationTimestamp")
-	unstructured.RemoveNestedField(r.Object, "template", "metadata", "creationTimestamp")
-	unstructured.RemoveNestedField(r.Object, "spec", "template", "metadata", "creationTimestamp")
-	unstructured.RemoveNestedField(r.Object, "status")
-
-	deployments, exists, err := unstructured.NestedSlice(r.Object, "spec", "install", "spec", "deployments")
-	if err != nil {
-		return err
-	}
-	if exists {
-		for _, obj := range deployments {
-			deployment := obj.(map[string]interface{})
-			unstructured.RemoveNestedField(deployment, "metadata", "creationTimestamp")
-			unstructured.RemoveNestedField(deployment, "spec", "template", "metadata", "creationTimestamp")
-			unstructured.RemoveNestedField(deployment, "status")
-		}
-		if err := unstructured.SetNestedSlice(r.Object, deployments, "spec", "install", "spec", "deployments"); err != nil {
-			return err
-		}
-	}
-
-	jsonBytes, err = json.Marshal(r.Object)
-	if err != nil {
-		return err
-	}
-
-	yamlBytes, err := yaml.JSONToYAML(jsonBytes)
-	if err != nil {
-		return err
-	}
-
-	// fix double quoted strings by removing unneeded single quotes...
-	s := string(yamlBytes)
-	s = strings.Replace(s, " '\"", " \"", -1)
-	s = strings.Replace(s, "\"'\n", "\"\n", -1)
-
-	yamlBytes = []byte(s)
-
-	_, err = writer.Write([]byte("---\n"))
-	if err != nil {
-		return err
-	}
-
-	_, err = writer.Write(yamlBytes)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
 func listNodesForPool(args *ProfileCreatorArgs) ([]*corev1.Node, error) {
 	nodes, err := profilecreator.GetNodeList(args.MustGatherDirPath)
 	if err != nil {