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

update-agent: Added reboot-wait parameter #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jasper-Ben
Copy link

Added reboot-wait parameter

This adds an reboot-wait parameter, which waits, after the last pod was terminated, an
fixed amount of time to finalize operations before reboot. This solves some problems
with storage provisioners like rook.

This PR is basically a reopening of the still relevant coreos/container-linux-update-operator#192

/cc: @martin31821 @johannwagner

@invidian
Copy link
Member

Hey @Jasper-Ben! Thanks for opening the PR. While I was re-creating issues from archived https://github.com/coreos/container-linux-update-operator repository I saw coreos/container-linux-update-operator#192 and comments there and I agreed with comments there, that the purpose of this PR was not entirely clear and it seems more like a workaround rather than a proper solution for the problem. This is why I didn't create an issue for it here.

Do you mind opening an issue for it so we can discuss it before we move to implementing (or merging) it?

From technical point of view, as a person who was recently doing some improvements to this project, I would also like to avoid adding new features until #60 is resolved, so we have a facility to actually test those new features, as currently we have almost no tests.

@Jasper-Ben
Copy link
Author

@invidian Thanks for the feedback! I agree that the PR is just a workaround, but the issue it tries to tackle is relevant and probably not easy to solve. And I'd rather have a workaround for now, than no working solution at all. I will follow your advise and open an issue and maybe we can work something out. 🙂

@Jasper-Ben
Copy link
Author

@invidian I just found your existing issue #30, which to my understanding basically describes the issue this PR tries to fix.

This adds an reboot-wait parameter, which waits, after the last pod was terminated, an
fixed amount of time to finalize operations before reboot. This solves some problems
this storage provisioners like rook.
@invidian
Copy link
Member

Hey @Jasper-Ben, sorry for waiting so long with this PR. I've created #132 today, as I think there is a good way to wait for volumes to be detached from node in Kubernetes right now, so we could leverage that.

I'm also getting closer into having some reasonable test coverage for agent (#116), which I believe are required for this feature, though I may consider deferring it further until #134 is resolved, depending how complex it will be.

@Jasper-Ben
Copy link
Author

Hey @Jasper-Ben! Thanks for opening the PR. While I was re-creating issues from archived https://github.com/coreos/container-linux-update-operator repository I saw coreos/container-linux-update-operator#192 and comments there and I agreed with comments there, that the purpose of this PR was not entirely clear and it seems more like a workaround rather than a proper solution for the problem. This is why I didn't create an issue for it here.

Do you mind opening an issue for it so we can discuss it before we move to implementing (or merging) it?

From technical point of view, as a person who was recently doing some improvements to this project, I would also like to avoid adding new features until #60 is resolved, so we have a facility to actually test those new features, as currently we have almost no tests.

No worries, thanks for looking into this! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants