Extended support for customizing whereabouts ip-reconciler cron schedule #392
Conversation
Signed-off-by: nicklesimba <[email protected]>
|
Hm I wonder if this isn't good enough for what you need: #317 IIUC, the user would edit the existing ds - configuring the cron via env variable, and kill the ds pods. |
This PR provides support for Openshift users to edit cron schedule via configmap. I think this is a separate feature. Edit: I think you might be correct after all...thank you for the suggestion Miguel! |
|
@nicklesimba are you agreeing that #317 is sufficient? If so, this PR could be closed I think. |
|
After some investigation, I think this support might still be necessary. What Miguel linked allows users to change the cron schedule at install time, but these changes allow a user to update during runtime. |
You can still react to updates by updating the config map and kill the daemonset pods; the new pods will load the "new" configuration. I mostly wonder if that's good enough. Happy to hear feedback. |
|
My understanding is that the whereabouts install-cni.sh script has the following definition: This makes it so that when installation happens, whatever environment variable is added by the configmap (even if it's correctly updated) will get overwritten by the above line. At least, this is the behavior I observed, and the explanation I gave is my hypothesis, but I might be misunderstanding installation order. |
| @@ -19,7 +19,7 @@ WHEREABOUTS_RECONCILER_CRON=${WHEREABOUTS_RECONCILER_CRON:-30 4 * * *} | |||
|
|
|||
| mkdir -p $CNI_CONF_DIR/whereabouts.d | |||
| WHEREABOUTS_KUBECONFIG=$CNI_CONF_DIR/whereabouts.d/whereabouts.kubeconfig | |||
| WHEREABOUTS_FLATFILE=$CNI_CONF_DIR/whereabouts.d/whereabouts.conf | |||
| WHEREABOUTS_FLATFILE=$CNI_CONF_DIR/whereabouts.d/whereabouts.conf # Yuki~ Nikhil's note: imo we should remove "flatfile" from whereabouts vocabulary and call this "WHEREABOUTS_CONF_FILE" instead. Flatfile may be the format but it's confusing naming. | |||
This line is setting a default: i.e. it will define But if you're saying it does not work ... That is probably something else we need to figure out. I can't say why it does not work as expected since I don't know. |
| @@ -130,10 +143,16 @@ spec: | |||
| mountPath: /host/opt/cni/bin | |||
| - name: cni-net-dir | |||
| mountPath: /host/etc/cni/net.d | |||
| - name: cron-scheduler-configmap | |||
| mountPath: /cron-schedule | |||
There was a problem hiding this comment.
wrong indentation. I guess this is why the daemonset does not start in e2e tests
|
|
||
| ## Reconciler Cron Expression Configuration for live clusters via configmap (optional) | ||
|
|
||
| Yuki~ README: this section may belong in the CNO since the steps outlined are not technically part of whereabouts. I'm not sure, and suggestions are welcome. |
| @@ -66,7 +68,7 @@ func main() { | |||
| defer networkController.Shutdown() | |||
|
|
|||
| s := gocron.NewScheduler(time.UTC) | |||
| schedule := cronExpressionFromFlatFile() | |||
| schedule := determineCronExpression() | |||
There was a problem hiding this comment.
hm, I think you still need to react to updates to the cron expression; meaning, this will only occur when the process starts.
Either you use an higher lib - like fsnotify - or implement something like that using a goroutine that checks if the file's contents have changed, and if so, restart the scheduler.
I guess even exiting the process could be good enough (but worse than reloading the config), since the pod would restart.
Added a configmap with "cron_schedule" key value pair. The value of cron_schedule (i.e. "30 4 * * *") will be put into a file of the same name on whereabouts ip-reconciler pods, such that when the reconciler is enabled, the file should appear on the pod.
Whereabouts will now search for the reconciler_cron_file, and if present, it will use the cron schedule from the file. If the file is not present a default value from the flatfile will be used instead.