csi: add hooks in both Node and Controller Services#968
csi: add hooks in both Node and Controller Services#968openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Conversation
|
/hold This is a WIP design. This code is being tested on openshift/aws-ebs-csi-driver-operator#104. |
openshift-ci-robot
added
the
do-not-merge/hold
|
Should there be a similar hook for DaemonSet? |
bertinatto
force-pushed
the
add-hook
branch
3 times, most recently
from
ef266f6 to
e7e6303
Compare
bertinatto
changed the title
Added, but no operator is using it ATM |
|
/hold cancel I've tested this with openshift/aws-ebs-csi-driver-operator#104 and it works. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@deads2k could you take a look, please? |
bertinatto
force-pushed
the
add-hook
branch
3 times, most recently
from
6ea769d to
ecd6418
Compare
| deployInformer appsinformersv1.DeploymentInformer, | ||
| optionalConfigInformer configinformers.SharedInformerFactory, | ||
| recorder events.Recorder, | ||
| optionalDeploymentHooks ...func(*appsv1.Deployment) error, |
There was a problem hiding this comment.
Would it make sense to either give owner of the controller a way how to poke the controller for resync (i.e. the owner has informer on secrets / config maps / whatnot and is able to enqueue "fake" event into CSIDriverControllerServiceController' queue) or pass informers to watch into the NewCSIDriverControllerServiceController?
I can see there is ResyncEvery(time.Minute) below, so this PR will work even without it.
There was a problem hiding this comment.
Would it make sense to address this in another PR?
Also, could you clarify a use case for poking the controller for resync?
|
/assign @deads2k |
|
/lgtm |
| }, | ||
| }, | ||
| { | ||
| // Deployment is fully deployed and its status is synced to CR |
There was a problem hiding this comment.
this rubs me the wrong way. Why did it need to be removed?
There was a problem hiding this comment.
It was a duplicate of the test case right above, probably copied by mistake.
| kubeClient kubernetes.Interface | ||
| dsInformer appsinformersv1.DaemonSetInformer | ||
| // Optional hook functions to modify the DaemonSet. | ||
| optionalDaemonSetHooks []func(*appsv1.DaemonSet) error |
There was a problem hiding this comment.
typed functions please. Also, make clear what happens on errors and where responsibility for identifying which function is failing lies in the error.
| for _, fn := range c.optionalDaemonSetHooks { | ||
| err := fn(required) | ||
| if err != nil { | ||
| return fmt.Errorf("error running hook function: %w", err) |
There was a problem hiding this comment.
you can at least give me an ordinal (i)
|
I'm curious what you're trying to replace/mutate. Some sorts of mutations are easier while you still have bytes (this is why we have the |
There was a problem hiding this comment.
I'm curious what you're trying to replace/mutate. Some sorts of mutations are easier while you still have bytes (this is why we have the replacePlaceholders).
As more CSI operators use these control loops, we want to give them freedom to mutate the deployment/daemonset without adding placeholders to library-go, specially if the mutation is specific to that operator, like this one: openshift/aws-ebs-csi-driver-operator#102
Most operators won't need this, and the default mutation approach should be the string replacements, but I figured for some situations mutating the object is better.
Does that make sense?
| }, | ||
| }, | ||
| { | ||
| // Deployment is fully deployed and its status is synced to CR |
There was a problem hiding this comment.
It was a duplicate of the test case right above, probably copied by mistake.
| for _, fn := range c.optionalDaemonSetHooks { | ||
| err := fn(required) | ||
| if err != nil { | ||
| return fmt.Errorf("error running hook function: %w", err) |
| kubeClient kubernetes.Interface | ||
| dsInformer appsinformersv1.DaemonSetInformer | ||
| // Optional hook functions to modify the DaemonSet. | ||
| optionalDaemonSetHooks []func(*appsv1.DaemonSet) error |
I understand the desire. I'd be somewhat interested in what you're combining because I've regretted it in the past. But I'm ok adding the option |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, deads2k, jsafrane The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
5 participants
CC @openshift/storage