Skip to content

Conversation

Aaron-9900
Copy link
Contributor

@Aaron-9900 Aaron-9900 commented Nov 25, 2024

Implementing the Gitops side of this issue: argoproj/argo-cd#20896

Adds an api to kube.go to get the images from a resource. Adds those images to ResourceSyncResult. This API accounts for how different k8s resources define the images.

@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch from 922aecd to 3f1734a Compare November 25, 2024 16:02
@Aaron-9900 Aaron-9900 changed the title Add GetResourceImages feat: return images from resources when sync occurs Nov 25, 2024
@Aaron-9900 Aaron-9900 marked this pull request as ready for review November 25, 2024 16:11
Copy link
Contributor

@andrii-korotkov-verkada andrii-korotkov-verkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test to when there are no images, please?

@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch from 5281e57 to ae4000b Compare November 27, 2024 10:13
for _, container := range containers {
containerMap, ok := container.(map[string]interface{})
if !ok {
return nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we continue instead of returning here? It might be something related to the single container only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to extract as much data as possible. Added continue on 5b2c46f but maybe @andrii-korotkov-verkada has a different take

@@ -404,6 +404,34 @@ func GetDeploymentReplicas(u *unstructured.Unstructured) *int64 {
return &val
}

func GetResourceImages(u *unstructured.Unstructured) []string {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For CronJobs it seems that the image is under spec.jobTemplate.spec.template.spec.containers. Should we also check there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6b9063a

Copy link

Copy link
Member

@rumstead rumstead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aaron-9900 Aaron-9900 requested a review from a team as a code owner January 7, 2025 12:05
Copy link

sonarqubecloud bot commented Jan 7, 2025

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 53.55%. Comparing base (8849c3f) to head (4fbe25e).
Report is 38 commits behind head on master.

Files with missing lines Patch % Lines
pkg/utils/kube/kube.go 93.54% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   54.26%   53.55%   -0.72%     
==========================================
  Files          64       64              
  Lines        6164     6442     +278     
==========================================
+ Hits         3345     3450     +105     
- Misses       2549     2715     +166     
- Partials      270      277       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch 3 times, most recently from 50c907f to a989b44 Compare February 12, 2025 14:53
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
…for cronjobs

Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch from a989b44 to e6f8fbe Compare February 12, 2025 14:59
@rumstead
Copy link
Member

rumstead commented Feb 18, 2025

Can you create a PR to Argo CD updating the go.mod to your branch? That will allow us to validate the e2e tests run and pass.

EDIT: Nvm, found it argoproj/argo-cd#20954. Do the e2e pass?

@Aaron-9900
Copy link
Contributor Author

@rumstead done in argoproj/argo-cd#20954, tests are now passing

Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch from f14771b to 914b89c Compare February 26, 2025 16:24
@Aaron-9900
Copy link
Contributor Author

Added new benchmark test for the sync operation. These are the results:

command: go test -bench=. -benchtime=2s -count 10

3 images

—— new

goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   63940	     37651 ns/op
BenchmarkSync-10    	   60710	     40457 ns/op
BenchmarkSync-10    	   56287	     40774 ns/op
BenchmarkSync-10    	   60465	     40192 ns/op
BenchmarkSync-10    	   60189	     40748 ns/op
BenchmarkSync-10    	   59324	     38114 ns/op
BenchmarkSync-10    	   61059	     37721 ns/op
BenchmarkSync-10    	   64406	     38233 ns/op
BenchmarkSync-10    	   58512	     37904 ns/op
BenchmarkSync-10    	   60928	     38100 ns/op

—— old
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   60399	     39880 ns/op
BenchmarkSync-10    	   61432	     35195 ns/op
BenchmarkSync-10    	   72246	     34367 ns/op
BenchmarkSync-10    	   70420	     34345 ns/op
BenchmarkSync-10    	   72147	     33829 ns/op
BenchmarkSync-10    	   70708	     37917 ns/op
BenchmarkSync-10    	   59630	     34623 ns/op
BenchmarkSync-10    	   71232	     33385 ns/op
BenchmarkSync-10    	   72580	     33692 ns/op
BenchmarkSync-10    	   69116	     34070 ns/op

—— comparison
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
        │   old.txt    │               new.txt               │
        │    sec/op    │   sec/op     vs base                │
Sync-10   34.36µ ± 10%   38.17µ ± 7%  +11.11% (p=0.001 n=10)	
5 images
—— new
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   59912	     42840 ns/op
BenchmarkSync-10    	   50362	     44076 ns/op
BenchmarkSync-10    	   57453	     44372 ns/op
BenchmarkSync-10    	   57046	     41663 ns/op
BenchmarkSync-10    	   56764	     44259 ns/op
BenchmarkSync-10    	   58297	     42187 ns/op
BenchmarkSync-10    	   55392	     43204 ns/op
BenchmarkSync-10    	   58656	     40735 ns/op
BenchmarkSync-10    	   62046	     42186 ns/op
BenchmarkSync-10    	   56892	     45048 ns/op
—— old
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   72872	     33205 ns/op
BenchmarkSync-10    	   73526	     33451 ns/op
BenchmarkSync-10    	   68599	     33591 ns/op
BenchmarkSync-10    	   70236	     33961 ns/op
BenchmarkSync-10    	   60986	     35286 ns/op
BenchmarkSync-10    	   71302	     34127 ns/op
BenchmarkSync-10    	   70725	     34121 ns/op
BenchmarkSync-10    	   72074	     34371 ns/op
BenchmarkSync-10    	   72940	     37116 ns/op
BenchmarkSync-10    	   68548	     35546 ns/op
— Comparison
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
        │   old.txt   │               new.txt               │
        │   sec/op    │   sec/op     vs base                │
Sync-10   34.12µ ± 4%   43.02µ ± 3%  +26.08% (p=0.000 n=10)
10 images
— new
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   52508	     45693 ns/op
BenchmarkSync-10    	   55818	     43287 ns/op
BenchmarkSync-10    	   55305	     46177 ns/op
BenchmarkSync-10    	   54345	     44417 ns/op
BenchmarkSync-10    	   54358	     46124 ns/op
BenchmarkSync-10    	   51956	     44968 ns/op
BenchmarkSync-10    	   49981	     48196 ns/op
BenchmarkSync-10    	   48973	     49551 ns/op
BenchmarkSync-10    	   49080	     49411 ns/op
BenchmarkSync-10    	   52101	     46850 ns/op

—— old
goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
BenchmarkSync-10    	   70117	     36253 ns/op
BenchmarkSync-10    	   66276	     35481 ns/op
BenchmarkSync-10    	   73176	     33779 ns/op
BenchmarkSync-10    	   71619	     34551 ns/op
BenchmarkSync-10    	   74025	     33803 ns/op
BenchmarkSync-10    	   73438	     33614 ns/op
BenchmarkSync-10    	   72687	     33609 ns/op
BenchmarkSync-10    	   70243	     33591 ns/op
BenchmarkSync-10    	   70620	     34983 ns/op
BenchmarkSync-10    	   70617	     33780 ns/op
PASS
ok  	github.com/argoproj/gitops-engine/pkg/sync	119.814s

—— comparison

goos: darwin
goarch: arm64
pkg: github.com/argoproj/gitops-engine/pkg/sync
cpu: Apple M1 Max
        │   old.txt   │               new.txt               │
        │   sec/op    │   sec/op     vs base                │
Sync-10   33.79µ ± 5%   46.15µ ± 7%  +36.57% (p=0.000 n=10)

Overall, we see a small increase in time/operation for a small amount of images (11% for 3 images), and it gets larger as the images grow (36% for 10 images). This is expected, since we are now iterating over images. Even with this growth, the total value is still quite low.

Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com>
@Aaron-9900 Aaron-9900 force-pushed the feat/add-images-to-resources branch from b985995 to 4fbe25e Compare February 27, 2025 10:00
Copy link

@crenshaw-dev crenshaw-dev merged commit c617562 into argoproj:master Mar 27, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants