[Asset Inventory] Run fetchers periodically #2902

kubasobon · 2025-01-07T11:42:45Z

Summary of your changes

Fixes an issue where fetchers ran only once.

Related Issues

Checklist

I have added tests that prove my fix is effective or that my feature works
I have added the necessary README/documentation (if appropriate)

mergify · 2025-01-07T11:43:20Z

This pull request does not have a backport label. Could you fix it @kubasobon? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
NOTE: backport-v8.x has been added to help with the transition to the new branch 8.x.

romulets

So, has it never worked? 😬

romulets · 2025-01-07T14:00:51Z

internal/inventory/inventory.go

+				go func(fetcher AssetFetcher) {
+					fetcher.Fetch(ctx, a.assetCh)
+				}(fetcher)
+			}


Shouldn't we remove the the fetcher loop on top of this function?

Well, no. I added this so all fetchers run every fetcherPeriod tick. But I think canceled context takes precedence. If it's canceled for whatever reason, let's not run any more fetchers.

And the extra one on top is so that it starts immediately, on second 0. Otherwise it would take until the first fetcherPeriod tick to run first fetchers.

Hm true, just checked the Ticker and we can't make it run straight away.

Nitpick: Maybe move this code to a function? Just so we have a nice wrapped function, it prevents if in the future we need to fix something and we do in one place only

A note: in manager (used in fetching of cspm mostly), we use a timer, first with a 0 value and then reset with the period value, to do the trick of "trigger now and then per period".

cloudbeat/internal/resources/fetching/manager/manager.go

Lines 72 to 87 in b493e0c

timer := time.NewTimer(0)

defer timer.Stop()

for {

select {

case <-ctx.Done():

m.log.Info("Fetchers manager canceled")

return

case <-timer.C:

// update the interval

timer.Reset(m.interval)

// this is blocking so the stop will not be called until all the fetchers are finished

// in case there is a blocking fetcher it will halt (til the m.timeout)

go m.fetchIteration(ctx)

}

}

I'm not saying we should do the same with asset inventory since I think the ticker is better for what we want.

Thanks. Yeah, I know the trick, but I always find calling timer.Reset() every time more messy. Nothing against the implementation, just personal taste I guess.

kubasobon · 2025-01-07T14:10:47Z

Yes, unfortunately it seems like it always ran just once and that's it. See #2887

romulets

I left a suggestion, but looks good otherwise

romulets · 2025-01-08T09:19:59Z

internal/inventory/inventory.go

+				go func(fetcher AssetFetcher) {
+					fetcher.Fetch(ctx, a.assetCh)
+				}(fetcher)
+			}


Hm true, just checked the Ticker and we can't make it run straight away.

Nitpick: Maybe move this code to a function? Just so we have a nice wrapped function, it prevents if in the future we need to fix something and we do in one place only

kubasobon · 2025-01-08T09:36:54Z

I've extracted duplicate code to runAllFetchersOnce(). I hope the comment and function name make it even clearer that it's not a looping situation.

orouz · 2025-01-08T10:01:17Z

internal/inventory/inventory.go

