[WIP] Enhance daemonset on rollingupdate. #1140

zhangxiaoyu-zidif · 2019-07-14T23:55:20Z

Now we find Daemonset has a weak ability to do rollingUpdate. So we need to do moere enhancement, such as pause, selector & and surge and more.

So we want to do some enhancement on Daemonset rollingupdate.

fix kubernetes/kubernetes#80130

/cc @resouer @answer1991

k8s-ci-robot · 2019-07-14T23:55:22Z

@zhangxiaoyu-zidif: GitHub didn't allow me to request PR reviews from the following users: answer1991.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Now we find Daemonset has a weak ability to do rollingUpdate. So we need to do moere enhancement, such as pause, selector & and surge and more.

So we want to do some enhancement on Daemonset rollingupdate.

fix kubernetes/kubernetes#80130

/cc @resouer @answer1991

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

draveness · 2019-07-15T00:32:51Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+- Add new fields `Selector` and `Partition` for existing RollingUpdateDaemonSet.
+
+### Non-Goals
+- This inplement is only implemeted to affect to update strategy of Daemonset. No other behaviors of Daemonset will be changed.


s/inplement/implement

OK, thanks. I will fix it this weekend.

draveness · 2019-07-15T00:33:25Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+  - Add `SurgingRollingUpdate` for Daemonset as a new update strategy.
+  - Add two more fields, `partition` and `selector` for `RollingUpdate` and `SurgingRollingUpdate`.
+    - `partition` is the number of DaemonSet pods remained to be old version.
+    - `selector` is to query over nodes whoes labels are matched by the Daemonset `RollingUpdate` or `SurgingRollingUpdate`.


It would be tricky if the daemonset use host port to expose service.

Never mind, user can choose their own proper update strategy. We just provide more choices. And to some case, surge is necessary, I will add them later.

kow3ns · 2019-07-15T16:41:38Z

/assign @janetkuo

janetkuo · 2019-08-01T00:22:04Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+
+1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if  some results of new version image meet expect. If not, user can rollback images to stable version.
+
+1. In some case, user do not want to stop some services when rolling update, so SurgingRollingUpdate is a must choice.


Would you clarify this more? This sentence is a bit vague. Try making it more concrete/convincing. Maybe include some real world examples.

Also note that DaemonSet rolling update is designed to be delete-first-create-later (and only create when the old Pod is gone) because most DaemonSet pods cannot tolerate two instances running in the same node at a time. For example, it's common to have hostPort specified - how do you handle that?

Ref a previous proposal for the same effort: kubernetes/community#977

OK, I would give more real examples soon.

Now In our cluster, we deploy DNS resolver and other containers serving infrastructure by daemonset in every nodes. As an online e-commercial services which have 200K+ QPS, we can not stops those services even in a very short time. So surgingRollingUpdate is a necessary strategy for our production environment.

PS. Adding partition and Selector for SurgingRollingUpdate will help user to find some errors such as hostPort conflicts, in a limit range of 404 disaster.

draveness · 2019-08-05T06:40:18Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md


 ## Proposal

 ### User Stories

 #### Story 1
+In some end users' clusters, they deploy DNS resolver and other containers serving infrastructure by daemonset in every nodes. To an online e-commercial services which serve 200K+ QPS, it must cause cascading disaster to stop those infrastructure services even in a very short time by current Daemonset RollingUpdate strategy. So surgingRollingUpdate is a necessary strategy for our production environment. 


I don't think a rolling update would solve this problem, you can't replace a running pod which specify host port in the spec without stopping it. SurgingRollingUpdate does not make sense to me, delete by partitions or in specific order may mitigate this problem.

SurgingRollingUpdate will solve those cases without binding host port, so it could be a supplement for existing update strategy. Operators should have knowledge to choose suitable one. As to partitions and selector, they can help to avoid mistake and reduce its cost.

@draveness Not sure if I'm understanding your correctly but: how rollout strategy is related to host port?

@draveness Not sure if I'm understanding your correctly but: how rollout strategy is related to host port?

It is a common pattern to use host port in daemonset, suppose we use a rolling update strategy like deployment, deamonsets should create a new pod with the same hostport on the same machine, but it will fail to start since the port on the machine is owned by the older daemonset pod. SurgingRollingUpdate does not hold since we can't create more pods with the same port on the same machine.

SurgingRollingUpdate will solve those cases without binding host port, so it could be a supplement for existing update strategy.

If we use a damonset without hostport, why not using deployment instead.

We could use daemonset for log collecting which does not need a hostport, but I think two log collecting pod running on the same machine could be problematic. Add a maxUnavailable field instead of surging to the daemonset pod would make more sense.

I don't think that's the intention of this proposal which is trying to propose an optional strategy instead of enhancement for existing ones.

But yeah, this KEP dose not make sense to me as well since it mixed too many things together, and the use cases are not clear.

resouer · 2019-08-07T21:24:24Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+### User Stories
+
+#### Story 1
+In some end users' clusters, they deploy DNS resolver and other containers serving infrastructure by daemonset in every nodes. To an online e-commercial services which serve 200K+ QPS, it must cause cascading disaster to stop those infrastructure services even in a very short time by current Daemonset RollingUpdate strategy. So surgingRollingUpdate is a necessary strategy for our production environment. 


This narrative is problematic to me. A user story should be described in objective language and be general as possible as you can.

resouer · 2019-08-09T01:36:53Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+	// Default value is 0.
+	// Maximum value is status.DesiredNumberScheduled, which means no pod will be updated.
+	// +optional
+	Partition *int32 `json:"partition,omitempty" protobuf:"varint,3,opt,name=partition"`


No sure why the existing MaxUnnavailable strategy is missing?

Sorry for misunderstanding, I use ”...“ to omit the existed fields.

resouer

Seems this KEP mixed three things at least in one doc, and barely provided concrete use cases in the narrative.

Ability to Pause during DaemonSet rollout;
Ability of Select & Partition during DaemonSet rollout;
Ability of Surging during DaemonSet rollout.

IMO, each of these proposals will need its own KEP for both API change and detailed design. I would suggest you to create an umbrella issue to link them together, and propose them one by one. Start from the easiest change of course.

resouer · 2019-08-09T01:41:35Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md


 ## Proposal

 ### User Stories

 #### Story 1
+In some end users' clusters, they deploy DNS resolver and other containers serving infrastructure by daemonset in every nodes. To an online e-commercial services which serve 200K+ QPS, it must cause cascading disaster to stop those infrastructure services even in a very short time by current Daemonset RollingUpdate strategy. So surgingRollingUpdate is a necessary strategy for our production environment. 


I don't think that's the intention of this proposal which is trying to propose an optional strategy instead of enhancement for existing ones.

But yeah, this KEP dose not make sense to me as well since it mixed too many things together, and the use cases are not clear.

pigletfly · 2019-08-14T16:09:23Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+
+1. In some case, user do not want to stop some services when rolling update, so SurgingRollingUpdate is a must choice.
+
+1. When rollingUpdate or surgeRollingUpdate, user should have some chioce to select some specific or random nodes to update pods running on it.


s/chioce/choices

zhangxiaoyu-zidif · 2019-08-16T09:41:56Z

Seems this KEP mixed three things at least in one doc, and barely provided concrete use cases in the narrative.

Ability to Pause during DaemonSet rollout;

Ability of Select & Partition during DaemonSet rollout;

Ability of Surging during DaemonSet rollout.

IMO, each of these proposals will need its own KEP for both API change and detailed design. I would suggest you to create an umbrella issue to link them together, and propose them one by one. Start from the easiest change of course.

OK, I will split them soon and re-send those KEPs.

damdo · 2019-10-30T09:55:24Z

Hey @zhangxiaoyu-zidif any update on this :) ?

zhangxiaoyu-zidif · 2019-11-04T01:32:01Z

Hey @zhangxiaoyu-zidif any update on this :) ?

Sorry I am busy because of Alibaba‘s Annual Singles' Day Shopping Festival, this doc will be updated later.

mikedanese · 2019-11-04T02:32:15Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+
+Consider the following scenarios:-
+
+1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if  some results of new version image meet expect. If not, user can rollback images to stable version.


How do deployments handle this?

sorry, I make it confused. I just want to execute command like using deployment for users.

Or, we should provide an observation period(pause for a while) for the upgraded Pod to determine that the upgrade is real and effective. :)

it's a good idea to serve an observation channel for users. such as show which node has finished the update and which has not in the whole update process. As to observation period, let me think how to import this feature. ：）

@wgliang we can add some fileds:

// The total nodes that are running updated daemon pod // key is node while value is namespace podName: <Namespace/PodName>. // +optional UpdatedNumberScheduledNodes map[string]string `json:"updatedNumberScheduledNodes,omitempty" protobuf:"bytes,11,rep,name=updatedNumberScheduledNodes"`

to see what the status of nodes and its daemon pod. if you want to pause the sync, you can pasued the action of ds sync.

k8s-ci-robot · 2019-11-13T09:00:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhangxiaoyu-zidif
To complete the pull request process, please assign janetkuo
You can assign the PR to them by writing /assign @janetkuo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-apps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-11-13T09:01:20Z

@zhangxiaoyu-zidif: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-enhancements-verify	`2589459`	link	`/test pull-enhancements-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wgliang · 2019-11-25T08:12:40Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+
+Consider the following scenarios:-
+
+1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if  some results of new version image meet expect. If not, user can rollback images to stable version.


Or, we should provide an observation period(pause for a while) for the upgraded Pod to determine that the upgrade is real and effective. :)

wgliang · 2019-11-25T08:15:36Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+
+1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if  some results of new version image meet expect. If not, user can rollback images to stable version.
+
+1. When rollingUpdate, user should have some chioces to select some specific or random nodes to update pods running on it.


Or, we can control the progress of the upgrade, such as setting the target percentage of the upgrade.

yes, that's why we want to use field partition enhance update strategy.

This is really essential in a complex private cloud environment.

We are also landing DaemonSet to upgrade some agent components. We also encountered these problems. From the current point of view, the only feasible solution is CRD. Looking forward to this KEP landing.

Yes, we will send our implement to other project to release this enhancement at the same time. But if it could be adopted by K/K, that is the best choice.

Cool, agree with U.

smarterclayton · 2020-01-29T22:43:40Z

Did we split surge into a new enhancement?

I think surge is the most obvious improvement for availability of existing DaemonSets (since there's no other easy way to mitigate the duplication). I suspect we could get fairly fast agreement on #51161 with a default value of zero. The primary user is host networking components that expect to coexist (and are not using host ports on pod specs, which are advisory only). I think bypassing host port requesting is a reasonable limitation for those sorts of use cases. @soltysh @dcbw because of feedback from networking providers.

krmayankk · 2020-02-25T08:11:10Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+  - Add `SurgingRollingUpdate` for Daemonset as a new update strategy.
+  - Add two more fields, `partition` and `selector` for `RollingUpdate`
+    - `partition` is the number of DaemonSet pods remained to be old version.
+    - `selector` is to query over nodes whoes labels are matched by the Daemonset `RollingUpdate`.


DaemonSet already has a nodeSelector which selects where to run the pods, why do we need to select a subset of those nodes by selector? What is the real use case ?

smarterclayton · 2020-02-29T21:04:10Z

I'm going to open a surge proposal since it has been increasingly difficult to maintain availability for node level components in daemonsets without it. I have a working implementation which mirrors deployments which I'm using to test with node services that must remain available during upgrade (i.e. CNI, CSI, or other node plugins).

smarterclayton · 2020-03-02T22:14:28Z

I opened #1591 and #1590 to carve off the surge aspect. I believe (and have prototyped) the implementation of MaxSurge consistent with Deployments is fairly straightforward and would require very little novel change to Kubernetes.

janetkuo · 2020-03-03T00:04:27Z

keps/sig-apps/20190715-rollingUpdate-enhancement-for-daemonset.md

+	// A label query over nodes that are managed by the daemon set RollingUpdate.
+	// Must match in order to be controlled.
+	// It must match the node's labels.
+	Selector *metav1.LabelSelector `json:"selector" protobuf:"bytes,2,opt,name=selector"`


Would you explain more about how this selector works with rolling update?

Note that DaemonSet works with node selector.
https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/#running-pods-on-only-some-nodes

fejta-bot · 2020-06-01T00:37:46Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-07-01T01:19:29Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-07-31T01:59:52Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-07-31T01:59:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-07-31T02:00:05Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2019

k8s-ci-robot requested a review from resouer July 14, 2019 23:55

init enhance daemonset on rollingupdate

78d299c

zhangxiaoyu-zidif force-pushed the advanced-daemonset branch from 2e99536 to 78d299c Compare July 14, 2019 23:57

draveness reviewed Jul 15, 2019

View reviewed changes

k8s-ci-robot assigned janetkuo Jul 15, 2019

janetkuo requested changes Aug 1, 2019

View reviewed changes

add use story why we need surgingRollingUpdate

faa0870

draveness suggested changes Aug 5, 2019

View reviewed changes

resouer reviewed Aug 7, 2019

View reviewed changes

resouer reviewed Aug 9, 2019

View reviewed changes

resouer suggested changes Aug 9, 2019

View reviewed changes

pigletfly reviewed Aug 14, 2019

View reviewed changes

zhangxiaoyu-zidif mentioned this pull request Nov 4, 2019

[WIP]Enhance daemonset on rollingupdate #1352

Closed

mikedanese reviewed Nov 4, 2019

View reviewed changes

delete SurgingUpdate

2589459

wgliang reviewed Nov 25, 2019

View reviewed changes

krmayankk reviewed Feb 25, 2020

View reviewed changes

smarterclayton mentioned this pull request Mar 2, 2020

DaemonSets should allow MaxSurge to minimize workload downtime during upgrade #1590

Merged

janetkuo reviewed Mar 3, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 1, 2020

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2020

k8s-ci-robot closed this Jul 31, 2020


		1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if some results of new version image meet expect. If not, user can rollback images to stable version.

		1. In some case, user do not want to stop some services when rolling update, so SurgingRollingUpdate is a must choice.


		1. In some case, user do not want to stop some services when rolling update, so SurgingRollingUpdate is a must choice.

		1. When rollingUpdate or surgeRollingUpdate, user should have some chioce to select some specific or random nodes to update pods running on it.


		Consider the following scenarios:-

		1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if some results of new version image meet expect. If not, user can rollback images to stable version.


		1. When executing Daemonset update, even those Pods run well, user need to pause updation to check if some results of new version image meet expect. If not, user can rollback images to stable version.

		1. When rollingUpdate, user should have some chioces to select some specific or random nodes to update pods running on it.

[WIP] Enhance daemonset on rollingupdate. #1140

[WIP] Enhance daemonset on rollingupdate. #1140

Conversation

zhangxiaoyu-zidif commented Jul 14, 2019

k8s-ci-robot commented Jul 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kow3ns commented Jul 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangxiaoyu-zidif commented Aug 16, 2019

damdo commented Oct 30, 2019

zhangxiaoyu-zidif commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 13, 2019

k8s-ci-robot commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 29, 2020

Choose a reason for hiding this comment

smarterclayton commented Feb 29, 2020

smarterclayton commented Mar 2, 2020

Choose a reason for hiding this comment

fejta-bot commented Jun 1, 2020

fejta-bot commented Jul 1, 2020

fejta-bot commented Jul 31, 2020

k8s-ci-robot commented Jul 31, 2020

k8s-ci-robot commented Jul 31, 2020

resouer Aug 7, 2019 •

edited

Loading

resouer left a comment •

edited

Loading