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.

Sign up for GitHub

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

Jump to bottom

Proposal: YurtAppDaemon #422

Merged
merged 2 commits into from
Sep 29, 2021
Merged

Proposal: YurtAppDaemon #422

merged 2 commits into from
Sep 29, 2021

Conversation

kadisi
Copy link
Member

@kadisi kadisi commented Aug 10, 2021

Signed-off-by: bingyu.zj bingyu.zj@alibaba-inc.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

@openyurt-bot
Copy link
Collaborator

@kadisi: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Signed-off-by: bingyu.zj bingyu.zj@alibaba-inc.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

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.

@openyurt-bot openyurt-bot added the size/L size/L: 100-499 label Aug 10, 2021
@kadisi
Copy link
Member Author

kadisi commented Aug 10, 2021

/kind feature

@openyurt-bot openyurt-bot added the kind/feature kind/feature label Aug 10, 2021
@kadisi
Copy link
Member Author

kadisi commented Aug 10, 2021

/kind documentation

@openyurt-bot openyurt-bot added the kind/documentation kind/documentation label Aug 10, 2021
@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadisi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openyurt-bot openyurt-bot added the approved approved label Aug 10, 2021
@rambohe-ch
Copy link
Member

@kadisi Based on the proposal, An idea comes out that combine UnitedDeployment and UnitedDaemonSet, and we can add some fields in UnitedDeployment specify that create/delete Deployments/Statefulset along with create/delete nodePool.

zzguang
zzguang reviewed Aug 10, 2021
View reviewed changes
Copy link
Member

@zzguang zzguang left a comment

Choose a reason for hiding this comment

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

@kadisi , as wenjun mentioned about the united-service feature request on last community meeting, the uniteddaemonset workload template will add ServiceTemplate support in future, right?

@gnunu
Copy link
Member

gnunu commented Aug 10, 2021

@kadisi Based on the proposal, An idea comes out that combine UnitedDeployment and UnitedDaemonSet, and we can add some fields in UnitedDeployment specify that create/delete Deployments/Statefulset along with create/delete nodePool.

for daemonset, another question. now UnitedDeployment supports StatefulSet and Deployment. if DaemonSet (one pod per node in nodepool) should be supported also? i think it has its scenarios.

if the above thought is reasonable, then there is something subtle about naming here: daemonset means one deployment per nodepool (pool level daemonset), or one pod per node (node level daemonset)?

sounds UnitedDaemonSet can have some stress on nodepool level, and thus are at the same level as UnitedDeployment.

@Fei-Guo
Copy link
Member

Fei-Guo commented Aug 10, 2021

I can see the difference between UnitedDeamonset and UnitedDeployment as the former can automatically deploy workload when nodepool is created. The benefit is that we can could avoid manually changing UnitedDeployment had it been used for the same purpose. The naming maybe misleading, how about NodePoolDaemonSet?

@kadisi
Copy link
Member Author

kadisi commented Aug 12, 2021

@kadisi Based on the proposal, An idea comes out that combine UnitedDeployment and UnitedDaemonSet, and we can add some fields in UnitedDeployment specify that create/delete Deployments/Statefulset along with create/delete nodePool.

@rambohe-ch This is not necessary, UnitedDeployment is not strongly associated with NodePool。If you delete a Pool in UnitedDeployment,sub Deployments/Statefulset will be automatically deleted

@kadisi
Copy link
Member Author

kadisi commented Aug 12, 2021

for daemonset, another question. now UnitedDeployment supports StatefulSet and Deployment. if DaemonSet (one pod per node in nodepool) should be supported also? i think it has its scenarios.

if the above thought is reasonable, then there is something subtle about naming here: daemonset means one deployment per nodepool (pool level daemonset), or one pod per node (node level daemonset)?

sounds UnitedDaemonSet can have some stress on nodepool level, and thus are at the same level as UnitedDeployment.

@gnunu
for Daemonset, there is no need for more encapsulation; it can be done on its own.

@kadisi
Copy link
Member Author

kadisi commented Aug 12, 2021

@kadisi , as wenjun mentioned about the united-service feature request on last community meeting, the uniteddaemonset workload template will add ServiceTemplate support in future, right?

@zzguang yes

@kadisi
Copy link
Member Author

kadisi commented Aug 12, 2021

I can see the difference between UnitedDeamonset and UnitedDeployment as the former can automatically deploy workload when nodepool is created. The benefit is that we can could avoid manually changing UnitedDeployment had it been used for the same purpose. The naming maybe misleading, how about NodePoolDaemonSet?

@Fei-Guo good idea

@kadisi kadisi force-pushed the proposal_for_uniteddaemonset branch from 5a2210d to 6430a50 Compare August 18, 2021 02:22
@kadisi-bot
Proposal: UnitedDaemonSet
6d4f1f8 
Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>
@kadisi kadisi force-pushed the proposal_for_uniteddaemonset branch from 6430a50 to 6d4f1f8 Compare August 25, 2021 02:20
@kadisi kadisi mentioned this pull request Aug 25, 2021
Closed
@rambohe-ch rambohe-ch mentioned this pull request Aug 27, 2021
Closed
@kadisi kadisi changed the title Proposal: UnitedDaemonSet Proposal: YurtAppDaemob Sep 27, 2021
@rambohe-ch rambohe-ch changed the title Proposal: YurtAppDaemob Proposal: YurtAppDaemon Sep 27, 2021
rambohe-ch
rambohe-ch reviewed Sep 27, 2021
View reviewed changes
docs/enhancements/20210729-yurtappdaemon.md Outdated

- What is the plan for implementing this feature?

Provide yurtunit-manager operator to YurtAppDaemon CRD
Copy link
Member