-const indexTemplate = "logs-cloud_asset_inventory.asset_inventory-%s_%s_%s_%s-default"
+const (
+	indexTemplate = "logs-cloud_asset_inventory.asset_inventory-%s_%s_%s_%s-default"
+	minimalPeriod = 30 * time.Second


maybe we can omit this const?

locally we can tweak

cloudbeat/deploy/asset-inventory/cloudbeat-gcp-asset-inventory.yml

Line 14 in 56579fc

period: 30s

and users have a hard-coded value, for example - https://github.com/elastic/integrations/blob/56d0473a2ed4900b7b24b6a254795b9bc82cb713/packages/cloud_asset_inventory/data_stream/asset_inventory/agent/stream/gcp.yml.hbs#L1

I would rather keep it. It's not a default value, but a guardian value. If the customer sets value too low, this will start to spawn many goroutines that hit API. I can lower it if you think it makes sense, but we definitely need to make sure there is a reasonable minimum threshold. Obviously the worst-case scenario is period: 0s, where we get an infinite loop of new goroutines, but even 5s or 10s is too small IMHO.

PTAL:
https://github.com/elastic/cloudbeat/pull/2902/files#diff-151451acd3d777673443e215b546390195bf895e79500c18a34bdf0029222fbfR57-R59

np with keeping it imo. but this value isn't configurable by the users, it's hardcoded in the integration package.

(cherry picked from commit bb7a3e9)

) [Asset Inventory] Run fetchers periodically (#2902) (cherry picked from commit bb7a3e9) Co-authored-by: Kuba Soboń <[email protected]>

…2907) [Asset Inventory] Run fetchers periodically (#2902) (cherry picked from commit bb7a3e9) Co-authored-by: Kuba Soboń <[email protected]>

…2906) [Asset Inventory] Run fetchers periodically (#2902) (cherry picked from commit bb7a3e9) Co-authored-by: Kuba Soboń <[email protected]>

run asset inventory fetchers periodically

d3171b9

kubasobon self-assigned this Jan 7, 2025

mergify bot added the backport-v8.x label Jan 7, 2025

kubasobon added 7 commits January 7, 2025 12:44

update test setup

c365097

test asset inventory cycles

7f8b20b

handle PublishAll calls

638fcef

handle PublishAll calls

e6fd5af

make counter atomic

2071af8

skip in short test suite

9059026

drop explicit zero

f431f4e

kubasobon mentioned this pull request Jan 7, 2025

[Agentless] healthy agent without cloudbeat logs #2887

Closed

kubasobon marked this pull request as ready for review January 7, 2025 13:52

kubasobon requested a review from a team as a code owner January 7, 2025 13:52

kubasobon added 2 commits January 7, 2025 14:54

fix expected value

2e32a94

be consistent, use val

7f1495f

romulets reviewed Jan 7, 2025

View reviewed changes

kubasobon requested a review from romulets January 7, 2025 14:14

romulets approved these changes Jan 8, 2025

View reviewed changes

extract duplicate code to runAllFetchersOnce()

8d6a00a

kubasobon added 2 commits January 8, 2025 10:43

add test for runAllFetchersOnce

dcf2c4b

Merge branch 'main' into asset-inventory-period

5bd515f

orouz reviewed Jan 8, 2025

View reviewed changes

moukoublen approved these changes Jan 8, 2025

View reviewed changes

kubasobon merged commit bb7a3e9 into elastic:main Jan 8, 2025
8 checks passed

kubasobon deleted the asset-inventory-period branch January 8, 2025 10:33

mergify bot pushed a commit that referenced this pull request Jan 8, 2025

[Asset Inventory] Run fetchers periodically (#2902)

464a556

(cherry picked from commit bb7a3e9)

mergify bot mentioned this pull request Jan 8, 2025

[8.x](backport #2902) [Asset Inventory] Run fetchers periodically #2905

Merged

2 tasks

kubasobon added backport-v8.16.0 backport-v8.17.0 labels Jan 8, 2025

mergify bot pushed a commit that referenced this pull request Jan 8, 2025

[Asset Inventory] Run fetchers periodically (#2902)

4eb797f

(cherry picked from commit bb7a3e9)

mergify bot pushed a commit that referenced this pull request Jan 8, 2025

[Asset Inventory] Run fetchers periodically (#2902)

500cb2a

(cherry picked from commit bb7a3e9)

This was referenced Jan 8, 2025

[8.16](backport #2902) [Asset Inventory] Run fetchers periodically #2906

Merged

[8.17](backport #2902) [Asset Inventory] Run fetchers periodically #2907

Merged

kubasobon added a commit that referenced this pull request Jan 8, 2025

[8.x](backport #2902) [Asset Inventory] Run fetchers periodically (#2905

8b683cb

) [Asset Inventory] Run fetchers periodically (#2902) (cherry picked from commit bb7a3e9) Co-authored-by: Kuba Soboń <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Asset Inventory] Run fetchers periodically #2902

[Asset Inventory] Run fetchers periodically #2902

kubasobon commented Jan 7, 2025

mergify bot commented Jan 7, 2025

romulets left a comment

romulets Jan 7, 2025

kubasobon Jan 7, 2025 •

edited

Loading

kubasobon Jan 7, 2025

romulets Jan 8, 2025

moukoublen Jan 8, 2025

kubasobon Jan 8, 2025

kubasobon commented Jan 7, 2025 •

edited

Loading

romulets left a comment

romulets Jan 8, 2025

kubasobon commented Jan 8, 2025

orouz Jan 8, 2025 •

edited

Loading

kubasobon Jan 8, 2025

kubasobon Jan 8, 2025 •

edited

Loading

orouz Jan 8, 2025

	timer := time.NewTimer(0)
	defer timer.Stop()

	for {
	select {
	case <-ctx.Done():
	m.log.Info("Fetchers manager canceled")
	return
	case <-timer.C:
	// update the interval
	timer.Reset(m.interval)
	// this is blocking so the stop will not be called until all the fetchers are finished
	// in case there is a blocking fetcher it will halt (til the m.timeout)
	go m.fetchIteration(ctx)
	}
	}

[Asset Inventory] Run fetchers periodically #2902

[Asset Inventory] Run fetchers periodically #2902

Conversation

kubasobon commented Jan 7, 2025

Summary of your changes

Related Issues

Checklist

mergify bot commented Jan 7, 2025

romulets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubasobon Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubasobon commented Jan 7, 2025 • edited Loading

romulets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubasobon commented Jan 8, 2025

orouz Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubasobon Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubasobon Jan 7, 2025 •

edited

Loading

kubasobon commented Jan 7, 2025 •

edited

Loading

orouz Jan 8, 2025 •

edited

Loading

kubasobon Jan 8, 2025 •

edited

Loading