Skip to content

tests: retry k8s ops related to env vars#15486

Merged
porridge merged 3 commits intomasterfrom
porridge/retry-depl-env
Jul 28, 2025
Merged

tests: retry k8s ops related to env vars#15486
porridge merged 3 commits intomasterfrom
porridge/retry-depl-env

Conversation

@porridge
Copy link
Copy Markdown
Contributor

@porridge porridge commented May 29, 2025 •
edited
Loading

Description

First stab at adding retries lost in #12292

If we deem this approach sensible, we can work on adding retries around more k8s client calls.

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • modified existing tests

How I validated my change

CI

@porridge porridge requested a review from stehessel May 29, 2025 09:03
sourcery-ai[bot]
sourcery-ai bot reviewed May 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @porridge - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented May 29, 2025 •
edited
Loading

Images are ready for the commit at 05977d7.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.9.x-284-g05977d705a.

janisz
janisz reviewed May 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@janisz janisz left a comment

Choose a reason for hiding this comment

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

How about using https://github.com/hashicorp/go-retryablehttp in k8s client? Then retry will be transparent.
We need to set restCfg.Transport with retyrable http transport in getConfig like we did in:

stackrox/tests/common.go

Lines 315 to 323 in 4f9e180

func getConfig(t *testing.T) *rest.Config {
config, err := clientcmd.NewDefaultClientConfigLoadingRules().Load()
require.NoError(t, err, "could not load default Kubernetes client config")
restCfg, err := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{}).ClientConfig()
require.NoError(t, err, "could not get REST client config from kubernetes config")
return restCfg
}

@codecov
Copy link
Copy Markdown

codecov bot commented May 29, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.69%. Comparing base (0a61145) to head (05977d7).
⚠️ Report is 36 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #15486      +/-   ##
==========================================
+ Coverage   48.68%   48.69%   +0.01%     
==========================================
  Files        2604     2605       +1     
  Lines      191663   191675      +12     
==========================================
+ Hits        93304    93330      +26     
+ Misses      91026    91012      -14     
  Partials     7333     7333
Flag Coverage Δ
go-unit-tests 48.69% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented May 29, 2025

How about using https://github.com/hashicorp/go-retryablehttp in k8s client?

Such generic approach seems tempting, but from its docs it seems it will retry even POST (which is non-idempotent) requests when they fail with a transport-level error. This seems unsafe to do blindly for all requests using this client. 🤔

Not saying using go-retryablehttp is wrong, but perhaps needs more thought than I have time for right now. :-(

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented May 29, 2025

Oh, and I broke the logic, turns out ignoring the error was intentional in some cases 🤦🏻

        	Test:       	TestDelegatedScanning
        	Messages:   	cannot find envVar "LOGLEVEL" in container "sensor" in deployment "sensor" in namespace "stackrox"

@porridge porridge removed the request for review from stehessel May 29, 2025 10:27
@porridge porridge marked this pull request as draft May 29, 2025 10:27
@porridge porridge force-pushed the porridge/retry-depl-env branch from 2c71f55 to b64b3a5 Compare July 22, 2025 08:09
@red-hat-konflux
Copy link
Copy Markdown
Contributor

red-hat-konflux bot commented Jul 22, 2025

Caution

There are some errors in your PipelineRun template.

PipelineRun Error
quay-proxy no kind "ImageDigestMirrorSet" is registered for version "config.openshift.io/v1" in scheme "k8s.io/client-go/kubernetes/scheme/register.go:83"

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Jul 22, 2025

/test all

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Jul 22, 2025

/test all

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Jul 22, 2025

/test gke-nongroovy-e2e-tests
/test ocp-4-12-nongroovy-e2e-tests
/test ocp-4-19-nongroovy-e2e-tests
/test ocp-4-18-nongroovy-e2e-tests

@porridge porridge marked this pull request as ready for review July 23, 2025 05:02
@porridge porridge requested a review from janisz July 23, 2025 05:03
janisz
janisz approved these changes Jul 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@janisz janisz left a comment

Choose a reason for hiding this comment

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

LGTM comments could be solved as follow up

@porridge porridge merged commit 7522c8c into master Jul 28, 2025
90 checks passed
@porridge porridge deleted the porridge/retry-depl-env branch July 28, 2025 07:17
Stringy pushed a commit that referenced this pull request Aug 15, 2025
@porridge @Stringy
tests: retry k8s ops related to env vars (#15486)
bcadb67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@janisz janisz janisz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@porridge @rhacs-bot @janisz