-
Notifications
You must be signed in to change notification settings - Fork 115
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
use kubeclient to get and delete machineconfig #848
base: master
Are you sure you want to change the base?
use kubeclient to get and delete machineconfig #848
Conversation
Signed-off-by: liucongran <[email protected]>
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Signed-off-by: liucongran <[email protected]>
489995f
to
e7a88a3
Compare
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.
@crliu3227 thanks for dealing with this issue.
It would be great if we could disable the cache for all MachineConfig when we create the Manager in main.go#L134
According to this article, it should be possible:
https://medium.com/@timebertt/kubernetes-controllers-at-scale-clients-caches-conflicts-patches-explained-aa0f7a8b4332#8181
WDYT?
@@ -59,6 +59,7 @@ type SriovOperatorConfigReconciler struct { | |||
Scheme *runtime.Scheme | |||
PlatformHelper platforms.Interface | |||
FeatureGate featuregate.FeatureGate | |||
KubeClient client.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.
Can we be more specific and use mgr.GetAPIReader()
. It would be clearer if anyone else needs to do operations avoiding the cache,
KubeClient client.Client | |
uncachedAPIReader client.Reader |
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.
OK, I will modify this part and resubmit the PR again
Signed-off-by: liucongran <[email protected]>
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.
LGTM
@@ -413,7 +414,8 @@ func (r *SriovOperatorConfigReconciler) syncOpenShiftSystemdService(ctx context. | |||
|
|||
if cr.Spec.ConfigurationMode != sriovnetworkv1.SystemdConfigurationMode { | |||
obj := &machinev1.MachineConfig{} | |||
err := r.Get(context.TODO(), types.NamespacedName{Name: consts.SystemdServiceOcpMachineConfigName}, obj) | |||
// use uncached api reader to get machineconfig to reduce memory footprint | |||
err := r.UncachedAPIReader.Get(context.TODO(), types.NamespacedName{Name: consts.SystemdServiceOcpMachineConfigName}, obj) |
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.
Why not to use ctx
that is passed to syncOpenShiftSystemdService
as arg?
Use kubeclient to get and delete machineconfig, to fix #846