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.

Sign up for GitHub

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

Jump to bottom

Remove option to disable kube caching #5599

Closed
nrfox opened this issue Nov 1, 2022 · 3 comments · Fixed by #6364
Closed

Remove option to disable kube caching #5599

nrfox opened this issue Nov 1, 2022 · 3 comments · Fixed by #6364
Assignees
@nrfox
Labels
backlog Triaged Issue added to backlog enhancement This is the preferred way to describe new end-to-end features. test-add-coverage 📎 this needs test coverage

Comments

@nrfox
Copy link
Contributor

nrfox commented Nov 1, 2022

Caching has been enabled by default for some time now. This issue is to remove the option to disable kube caching. Kiali may still query the kube api directly in some cases but will largely rely on the local cache. Some supported kube types are not currently cached like CronJob but these should be added as well.

This is a followup to #5319 and #4678.
@nrfox nrfox added the enhancement This is the preferred way to describe new end-to-end features. label Nov 1, 2022
@nrfox nrfox self-assigned this Nov 1, 2022
@jshaughn jshaughn added the backlog Triaged Issue added to backlog label Nov 18, 2022
@nrfox nrfox mentioned this issue Dec 8, 2022
Merged
@nrfox nrfox mentioned this issue Jan 3, 2023
Closed
@israel-hdez israel-hdez mentioned this issue Jan 13, 2023
Closed
@nrfox
Copy link
Contributor Author

nrfox commented Jan 27, 2023

Rather than blocking making the cache mandatory on refactoring all the backend unit tests, @israel-hdez had the idea of removing the external config option while keeping the internal cfg option that the unit tests can still set. I think this is a better way forward and we should do this while the test refactoring is on-going.

@israel-hdez I'm assigning you to this task but feel free to unassign yourself or assign someone else.

@nrfox nrfox assigned israel-hdez and unassigned nrfox Jan 27, 2023
israel-hdez added a commit to israel-hdez/swscore that referenced this issue Jan 31, 2023
@israel-hdez
No longer read kubernetes_config.cache_enabled config
c359b0e 
This small change will prevent reading (unmarshalling) the kubernetes_config.cache_enabled configuration from a YAML file (i.e. from a Kubernetes ConfigMap).

This effectively makes it impossible to disable Kiali's kube cache via the usual configuration mechanism. This will allow to write new code under the assumption that the kube cache is always enabled (i.e. no more if/else conditionals around this configuration variable).

The kube cache can still be disabled programmatically. This will allow to gradually update existing code to stop relying on the removed configuration (most notably, unit tests need to be adapted).

Related kiali#5599
This was referenced Jan 31, 2023
Merged
Merged
@nrfox nrfox mentioned this issue Jan 31, 2023
Merged
israel-hdez added a commit to israel-hdez/swscore that referenced this issue Feb 14, 2023
@israel-hdez
Adapt apps_test.go to not rely on turning off the cache
c02239f 
The adaptation moves from using mocks to using a fake client. This allows for starting a cache where the underlying client is a fake, making it possible to allow the code to use "caching" as with the real client.

Related kiali#5599
@israel-hdez israel-hdez mentioned this issue Feb 14, 2023
Merged
@israel-hdez
Copy link
Member

Transferring this issue to @leandroberetta as the new Ωwner.

@matejnesuta matejnesuta added the test-add-coverage 📎 this needs test coverage label Mar 1, 2023
@nrfox nrfox self-assigned this Mar 13, 2023
@leandroberetta leandroberetta removed their assignment Mar 13, 2023
This was referenced Mar 15, 2023
Open
Merged
@nrfox nrfox mentioned this issue Apr 10, 2023
Merged
@nrfox
Copy link
Contributor Author

nrfox commented May 25, 2023

The gist of this work is just removing kialiCache != nil references and updating any unit tests that break. After removing the references locally and running the test suite, there's 21 tests that need to be updated. It's possible though that there would be additional failures after fixing some tests since many test failures cause panics and these can mask additional failures once the failing test is fixed.

@nrfox nrfox mentioned this issue Jun 20, 2023
Merged
@nrfox nrfox mentioned this issue Jul 17, 2023
Merged
jmazzitelli added a commit to jmazzitelli/kiali-operator that referenced this issue Jul 18, 2023
@jmazzitelli
remove cache related settings that are no longer used
8a42384 
part of kiali/kiali#5599
@jmazzitelli jmazzitelli mentioned this issue Jul 18, 2023
Merged
nrfox pushed a commit to kiali/kiali-operator that referenced this issue Jul 18, 2023
@jmazzitelli
remove cache related settings that are no longer used (#670)
d4f253b 
part of kiali/kiali#5599
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nrfox nrfox

Labels
backlog Triaged Issue added to backlog enhancement This is the preferred way to describe new end-to-end features. test-add-coverage 📎 this needs test coverage
Projects
Kiali Sprint 24-5 | Kiali 1.84
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make cache mandatory and remove some cache config opts nrfox/kiali
5 participants
@leandroberetta @jshaughn @nrfox @israel-hdez @matejnesuta