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 and reset service instance count in sfcluster #1914

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

jintusebastian
Copy link
Contributor

  • fixed the issue in instance counter controler flow
  • fixed the instance count in sfcluster in case of mismatch

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #1914 (4b08117) into master (388930b) will decrease coverage by 0.70%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1914      +/-   ##
==========================================
- Coverage   73.07%   72.37%   -0.70%     
==========================================
  Files          48       48              
  Lines        4639     4641       +2     
==========================================
- Hits         3390     3359      -31     
- Misses        900      925      +25     
- Partials      349      357       +8     

Copy link
Contributor

@anoopjb anoopjb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

// Calculate the expected service instance count for each cluster
instanceCount := make(map[string]int)
for more := true; more; more = (sfserviceinstances.Continue != "") {
err := client.List(ctx, sfserviceinstances, instanceOptions, kubernetes.Limit(constants.ListPaginationLimit),
Copy link
Contributor

Choose a reason for hiding this comment

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

It is nice that we are only reading 100 (constants.ListPaginationLimit) sfserviceinstances at a time 👍 . This should prevent the possibility of an out of memory error in heavy landscapes. I think it will be a good idea to monitor memory usage after releasing this feature.
Here is a guide from Golang garbage collector, https://tip.golang.org/doc/gc-guide#Optimization_guide

@jintusebastian jintusebastian force-pushed the fix/sfcluster-serviceinstance-count-mismatch branch from 9c5aea8 to 4b08117 Compare September 13, 2023 05:29
@jintusebastian jintusebastian merged commit 8b6569f into master Sep 13, 2023
@jintusebastian jintusebastian deleted the fix/sfcluster-serviceinstance-count-mismatch branch September 13, 2023 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants