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 restart utility function. #635

Merged
merged 2 commits into from
Sep 21, 2021
Merged

Add restart utility function. #635

merged 2 commits into from
Sep 21, 2021

Conversation

vifino
Copy link
Contributor

@vifino vifino commented Sep 20, 2021

This PR adds the restart(name) function on Api<K> where K is one of Deployment, DaemonSet, StatefulSet or ReplicaSet.
It also adds the complementary method to the kube crate.

To implement this functionality, I chose to add chrono to the kube-core crate, as the alternatives involved lots of code and it's already used in the bigger crate.

This fixes #630 by implementing the necessary functionality.

I hope this PR is acceptable.
Thank you.

Signed-off-by: Adrian Pistol <vifino@tty.sh>
@clux
Copy link
Member

clux commented Sep 20, 2021

Hello again! Thanks a lot for doing this.
I think this looks perfectly appropriate! Started CI.

Just a bit of double checking here. I was cross-referencing a bit with kubectl it does seem like they use a fieldManager with a strategic merge patch.

Now, I don't know why they have chosen to do this (it could be a fairly arbitrary choice), but it would be good to make sure that the strategy we use won't interfere with other annotations, so if you are able to double check that (in a quick test with restart on a deploy with more labels to see that other labels persist), that would be great.

@vifino
Copy link
Contributor Author

vifino commented Sep 20, 2021

Hey @clux!

I've tested this on a few deployments, all work fine. Some of those deployments have lots of labels and quite a few annotations.
A kubectl rollout restart still works just fine after doing it this way.

The only thing I noticed was that there is both a kubectl.kubernetes.io/restartedAt and a kube.kubernetes.io/restartedAt annotation, but that makes sense.
It works just fine for me.

@vifino
Copy link
Contributor Author

vifino commented Sep 20, 2021

Hm. The rustfmt check fails, but cargo fmt --all does nothing?
I'll apply the changes and force-push.

Signed-off-by: Adrian Pistol <vifino@tty.sh>
@clux
Copy link
Member

clux commented Sep 20, 2021

Hm. The rustfmt check fails, but cargo fmt --all does nothing?
I'll apply the changes and force-push.

Ah, sorry, yeah, we use a couple of nightly only formatting rules. make fmt does the equivalent of CI.

@clux
Copy link
Member

clux commented Sep 20, 2021

n a few deployments, all work fine. Some of those deployments have lots of labels and quite a few annotations.
A kubectl rollout restart still works just fine after doing it this way.

Ah, perfect. That's the type of thing I was worried about. (I imagine it would not be like that if we took kubectl's label since they have a manager, but that's irrelevant).

@clux
Copy link
Member

clux commented Sep 21, 2021

Ah, for some reason I have to keep approving since you're a first time contributor. Anyway, if just rustfmt fails here I'll just merge it. There's a follow-up system that fixes any issues.

@clux
Copy link
Member

clux commented Sep 21, 2021

Ah, it's fixed. Perfect. Thank you very much. I'll get it into the next release probably sometime this week :-)

@clux clux merged commit 1c9c9b2 into kube-rs:master Sep 21, 2021
@vifino
Copy link
Contributor Author

vifino commented Sep 21, 2021

Nice! Thanks for merging.
I'll be glad when the next release gets tagged! :)

nightkr pushed a commit to nightkr/kube-rs that referenced this pull request Oct 5, 2021
* kube-core: Add Restart marker and request.

Signed-off-by: Adrian Pistol <vifino@tty.sh>

* kube: Add restart method to Deployments and Sets.

Signed-off-by: Adrian Pistol <vifino@tty.sh>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
@clux
Copy link
Member

clux commented Oct 9, 2021

Released in 0.61.0. Thanks again :-)

@vifino
Copy link
Contributor Author

vifino commented Oct 10, 2021

Woohoo!

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

Successfully merging this pull request may close these issues.

How to do a rolling restart of a deployment?
2 participants