Cleanup Kubernetes datagather #571

inteon · 2024-09-16T17:56:02Z

Simplifications I found while reading through the code.

inteon · 2024-09-16T18:01:16Z

pkg/agent/run.go

@@ -101,7 +100,6 @@ func Run(cmd *cobra.Command, args []string) {
 	}

 	dataGatherers := map[string]datagatherer.DataGatherer{}
-	var wg sync.WaitGroup


This waitgroup was not used.

inteon · 2024-09-16T18:02:21Z

pkg/agent/run.go

-		// wait for the informer to complete an initial sync, we do this to
-		// attempt to have an initial set of data for the first upload of
-		// the run.
-		if err := newDg.WaitForCacheSync(bootCtx.Done()); err != nil {


Instead of waiting for each dataGatherer before starting to the next, we should first start all dataGatherers and loop again to wait for them to finish ( that is what I did in the updated code ).

inteon · 2024-09-16T18:03:46Z

pkg/datagatherer/k8s/dynamic.go

@@ -143,20 +143,21 @@ var kubernetesNativeResources = map[schema.GroupVersionResource]sharedInformerFu

 // NewDataGatherer constructs a new instance of the generic K8s data-gatherer for the provided
 func (c *ConfigDynamic) NewDataGatherer(ctx context.Context) (datagatherer.DataGatherer, error) {
-	cl, err := NewDynamicClient(c.KubeConfigPath)


We only have to create a dynamic client when the GVK is not a native resource.
So this function should be called as part of the else body.

Makes sense! Is it fair to say that there's not really a reason for newDataGathererWithClient to be a separate function? It's fishy that we call it with one of cl or clientset with the other set to nil.

Not a requirement in this PR, of course!

inteon · 2024-09-16T18:05:23Z

pkg/datagatherer/k8s/dynamic.go

 	}

-	factory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(


We are creating these factories to create just 1 informer.
I still create the factory ( maybe this can be simplified in the future ), but I now let it go out-of-scope and continue with the created informer instead.

inteon · 2024-09-17T08:26:27Z

pkg/datagatherer/k8s/dynamic.go

-// namespaceResourceInterface will 'namespace' a NamespaceableResourceInterface
-// if the 'namespace' parameter is non-empty, otherwise it will return the
-// given ResourceInterface as-is.
-func namespaceResourceInterface(iface dynamic.NamespaceableResourceInterface, namespace string) dynamic.ResourceInterface {


This function was unused.

inteon · 2024-09-17T08:27:29Z

pkg/agent/run.go

@@ -145,19 +141,24 @@ func Run(cmd *cobra.Command, args []string) {
 		dataGatherers[dgConfig.Name] = newDg
 	}

-	// wait for initial sync period to complete. if unsuccessful, then crash


This logic was not doing anything, since the WaitGroup was not being used.

…sed fields and duplicate code Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

SgtCoDFish

/lgtm
/approve

The only reason I'm even a little concerned about this is that I don't really know how the agent is tested and the extent to which it's tested. On the assumption that it is well tested I'm happy to approve!

This is really good, thank you!

SgtCoDFish · 2024-09-17T17:10:50Z

pkg/datagatherer/k8s/dynamic.go

@@ -143,20 +143,21 @@ var kubernetesNativeResources = map[schema.GroupVersionResource]sharedInformerFu

 // NewDataGatherer constructs a new instance of the generic K8s data-gatherer for the provided
 func (c *ConfigDynamic) NewDataGatherer(ctx context.Context) (datagatherer.DataGatherer, error) {
-	cl, err := NewDynamicClient(c.KubeConfigPath)


Makes sense! Is it fair to say that there's not really a reason for newDataGathererWithClient to be a separate function? It's fishy that we call it with one of cl or clientset with the other set to nil.

Not a requirement in this PR, of course!

maelvls · 2024-09-30T14:13:18Z

pkg/agent/run.go

+	defer func() {
+		// TODO: replace Fatalf log calls with Errorf and return the error
+		cancel()
+		if err := group.Wait(); err != nil {
+			logs.Log.Fatalf("failed to wait for controller-runtime component to stop: %v", err)
+		}
+	}()


What is this doing? It seems like we now cancel immediately, and then block on group.Wait?

Indeed, we cancel the context to start the shutdown and wait for the shutdown to complete using group.Wait

Why do we try to shutdown here? I don't understand the purpose of this defer func.

From what I can tell, it should be the opposite: if the errgroup ever ends, cancel the context so that the other things stop.

An error group will always cancel its context when one of the goroutines returns an error.

For successful execution, we only reach this defer call when the following for loop ends because OneShot is true:

jetstack-secure/pkg/agent/run.go

Lines 167 to 181 in 7f756c3

for {

// if period is set in the config, then use that if not already set

if Flags.Period == 0 && config.Period > 0 {

logs.Log.Printf("Using period from config %s", config.Period)

Flags.Period = config.Period

}

gatherAndOutputData(config, preflightClient, dataGatherers)

if Flags.OneShot {

break

}

time.Sleep(Flags.Period)

}

If OneShot is false, the loop will keep looping.

In my opinion, this information should be written down as a comment in code since this section of code isn't obvious.

Sorry about that, I was reading go func instead of defer func... It now makes sense.

inteon commented Sep 16, 2024

View reviewed changes

inteon force-pushed the cleanup_datagather branch 3 times, most recently from 626b018 to c3b59f1 Compare September 17, 2024 08:14

inteon commented Sep 17, 2024

View reviewed changes

cleanup the Kubernetes-related code in the datagatherer, removing unu…

7f756c3

…sed fields and duplicate code Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the cleanup_datagather branch from c3b59f1 to 7f756c3 Compare September 17, 2024 13:23

SgtCoDFish approved these changes Sep 17, 2024

View reviewed changes

inteon merged commit 924390f into master Sep 18, 2024
8 checks passed

SgtCoDFish deleted the cleanup_datagather branch September 18, 2024 09:08

maelvls reviewed Sep 30, 2024

View reviewed changes

		}

		factory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(

	for {
	// if period is set in the config, then use that if not already set
	if Flags.Period == 0 && config.Period > 0 {
	logs.Log.Printf("Using period from config %s", config.Period)
	Flags.Period = config.Period
	}

	gatherAndOutputData(config, preflightClient, dataGatherers)

	if Flags.OneShot {
	break
	}

	time.Sleep(Flags.Period)
	}

Cleanup Kubernetes datagather #571

Cleanup Kubernetes datagather #571

Uh oh!

Conversation

inteon commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inteon Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SgtCoDFish left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

inteon commented Sep 16, 2024 •

edited

Loading

inteon Sep 16, 2024 •

edited

Loading

SgtCoDFish left a comment •

edited

Loading

maelvls Oct 7, 2024 •

edited

Loading

maelvls Nov 4, 2024 •

edited

Loading