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

[feature] implementation of nodegroup and edgeapplication controller #3719

Merged
merged 6 commits into from
Jun 15, 2022
Merged

[feature] implementation of nodegroup and edgeapplication controller #3719

merged 6 commits into from
Jun 15, 2022

Conversation

Congrool
Copy link
Member

@Congrool Congrool commented Mar 18, 2022 •
edited

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:
Implementation of feature at #3582.
This pr includes

  1. NodeGroup CRD types, yaml and generated code using code-generator
  2. NodeGroup controller

This controller is implemented through controller-runtime@v0.10.3, it will watch node and nodegroup resources and do the reconcilation as follows:

  1. check if this nodegroup has been deleted, if so, remove belonging-to label on all its contained nodes and remove the finalizer.
  2. check if this nodegroup has a finalizer, if not, add the finalizer to it.
  3. add belonging-to label to new added nodes
  4. remove belonging-to label on deleted nodes
  5. update status of nodegroup

node add/update/delete event will be translated to nodegroup event and reconcile as described above.

2022.4.6 CHANGELOG:

  1. add edgeapplication api and its generated code
  2. add edgeapplication controller, including status manager and overrider manager.
  • edgeapplication controller: main reconcile process
  • status manager: manage the status field of edgeapplication api
  • overrider manager: contains name overrider, replicas overrider and image overrider(from karmada), takes the responsibility of overriding the provided manifests.
  1. add controller manager cmd: a new cmd to start nodegroup controller and edgeapplication controller.

/cc @vincentgoat @fisherxu @lonelyCZ

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 18, 2022
@kubeedge-bot kubeedge-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 18, 2022
@kubeedge-bot
Copy link
Collaborator

@Congrool: GitHub didn't allow me to request PR reviews from the following users: vincentgoat, lonelyCZ.

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

In response to this:

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:
Implementation of feature at #3582.
This pr includes

  1. NodeGroup CRD types, yaml and generated code using code-generator
  2. NodeGroup controller

This controller is implemented through controller-runtime@v0.10.3, it will watch node and nodegroup resources and do the reconcilation as follows:

  1. check if this nodegroup has been deleted, if so, remove belonging-to label on all its contained nodes and remove the finalizer.
  2. check if this nodegroup has a finalizer, if not, add the finalizer to it.
  3. add belonging-to label to new added nodes
  4. remove belonging-to label on deleted nodes
  5. update status of nodegroup

node add/update/delete event will be translated to nodegroup and reconcile as described above.

/cc @vincentgoat @fisherxu @lonelyCZ

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.

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng after the PR has been reviewed.
You can assign the PR to them by writing /assign @kevin-wangzefeng 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:

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

@kubeedge-bot kubeedge-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 18, 2022
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2022
@Congrool Congrool force-pushed the nodegroup branch 5 times, most recently from 628a9f8 to 6c221b3 Compare March 25, 2022 08:21
@Congrool Congrool changed the title [feature] implementation of node group [feature] implementation of nodegroup and edgeapplication controller Apr 6, 2022
@Congrool Congrool force-pushed the nodegroup branch 4 times, most recently from cfeb283 to 785fb14 Compare April 6, 2022 07:34
@Congrool
Copy link
Member Author

Congrool commented Apr 7, 2022

It has not been tested.
/hold

@kubeedge-bot kubeedge-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2022
@Congrool Congrool force-pushed the nodegroup branch 3 times, most recently from 2948932 to 68e9382 Compare May 3, 2022 11:34
Congrool and others added 3 commits May 19, 2022 15:51
@Congrool @vincentgoat
update vendor for controller-runtime@v0.10.3
828d742 
Signed-off-by: Congrool <chpzhangyifei@qq.com>
Signed-off-by: Congrool <chpzhangyifei@zju.edu.cn>
@Congrool @vincentgoat
init nodegroup
1d9e20f 
Signed-off-by: Congrool <chpzhangyifei@qq.com>
Signed-off-by: Congrool <chpzhangyifei@zju.edu.cn>
@vincentgoat
add edgeapplication apis types
4b45fff 
Signed-off-by: vincentgoat <linguohui1@huawei.com>
@Congrool @vincentgoat
init edgeapplication
0c18cdb 
Signed-off-by: Congrool <chpzhangyifei@qq.com>
Signed-off-by: Congrool <chpzhangyifei@zju.edu.cn>
Congrool and others added 2 commits May 19, 2022 17:08
@Congrool
controller manager fix
60e540a 
Signed-off-by: Congrool <chpzhangyifei@qq.com>
Signed-off-by: Congrool <chpzhangyifei@zju.edu.cn>
@vincentgoat @Congrool
filter resource in dynamiccontroller
f094c55 
Signed-off-by: vincentgoat <linguohui1@huawei.com>
@vincentgoat
Copy link
Member

vincentgoat commented May 20, 2022 •
edited

Add:
TEST CASE

NODEGROUP/EDGEAPPLICATION BEST PRACTICE

@vincentgoat
Copy link
Member

It has not been tested. /hold

Already done test @Congrool

@Congrool
Copy link
Member Author

Thanks.

/unhold

@kubeedge-bot kubeedge-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2022
@vincentgoat vincentgoat mentioned this pull request May 26, 2022
Closed
fisherxu
fisherxu reviewed Jun 15, 2022
View reviewed changes
pkg/apis/apps/v1alpha1/nodegroup_types.go
// SucceededSelection represents that this node has been selected as a member of this NodeGroup.
SucceededSelection SelectionStatus = "Succeeded"
// FailedSelection represents that this node failed to become a member of this NodeGroup.
FailedSelection SelectionStatus = "Failed"
Copy link
Member

Choose a reason for hiding this comment

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

If the node hasn't been selected, it should not be included in the status?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, only nodes selected by node name or labels will have their status in the nodegroup, and the statuses of these nodes have two situations, including SucceededSelection and FailedSelection.

Nodes that not selected by the nodegroup will not have their status entries.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's the meaning of FailedSelection? The node in nodegroup, but select failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently there're two situations that will have FailedSelection:

  1. Use node name to select the node, but the node does not exist. In this situation, we should notify the users.
  2. Select a node that already belongs to another nodegroup. We should make sure that one node can only belong to one nodegroup, so when user want to select a node that already blongs to another nodegroup, due to negligence or something else, we also should notify the users.

Copy link
Member

Choose a reason for hiding this comment

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

Okey, sounds good, maybe we can add some comments later :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll add later.

fisherxu
fisherxu reviewed Jun 15, 2022
View reviewed changes
Copy link
Member

@fisherxu fisherxu left a comment •
edited

Choose a reason for hiding this comment

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

/lgtm

Looks good now, we can improve it in the future in practise.

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2022
@kevin-wangzefeng
Copy link
Member

/approve

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2022
@kubeedge-bot kubeedge-bot merged commit 436021c into kubeedge:master Jun 15, 2022
@Congrool Congrool mentioned this pull request Jun 27, 2022
Closed
7 tasks
@Congrool Congrool mentioned this pull request Jul 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisherxu fisherxu fisherxu left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Congrool @kubeedge-bot @vincentgoat @kevin-wangzefeng @fisherxu