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

[fix] : use single copy of instances cache #276

Merged
merged 3 commits into from
Dec 18, 2024
Merged

Conversation

rahulait
Copy link
Collaborator

@rahulait rahulait commented Dec 17, 2024

This PR allows CCM to use single copy of instance cache for all controllers. Previously, all controllers were maintaining their own copy of instance cache and were refreshing it when using it.

To test, I intentionally started CCM with flags - --node-status-update-frequency=20s and - --linodego-debug=true which makes CCM to update node status every 20s rather than default 5min. With existing main branch and mentioned flags, I can see it making double calls to linode API for updating cache (one for instance cache kept by route_controller and one by instances controller). With an image from this PR, I can see it makes half of what I see with main branch. It confirms that both controllers are now sharing the same cache.

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

@github-actions github-actions bot added the bugfix for any bug fixes in the changelog. label Dec 17, 2024
@@ -123,7 +125,7 @@ func newCloud() (cloudprovider.Interface, error) {
// create struct that satisfies cloudprovider.Interface
lcloud := &linodeCloud{
Copy link
Contributor

Choose a reason for hiding this comment

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

The Initialize method here calls newInstances yet again for the Node Controller - do we want that one to share this instances struct, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, good catch. Updated the PR to address this as well.

@rahulait rahulait force-pushed the fix-instance-caching branch 2 times, most recently from 9dce12f to b148f90 Compare December 17, 2024 16:24
@rahulait rahulait force-pushed the fix-instance-caching branch from b148f90 to c5368d9 Compare December 17, 2024 16:25
@rahulait rahulait changed the title [fix] : use single copy of instances [fix] : use single copy of instances cache Dec 17, 2024
@rahulait rahulait merged commit 162a835 into main Dec 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix for any bug fixes in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants