-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🐛fix ClusterCache doesn't pick latest kubeconfig secret proactively #12400
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
base: main
Are you sure you want to change the base?
Changes from all commits
49bbe5d
f12860e
03b6dd6
283f94a
2a67216
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -450,6 +450,7 @@ func (cc *clusterCache) Reconcile(ctx context.Context, req reconcile.Request) (r | |
|
||
// Try to connect, if not connected. | ||
connected := accessor.Connected(ctx) | ||
|
||
if !connected { | ||
lastConnectionCreationErrorTimestamp := accessor.GetLastConnectionCreationErrorTimestamp(ctx) | ||
|
||
|
@@ -511,6 +512,16 @@ func (cc *clusterCache) Reconcile(ctx context.Context, req reconcile.Request) (r | |
requeueAfterDurations = append(requeueAfterDurations, accessor.config.HealthProbe.Interval) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are hitting tooManyConsecutiveFailures in your scenario, right? Would it also be enough for your use case to make HealthProbe.Timeout/Interval/FailureThreshold configurable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if the health check would open a new connection it would detect it, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's correct~ another idea is to disconnect for a given time (e.g. 5m) period to force refresh connection. will this be better? |
||
} | ||
} | ||
|
||
// Check if kubeconfig was updated | ||
kubeconfigUpdated, err := accessor.KubeConfigUpdated(ctx) | ||
if err != nil { | ||
log.Error(err, "error checking if kubeconfig was updated") | ||
} else if kubeconfigUpdated { | ||
log.Info("Kubeconfig was updated, disconnecting to re-connect with the new kubeconfig") | ||
accessor.Disconnect(ctx) | ||
didDisconnect = true | ||
} | ||
} | ||
|
||
// Send events to cluster sources. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to a huge number of get secret calls to the apiserver if someone uses an uncached client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that would be a problem, do you have any suggestions? shall we make this configurable?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i understand that if user set
syncPeriod
to very short time, e.g. 10sec, then it may cause perf issue, right?Maybe we can update doc to infrom user the impact of setting too little value.
I can also add another
checkPeriod
to reduce the check frequency, but that seems to make configration more complex. Or we can just hardcode the checkPeriod value like5m