Choose a reason for hiding this comment

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

yurtunit-manager --> yurt-app-manager?

rambohe-ch
rambohe-ch reviewed Sep 27, 2021
View reviewed changes
docs/enhancements/20210729-yurtappdaemon.md Outdated
## Implementation History

+ [ ] : YurtAppDaemon api crd
+ [ ] : yurtunit-manager YurtAppDaemon controller
Copy link
Member

Choose a reason for hiding this comment

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

yurtunit-manager --> yurt-app-manager?

rambohe-ch
rambohe-ch reviewed Sep 27, 2021
View reviewed changes
docs/enhancements/20210729-yurtappdaemon.md Outdated
WorkLoadProvisioned YurtAppDaemonConditionType = "WorkLoadProvisioned"
// WorkLoadUpdated means all the workload are updated.
WorkLoadUpdated YurtAppDaemonConditionType = "WorkLoadUpdated"
// WorkLoadFailure is added to a UnitedDeployment when one of its workload has failure during its own reconciling.
Copy link
Member

Choose a reason for hiding this comment

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

UnitedDeployment --> YurtAppManager?

rambohe-ch
rambohe-ch reviewed Sep 27, 2021
View reviewed changes
docs/enhancements/20210729-yurtappdaemon.md Outdated
// +kubebuilder:printcolumn:name="WorkloadTemplate",type="string",JSONPath=".status.templateType",description="The WorkloadTemplate Type."
// +kubebuilder:printcolumn:name="AGE",type="date",JSONPath=".metadata.creationTimestamp",description="CreationTimestamp is a timestamp representing the server time when this object was created. It is not guaranteed to be set in happens-before order across separate operations. Clients may not set this value. It is represented in RFC3339 form and is in UTC."

// YurtAppDaemon is the Schema for the uniteddeployments API
Copy link
Member

Choose a reason for hiding this comment

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

uniteddeployment --> yurtappdaemons?

rambohe-ch
rambohe-ch reviewed Sep 27, 2021
View reviewed changes
docs/enhancements/20210729-yurtappdaemon.md Outdated
Items []YurtAppDaemon `json:"items"`
}

// WorkloadTemplate defines the pool template under the UnitedDeployment.
Copy link
Member

Choose a reason for hiding this comment

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

UnitedDeployment --> YurtAppDaemon?

@kadisi kadisi force-pushed the proposal_for_uniteddaemonset branch from 1e4cedd to b5bdbbf Compare September 27, 2021 07:59
@DrmagicE
Copy link
Member

DrmagicE commented Sep 27, 2021 •
edited
Loading

How about adding patch feature to YurtAppDaemon, just like we did in YurtAppSet.

@kadisi-bot
change uniteddaemonset to yurtappdaemon
1616206 
Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>
@kadisi kadisi force-pushed the proposal_for_uniteddaemonset branch from b5bdbbf to 1616206 Compare September 27, 2021 09:18
@rambohe-ch
Copy link
Member

How about adding patch feature to YurtAppDaemon, just like we did in YurtAppSet.

@DrmagicE I think we'd better implement general YurtAppDaemon at first. and after that we can discuss whether add patch feature or not based on requirements from end users.

@DrmagicE
Copy link
Member

@rambohe-ch OK, It is reasonable.

@rambohe-ch
Copy link
Member

/lgtm
/approve

@openyurt-bot openyurt-bot added the lgtm lgtm label Sep 29, 2021
@openyurt-bot openyurt-bot merged commit aae1d9d into openyurtio:master Sep 29, 2021
YRXING pushed a commit to YRXING/openyurt that referenced this pull request Oct 9, 2021
@kadisi @kadisi-bot @YRXING
Proposal: YurtAppDaemon (openyurtio#422)
7d6345c 
* Proposal: UnitedDaemonSet

Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>

* change uniteddaemonset to yurtappdaemon

Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>

Co-authored-by: bingyu.zj <bingyu.zj@alibaba-inc.com>
@kadisi kadisi mentioned this pull request Nov 10, 2021
Merged
zzguang referenced this pull request in zzguang/openyurt Jan 23, 2022
add zzguang as reviewer of OpenYurt
c9d36c6 
Signed-off-by: zhenggu1 <zhengguang.zhang@intel.com>
@zzguang zzguang mentioned this pull request Jan 23, 2022
Merged
6 tasks
MrGirl pushed a commit to MrGirl/openyurt that referenced this pull request Mar 29, 2022
@kadisi @kadisi-bot
Proposal: YurtAppDaemon (openyurtio#422)
be05926 
* Proposal: UnitedDaemonSet

Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>

* change uniteddaemonset to yurtappdaemon

Signed-off-by: bingyu.zj <bingyu.zj@alibaba-inc.com>

Co-authored-by: bingyu.zj <bingyu.zj@alibaba-inc.com>
@zzguang zzguang mentioned this pull request Jul 15, 2022
Closed
6 tasks
@zzguang zzguang mentioned this pull request Jul 25, 2022
Merged
6 tasks
@zzguang zzguang mentioned this pull request Dec 12, 2022
Merged
@kadisi kadisi deleted the proposal_for_uniteddaemonset branch February 14, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rambohe-ch rambohe-ch rambohe-ch left review comments

@zzguang zzguang zzguang left review comments

@charleszheng44 charleszheng44 Awaiting requested review from charleszheng44

@Fei-Guo Fei-Guo Awaiting requested review from Fei-Guo

Assignees

@rambohe-ch rambohe-ch

Labels
approved approved kind/documentation kind/documentation kind/feature kind/feature lgtm lgtm size/L size/L: 100-499
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kadisi @openyurt-bot @rambohe-ch @gnunu @Fei-Guo @DrmagicE @zzguang @kadisi-bot