search

k8snetworkplumbingwg / whereabouts

A CNI IPAM plugin that assigns IP addresses cluster-wide
Apache License 2.0
273 stars 120 forks source link

Extended support for customizing whereabouts ip-reconciler cron schedule #392

Open nicklesimba opened 7 months ago

nicklesimba commented 7 months ago

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.

maiqueb commented 7 months ago

Hm I wonder if this isn't good enough for what you need: https://github.com/k8snetworkplumbingwg/whereabouts/pull/317

IIUC, the user would edit the existing ds - configuring the cron via env variable, and kill the ds pods.

nicklesimba commented 7 months ago

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.

See: https://github.com/k8snetworkplumbingwg/whereabouts/pull/392/files#diff-307d21cb211139c5c6d37b29f30d91ccd3d8b39ea8a338c984b560ec0769e3f1R160

Edit: I think you might be correct after all...thank you for the suggestion Miguel!

cgoncalves commented 7 months ago

@nicklesimba are you agreeing that https://github.com/k8snetworkplumbingwg/whereabouts/pull/317 is sufficient? If so, this PR could be closed I think.

nicklesimba commented 6 months ago

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.

maiqueb commented 6 months ago

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.

nicklesimba commented 6 months ago

My understanding is that the whereabouts install-cni.sh script has the following definition:

https://github.com/k8snetworkplumbingwg/whereabouts/pull/392/files#diff-c8589450558444bb4def3f2633a8eb70db55df1c2a86366d265aeb768e3e625bR16

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.

maiqueb commented 6 months ago

My understanding is that the whereabouts install-cni.sh script has the following definition:

https://github.com/k8snetworkplumbingwg/whereabouts/pull/392/files#diff-c8589450558444bb4def3f2633a8eb70db55df1c2a86366d265aeb768e3e625bR16

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.

This line is setting a default: i.e. it will define WHEREABOUTS_RECONCILER_CRON with the value of the WHEREABOUTS_RECONCILER_CRON environment value, or with "30 4 *" is the env variable is not defined.

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.