/test

Docs changes look good to me, though I wonder why turn the feature off by default. I'll defer those details to @cilium/ipsec owners though.

nit: Please clean up the commit message.

Currently it shows like this:

ipsec: Add jitter to key file watcher to prevent thundering herd . Fixes#: 42721
Signed-off-by: Sharvil Shetty <sharvilshetty.200@gmail.com>

fixed documentation indents

Signed-off-by: Sharvil Shetty <sharvilshetty.200@gmail.com>

ipsec: Add key rotation jitter to prevent thundering herd. Fixes: https://github.com/cilium/cilium/issues/42721

Signed-off-by: Sharvil Shetty <shavyshetty@users.noreply.github.com>

ipsec: Add jitter to key file watcher to prevent thundering herd . Fixes#: 42721

Signed-off-by: Sharvil Shetty <sharvilshetty.200@gmail.com>

ipsec: Add key rotation jitter to prevent thundering herd. Fixes: https://github.com/cilium/cilium/issues/42721

Signed-off-by: Sharvil Shetty <shavyshetty@users.noreply.github.com>

ipsec: Add jitter to key file watcher to prevent thundering herd . Fixes#: 42721

Signed-off-by: Sharvil Shetty <sharvilshetty.200@gmail.com>

ipsec: Add key rotation jitter to prevent thundering herd. Fixes: https://github.com/cilium/cilium/issues/42721

Signed-off-by: Sharvil Shetty <shavyshetty@users.noreply.github.com>

It should be more like:

ipsec: Add jitter to key file watcher to prevent thundering herd 

We observed problems in our cluster where [...]

In order to fix this, add jitter into each agent to spread the
control plane load over a longer period so that the control plane
is not overwhelmed by many updates at the same time.

Signed-off-by: Sharvil Shetty <sharvilshetty.200@gmail.com>

ACK on the commit messages, I thought I cleaned them up, but it looks like not. Will do.

About the disabled by default, I left a note in the comment thread:

Based on the preferences in this thread, I updated the logic such that jitter will be between [0, maxRotation/2] if enabled. I kept the option as an opt-in for clusters rather than enabling jitter by default. Let me know if you all disagree with this.
#44263 (comment)

My thought is that smaller clusters won't encounter this issue, and cluster owners might prefer an immediate rotation instead. We only started seeing issues after reaching a certain size. I am flexible to go either way will wait for others to chime in.

My thought is that smaller clusters won't encounter this issue, and cluster owners might prefer an immediate rotation instead.

We do have some logic in pkg/backoff that scales time intervals depending on cluster size, might be another option to consider if the raw jitter numbers are too much. Though maybe that is a configuration not for the jitter but more for the maxRotation value(?)

Heads up a couple of the smoke tests fails, which will require you to update the PR. The failures list the commands to fix these things up at the end:
https://github.com/cilium/cilium/actions/runs/22002029026/job/63579007443?pr=44263
https://github.com/cilium/cilium/actions/runs/22002029056/job/63579029836?pr=44263

Follow-up of the comment: I discussed this briefly with Paul. We believe we can always enable this behavior using a relatively small jitter delay, such as [0, keyRotationDuration/10]. I'd kindly ask to:

1. adjust code accordingly
2. remove the agent/helm flag to enable/disable: would not be easy and intuitive to use, let's always add a small jitter.
3. adjust Doc accordingly

Sorry, I missed @pchaigno's comment from a couple of days ago. I am not sure I understand this part:

If we go for [0, maxRotation/2], then we've just halved the rotation duration for all existing users in the worst case. Not great.

Just curious, how does this effectively half the rotation duration? And if we instead do /10, would that mean we have reduced the rotation duration by a factor of 10?

In the absence of jitter, the max rotation duration is respected, so how does adding a delay ( jitter) to keyfilewatcher alter that behavior and affect the validity of the old key?

@shavyshetty The worst case is when jitter = maxRotation/2, which is 2min30sec in the default case with maxRotation=5min. This means nodes realize there's a new key only after 2min30sec. As a result, the rotation procedure has only 5min - 2min30sec before we cleanup old state/policies referring to the previous key.

In large clusters this might not be enough, for this reason we're suggesting using a maximum jitter relatively small wrt the maximum key rotation time.

@shavyshetty The worst case is when jitter = maxRotation/2, which is 2min30sec in the default case with maxRotation=5min. This means nodes realize there's a new key only after 2min30sec. As a result, the rotation procedure has only 5min - 2min30sec before we cleanup old state/policies referring to the previous key.

In large clusters this might not be enough, for this reason we're suggesting using a maximum jitter relatively small wrt the maximum key rotation time.

This logic change sounds good to me.

I do want to note that there is some natural jitter already, because in kubernetes, an update to a secret / configmap is not instant across all nodes. It takes some time for all kubelet instances to notice the change, and make the update. In my experience, it spreads the changes out over ~90 seconds.

While it is a bit confusing to have jitter at two levels (one at kubelet, one introduced in this PR), I think it is still the right call. For large clusters, we likely want a larger jitter window & a larger maxRotation.

Thanks for your reviews @Artyop, @adkafka, @joestringer, @pchaigno, and @smagnani96. I think I have now incorporated all your feedback, based on our collective understanding of the trade-offs for each option we explored.
I appreciate your willingness to incorporate this change into Cillium.

Please let me know if any further changes are needed for some CI tests.

@shavyshetty you need to go fmt to have the go checks to pass

/test

@smagnani96 @pchaigno @joestringer
Hey folks, just checking if anything is pending on my end for the PR to be merged?

/test

/test

Thanks for the contribution @shavyshetty 🎉