Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check IstioCNI status for IstioRevisions that depend on IstioCNI #711

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

luksa
Copy link
Contributor

@luksa luksa commented Mar 13, 2025

Previously, when an IstioRevision depended on IstioCNI, either because the user enabled the use of IstioCNI through values.pilot.cni.enabled or because the platform profile enables it, the IstioRevision resource would show as healthy even though workloads would fail because IstioCNI hasn't been deployed. This commit adds a new DependenciesHealthy condition to IstioRevision and makes IstioRevision.status.state and the STATUS column report the fact that IstioCNI is missing or unhealthy. This makes it easier for users to see that their setup is misconfigured.

@luksa luksa requested a review from a team as a code owner March 13, 2025 14:21
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.34%. Comparing base (3650ea5) to head (e5616c2).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
...trollers/istiorevision/istiorevision_controller.go 82.69% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
+ Coverage   74.29%   74.34%   +0.05%     
==========================================
  Files          43       44       +1     
  Lines        2575     2631      +56     
==========================================
+ Hits         1913     1956      +43     
- Misses        569      580      +11     
- Partials       93       95       +2     

☔ 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.

@luksa luksa force-pushed the istio-cni-status branch 13 times, most recently from 5caeea7 to 22d710f Compare March 14, 2025 11:04
Previously, when an IstioRevision depended on IstioCNI, either because the user enabled the use of IstioCNI through values.pilot.cni.enabled or because the platform profile enables it, the IstioRevision resource would show as healthy even though workloads would fail because IstioCNI hasn't been deployed. This commit adds a new `DependenciesHealthy` condition to `IstioRevision` and makes `IstioRevision.status.state` and the `STATUS` column report the fact that IstioCNI is missing or unhealthy. This makes it easier for users to see that their setup is misconfigured.

Signed-off-by: Marko Lukša <[email protected]>
@luksa luksa force-pushed the istio-cni-status branch from 22d710f to 731d0a9 Compare March 14, 2025 19:01
@dgn
Copy link
Collaborator

dgn commented Mar 21, 2025

should we cover this in the IstioCNI integration test? Just a quick check of the happy/sad paths maybe - we could easily simulate an unhealthy IstioCNI there by manipulating status

Signed-off-by: Marko Lukša <[email protected]>
@luksa
Copy link
Contributor Author

luksa commented Mar 27, 2025

@dgn integration test added.

Copy link
Collaborator

@dgn dgn left a comment

Choose a reason for hiding this comment

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

Thanks Marko!

@istio-testing istio-testing merged commit 6ee0278 into istio-ecosystem:main Mar 27, 2025
16 checks passed
openshift-service-mesh-bot pushed a commit to openshift-service-mesh-bot/sail-operator that referenced this pull request Mar 27, 2025
* upstream/main:
  Check IstioCNI status for IstioRevisions that depend on IstioCNI (istio-ecosystem#711)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants