Add Lifecycle hooks configuration to Tenant

[mctoohey]

Adds Lifecycle hooks configuration to Tenant. This is parsed down to the MinIO container of each pod in the pool.

The tenant already allows liveliness, readiness and startup probes to be configured. Similar to those, the usage of lifecycle hooks is entirely optional. Adding configuration for lifecycle hooks (https://kubernetes.io/docs/tasks/configure-pod-container/attach-handler-lifecycle-event/) allows custom postStart/preStop hooks can be added to the MinIO container if desired.

[mctoohey]

Hi @jiuker, I was wondering if you had any thoughts on this change?

[pjuarezd]

@mctoohey would you mind tell us how you are planning to use the lifecycle hooks?, how can it be useful?

[pjuarezd]

Created a PR to your branch to add the CRD docs update @mctoohey mctoohey#1

[mctoohey]

@pjuarezd thank you for you response. Sure, I can provide some more context.
I am particular interested in the preStop lifecycle hook. This could be used to add an additional wait or check before shutting down the tenant pod. Giving time for remaining requests to process and for event notifications to be sent during which time the pod will not accept any new connections.
For my particular use case I am very interested in ensuring all bucket notifications for all requests are sent in the event of a graceful pod termination. I would like to have the option to add a preStop hook that waits a short period before sending the shutdown signal to the MinIO process. It could potentially check the minio_notify_current_send_in_progress metic to see if there are any notifications still being sent.
I acknowledge it is quite niche use case. Which is why my change only exposes this configuration to the tennant spec but leaves up to the end user to determine if/how they may want to use it.

[jiuker]

@pjuarezd thank you for you response. Sure, I can provide some more context. I am particular interested in the preStop lifecycle hook. This could be used to add an additional wait or check before shutting down the tenant pod. Giving time for remaining requests to process and for event notifications to be sent during which time the pod will not accept any new connections. For my particular use case I am very interested in ensuring all bucket notifications for all requests are sent in the event of a graceful pod termination. I would like to have the option to add a preStop hook that waits a short period before sending the shutdown signal to the MinIO process. It could potentially check the minio_notify_current_send_in_progress metic to see if there are any notifications still being sent. I acknowledge it is quite niche use case. Which is why my change only exposes this configuration to the tennant spec but leaves up to the end user to determine if/how they may want to use it.

That case should be minio's issue. @mctoohey

[mctoohey]

The other use case is a preStop that waits for incoming requests to finish being processed allows you to do a rolling restart (which I know MinIO doesn't recommend) of all pods in the replica set without experiencing any downtime.

[mctoohey]

I might be able to set --shutdown-timeout argument on MinIO to increase the graceful shutdown period. The preStop hook would still be nice so I could set up some custom alerting if MinIO fails to gracefully shutdown (with all requests inished).

[hrenard]

Nice !
In case of non-breaking rollout, waiting the cluster maintenance check to be ok would avoid some downtimes (like when upgrading).

[pjuarezd]

I might be able to set --shutdown-timeout argument on MinIO to increase the graceful shutdown period. The preStop hook would still be nice so I could set up some custom alerting if MinIO fails to gracefully shutdown (with all requests inished).

Having a preStop hook could only potentially add 2 additional seconds to the grace period, just somenthing to consider.
Instead it will be taking away some grace period time from the minio process to do graceful exit.

Default minio shutdown timeout is 5 seconds, extending the --shutdown-timeout to a bigger number seems like good option to give some additional room to minio to finish processing, since Kubernetes default grace period is 30 seconds, most likelly minio will exit on itself way before Kubernetes kills the process.

Extending the pod .spec.terminationGracePeriodSeconds: 30 to somenthing bigger in combination with extend the minio --shutdown-timeout would also work, but yeah, this is measure is only adding more time, not verifying that all received requests are already fulfilled.

Now, when a pod enters Terminating state, is removed from the endpoints and stops getting new traffic, wich means that only requests received before the terminating event might be pending to be shared with minio peers.

I don't think additional measures to ensure minio can finish requests are really needed, minio is already prepared to gracefully exit on SIGTERM in a short time fashion, but the preStop hook could be useful if we want to notify the pod is being terminated to Prometheus (for example), same goes for postStart, I would play with the events to do custom notifications instead.

[mctoohey]

Instead it will be taking away some grace period time from the minio process to do graceful exit.

My thought is that the preStop hook adds a period of time where the MinIO process will not receive any new connections and is still processing existing ones. So that when it does receive the SIGTERM it will be able to shutdown very quickly as there is nothing left to do.

minio is already prepared to gracefully exit on SIGTERM in a short time fashion

In my experience when restarting one MinIO pod in a multi node setup. Some client requests are always disrupted (not a huge deal since clients can retry) and some event notifications are dropped. My understanding from previous reading was that MinIO does not provide the guarantee that bucket notifications are delivered on shutdown.

For example if I make a PUT request, a SIGTERM is sent to MinIO, I get 200 response back but the notification may not have been sent (or been added to the queue if a queue is being used).

[pjuarezd]

In my experience when restarting one MinIO pod in a multi node setup. Some client requests are always disrupted (not a huge deal since clients can retry) and some event notifications are dropped. My understanding from previous reading was that MinIO does not provide the guarantee that bucket notifications are delivered on shutdown.

For example if I make a PUT request, a SIGTERM is sent to MinIO, I get 200 response back but the notification may not have been sent (or been added to the queue if a queue is being used).

That's right, there is no garantee that all event notifications will be delivered before the process shutdowns, but you should not lost events because of that, the right way should be to configure the notification target to persist the undelivered events, so that those are replayed once the process comes back online, ie for the Elasticsearch target set MINIO_NOTIFY_ELASTICSEARCH_QUEUE_DIR

[mctoohey]

the right way should be to configure the notification target to persist the undelivered events, so that those are replayed once the process comes back online

I should clarify that in my experience even when using a queue_dir sometimes events can be lost.

I just did a test with a kafka event notification with queue_dir set. I had a loop set to write a 1000 objects to MinIO (test_object1 to test_object1000). Restarted MinIO. After the restart I checked MinIO and saw that 456 had been written to MinIO but only 455 PUT object events had been received by Kafka and there were no events remaining in the queue directory.

I will continue to look into some of your suggestions but this is getting a bit off topic for this change. This change is really about whether we should be able to configure lifecycle hooks for the tenant container. I still think there are some use cases for lifecycle hooks such as sending start/stop notifications to external services. It would be nice if this configuration was exposed in the same way liveness, readiness and startup configuration is but leave it to the end user to determine if they have a use case for it.

[harshavardhana]

the right way should be to configure the notification target to persist the undelivered events, so that those are replayed once the process comes back online

I should clarify that in my experience even when using a queue_dir sometimes events can be lost.

I just did a test with a kafka event notification with queue_dir set. I had a loop set to write a 1000 objects to MinIO (test_object1 to test_object1000). Restarted MinIO. After the restart I checked MinIO and saw that 456 had been written to MinIO but only 455 PUT object events had been received by Kafka and there were no events remaining in the queue directory.

This should be treated as a bug, and not do workarounds around it.

Are you seeing that you did 1000 puts with 1000 success and you were doing a restart of the server after that?

[mctoohey]

@harshavardhana I've opened minio/minio#18404 with more detail on the what I have observed.

[pjuarezd]

could you rebase @mctoohey?

[pjuarezd]

I still belive this is good to have, empower final user in standard features is a good mantra

[mctoohey]

Would I be able to get another review on this? I think allowing end users to configure standard Kubernetes features on the MinIO pods is a reasonable thing to do.

[fangzhouxie]

Hi team,

Is anyone still reviewing this pull request? I find this change to be very useful and it would be great if it can be merged.

Thanks,

[cniackz]

@mctoohey or @fangzhouxie please help us resolve conflicts

[ramondeklein]

LGTM, but it would be nice to have an integration test for this.