Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to enable worker graceful shutdown #226

Conversation

sdaberdaku
Copy link
Member

In this PR I have automated the process of enabling the Worker Graceful Shutdown feature.

Enabling the option will:

  • Add a preStop lifecycle event to all worker Pods;
  • Set the shutdown.grace-period configuration property to gracePeriod;
  • Configure the workers' accessControl since the default system access control does not allow graceful shutdowns.

I also added a test to check if the graceful shutdown endpoint can be successfully called on the worker Pods.

Fixes #189

@cla-bot cla-bot bot added the cla-signed label Sep 25, 2024
@sdaberdaku sdaberdaku force-pushed the feature/worker-graceful-shutdown-option branch from 58e7763 to ebb5efa Compare September 25, 2024 19:51
@sdaberdaku sdaberdaku marked this pull request as ready for review September 25, 2024 19:59
@nineinchnick nineinchnick self-requested a review September 25, 2024 20:30
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I have some comments about making it even more robust, and not allowing users to make configuration mistakes.

charts/trino/templates/configmap-worker.yaml Show resolved Hide resolved
charts/trino/templates/tests/test-graceful-shutdown.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/templates/deployment-worker.yaml Show resolved Hide resolved
@sdaberdaku
Copy link
Member Author

Hello @nineinchnick,

Thank you so much for finding the time to review this PR!
I will try to implement all the required changes, hopefully during this week and I will let you know then they are ready.

@nineinchnick
Copy link
Member

Thanks! No rush and we can always chat on the Trino slack if anything is not clear enough

@sdaberdaku sdaberdaku force-pushed the feature/worker-graceful-shutdown-option branch from ebb5efa to 84d69aa Compare October 1, 2024 19:46
@sdaberdaku
Copy link
Member Author

sdaberdaku commented Oct 1, 2024

Dear @nineinchnick,

I have addressed all your comments.

Regarding the test implementation, I considered using a long-running query; however, I was uncertain about its appropriateness as it seemed more like a Trino test than a Helm Chart test. Ultimately, my testing approach involves deleting a worker pod using kubectl and verifying that the logs of the affected pod contain the "Shutdown requested" string. This method effectively tests both the worker's authorization and the lifecycle configuration.

Looking forward to your feedback!

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great now, thanks! Resolve the conflict and we can merge this.

charts/trino/templates/deployment-worker.yaml Outdated Show resolved Hide resolved
@nineinchnick nineinchnick added the enhancement New feature or request label Oct 3, 2024
@sdaberdaku sdaberdaku force-pushed the feature/worker-graceful-shutdown-option branch 3 times, most recently from 4b782a0 to 35d729c Compare October 3, 2024 10:16
@sdaberdaku
Copy link
Member Author

I rebased and addressed your last comment. Should be ready now!

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nitpick to get better docs.

charts/trino/values.yaml Outdated Show resolved Hide resolved
@sdaberdaku sdaberdaku force-pushed the feature/worker-graceful-shutdown-option branch from 35d729c to 3ae5670 Compare October 3, 2024 11:33
@nineinchnick nineinchnick merged commit bcea90a into trinodb:main Oct 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

accessControl properties are only added to the coordinator
2 participants