fix(STONEINTG-1087): retry longer to fetch App from ITS, and stop processing if not found #993

dheerajodha · 2025-01-20T08:54:52Z

There are some cases where the App and ITS are created almost at the same time.
This causes a problem when the App is not found while the controllers reconcile ITS.
To fix this, we increased the retry time from 10 ms to 10 seconds. This should give enough time to fetch the App.
And if we're still unable to find App, we stop reconciling, since there's no point in reconciling an ITS without any parent.

Maintainers will complete the following section

Commit messages are descriptive enough (hints)
Code coverage from testing does not decrease and new code is covered (check the PR coverage on codecov)
Controllers diagrams are updated when PR changes controllers code (if applicable)

openshift-ci · 2025-01-20T08:54:58Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov-commenter · 2025-01-20T09:01:29Z

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@403c750). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...nternal/controller/scenario/scenario_controller.go	78.94%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #993   +/-   ##
=======================================
  Coverage        ?   64.14%           
=======================================
  Files           ?       49           
  Lines           ?     6281           
  Branches        ?        0           
=======================================
  Hits            ?     4029           
  Misses          ?     1893           
  Partials        ?      359

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

dheerajodha · 2025-01-20T09:13:43Z

internal/controller/scenario/scenario_controller.go

 		logger.Error(err, "Failed to get Application from the IntegrationTestScenario after retry", "application", scenario.Spec.Application)
+		return ctrl.Result{}, nil


Once we have retried long enough for the Application to exist in the cluster, and still couldn't find it, the operator will stop processing this ITS. But in that case, this ITS would be an orphan and dangle in the cluster. Do you think we should delete this ITS here?

Hm, not sure, maybe we can at least mark it as invalid, wdyt?

Good idea, I'm thinking that ITS would still dangle, but then it would be easier to perform a manual cleanup eventually.

* There are some case where the App and ITS are created almost at the same time. This causes problem when the App is not found while the controllers reconcile ITS. * To fix this, we increased the retry time from 10 ms to 10 seconds. * This should give enough time to fetch the App. * And if we're still unable to find App, we stop reconciling, since there's no point in reconciling an ITS without any parent. Signed-off-by: Dheeraj<djodha@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Jan 20, 2025

github-actions bot added the size: XS label Jan 20, 2025

dheerajodha commented Jan 20, 2025

View reviewed changes

dheerajodha force-pushed the STONEINTG-1087 branch from 17d39a8 to cac7550 Compare January 21, 2025 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(STONEINTG-1087): retry longer to fetch App from ITS, and stop processing if not found #993

fix(STONEINTG-1087): retry longer to fetch App from ITS, and stop processing if not found #993

dheerajodha commented Jan 20, 2025

openshift-ci bot commented Jan 20, 2025

codecov-commenter commented Jan 20, 2025 •

edited

Loading

dheerajodha Jan 20, 2025

dirgim Jan 21, 2025 •

edited

Loading

dheerajodha Jan 21, 2025

		logger.Error(err, "Failed to get Application from the IntegrationTestScenario after retry", "application", scenario.Spec.Application)
		return ctrl.Result{}, nil

fix(STONEINTG-1087): retry longer to fetch App from ITS, and stop processing if not found #993

Are you sure you want to change the base?

fix(STONEINTG-1087): retry longer to fetch App from ITS, and stop processing if not found #993

Conversation

dheerajodha commented Jan 20, 2025

Maintainers will complete the following section

openshift-ci bot commented Jan 20, 2025

codecov-commenter commented Jan 20, 2025 • edited Loading

Codecov Report

dheerajodha Jan 20, 2025

Choose a reason for hiding this comment

dirgim Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

dheerajodha Jan 21, 2025

Choose a reason for hiding this comment

codecov-commenter commented Jan 20, 2025 •

edited

Loading

dirgim Jan 21, 2025 •

edited

Loading