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

Plan Update and Trigger with Wait #1470

Merged
merged 1 commit into from
May 27, 2020
Merged

Plan Update and Trigger with Wait #1470

merged 1 commit into from
May 27, 2020

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented Apr 15, 2020

--wait and --wait-time added for plan update and plan trigger

Signed-off-by: Ken Sipe kensipe@gmail.com

Copy link
Member

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

lgtm.

I wouldn't mind having the WaitTime added to the plan status --wait command as well, so we have a unified set of arguments.

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

I would like to postpone this PR until we have the instance admission controller (IAC) active by default. The problem is that in the absence of IAC, IC will effectively ignore updates if there is already a plan in progress. This will lead to WaitForInstance method not seeing any UID change and so the command will hang, potentially forever. This behavior was always there but this PR makes it easy to run into this issue.

@kensipe
Copy link
Member Author

kensipe commented Apr 17, 2020

@zen-dog while the story can be improved... we can doc the feature until the IAC is in place... what is the timeframe of IAC?

@kensipe
Copy link
Member Author

kensipe commented Apr 17, 2020

the rejection seems unwarranted... the "will hang, potentially forever" seems scary but it is not. This is a known situation.. and is on the client side. no harm will come from it and the "hang" if invoked can be cancelled with ctrl+c break... under this scenario it would act like a kubectl --watch which any regular user of kube would now how to break.

@zen-dog
Copy link
Contributor

zen-dog commented Apr 17, 2020

I don't believe many users are aware of all existing pitfalls that arise without active IAC in the presence of multiple plans. And I'd rather not expose users to them via a CLI option. The ETA for making IAC required is 0.13. That being said, feel free to merge anway if you feel strongly about needing this feature.

@zen-dog zen-dog dismissed their stale review April 17, 2020 14:42

¯_(ツ)_/¯

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe
Copy link
Member Author

kensipe commented May 26, 2020

rebased to the latest... we also now have IAC as default on.

@kensipe kensipe added the release/highlight This PR is a highlight for the next release label May 26, 2020
@kensipe kensipe merged commit 73cd8c2 into master May 27, 2020
@kensipe kensipe deleted the ken/update-wait branch May 27, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/highlight This PR is a highlight for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants