-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 KCP feature to clusterctl alpha rollout #6858
✨ Add KCP feature to clusterctl alpha rollout #6858
Conversation
f77c1f3
to
6dd881d
Compare
34d4f49
to
4747856
Compare
/retest tests were successful on Prow |
/test pull-cluster-api-e2e-informing-main flaky CC test |
24f31ae
to
95c0015
Compare
@@ -53,3 +65,8 @@ func setRestartedAtAnnotation(proxy cluster.Proxy, name, namespace string) error | |||
patch := client.RawPatch(types.MergePatchType, []byte(fmt.Sprintf("{\"spec\":{\"template\":{\"metadata\":{\"annotations\":{\"cluster.x-k8s.io/restartedAt\":\"%v\"}}}}}", time.Now().Format(time.RFC3339)))) | |||
return patchMachineDeployment(proxy, name, namespace, patch) | |||
} | |||
|
|||
func setRolloutAfter(proxy cluster.Proxy, name, namespace string) error { | |||
patch := client.RawPatch(types.MergePatchType, []byte(fmt.Sprintf("{\"spec\":{\"rolloutAfter\":\"%v\"}}", time.Now().Format(time.RFC3339)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea to handle issues regarding different timezones or async local times: we could use for rolloutAfter
the creation timestamp + a few seconds of the most recent control plane machine. In this case the rolling update should start in any case.
CAPI is checking if the machines creation timestamp is before rolloutAfter and rolloutAfter is before the reconciliation time.
return machine.CreationTimestamp.Before(rolloutAfter) && rolloutAfter.Before(reconciliationTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding #6858 (comment)
AFAIK kubectl rollout restart
has no rolloutAfter
alike field. I think that the rollout will be performed immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And btw, e.g. Deployment
has also a restartedAt
field like our MachineDeployment
.
❯ kk rollout restart deploy coredns
deployment.apps/coredns restarted
❯ kk get deploy coredns -oyaml | grep restartedAt
kubectl.kubernetes.io/restartedAt: "2022-07-18T12:09:53+02:00"
Same for Statefulsets
❯ kk rollout restart sts csi-resizer-cinder
statefulset.apps/csi-resizer-cinder restarted
❯ kk get sts csi-resizer-cinder -oyaml | grep restartedAt
kubectl.kubernetes.io/restartedAt: "2022-07-18T12:11:06+02:00"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR :-)
All in all it looks pretty good, only some nits :-)
Still want to / have to play around with it.
95c0015
to
c8c2742
Compare
c8c2742
to
3e892ae
Compare
One question: Does making use of E.g. a user may have already set So what would happen if Edit: so I currently think we should not make use of |
AFAIK the |
cc @ykakarap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks interesting to me. I will take a look a this.
3e892ae
to
86e02a0
Compare
86e02a0
to
2d38aa0
Compare
/retest |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/hold
for @fabriziopandini's review
Great work thank you!
/hold cancel |
/hold sorry, overlooked your hold Vince |
Will solve the merge conflict after the review from @fabriziopandini |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one question, otherwise lgtm
@@ -49,6 +49,8 @@ require ( | |||
github.com/blang/semver/v4 v4.0.0 // indirect | |||
github.com/cespare/xxhash/v2 v2.1.2 // indirect | |||
github.com/containerd/containerd v1.6.12 // indirect | |||
github.com/coredns/caddy v1.1.0 // indirect | |||
github.com/coredns/corefile-migration v1.0.18 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q for my understanding, why are there such changes in hack/tools/go.mod? this sems unrelated to the other changes in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also not sure why these changes are in the PR. But go mod tidy
and the Prow job want this change.
IIRC we found out why these changes happen @chrischdi, right? Do you still know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hack/tools/tilt-prepare uses "sigs.k8s.io/cluster-api/cmd/clusterct/*"
"sigs.k8s.io/cluster-api/cmd/clusterct/*" now uses "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" (for the KubeadmControlPlane type)
"sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" uses "github.com/coredns/corefile-migration" (for the KCP webhook)
"github.com/coredns/corefile-migration" uses "github.com/coredns/caddy" (internally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came to the same conclusion as stefan 🏆
@tobiasgiese Can you please rebase the PR? I would merge then |
Signed-off-by: Tobias Giese <tobias.giese@mercedes-benz.com>
6eb2e31
to
4593c3c
Compare
Done, thank you 👍🏻 |
Thank you very much!! (especially for the patience :)) /lgtm |
LGTM label has been added. Git tree hash: fad73e5eb2d6bfa576ff62c98f846f5987bce042
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
thanks for pushing this over the line @tobiasgiese! |
Signed-off-by: Tobias Giese tobias.giese@mercedes-benz.com
What this PR does / why we need it:
This PR implements the sub-commands
rollout
,pause
andresume
forclusterctl alpha rollout
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #3439 (replaces #6857)