Proposal: OpenYurt Convertor Operator for converting K8S to OpenYurt #389
Conversation
|
@gnunu: 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. In response to this:
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. |
|
/assign @yixingjia |
|
/kind feature |
|
/assign @rambohe-ch |
|
|
||
| The controller would listen incomming CRs, and analyze the requirements to figure out user's intention, that is, what nodes to convert, and what nodes to revert. | ||
|
|
||
| The controller would maintain a ConfigMap to record conversion/reversion rules, for esay recovery from issues and maintaining idempotence. |
There was a problem hiding this comment.
Can you explain the details about the rules saved in the ConfigMap?
| CRD's definition: | ||
|
|
||
| // YurtconvertorSpec defines the desired state of Yurtconvertor | ||
| type YurtconvertorSpec struct { |
There was a problem hiding this comment.
The "YurtConvertor" does not sound like a CRD name for declaring a "state". How about "YurtCluster"?
The CRD SPEC should include CloudNodes[] and EdgeNodes[]. Convert and Revert are actions, not states. The controller should figure out the nodes that need to be reverted, hence we don't need the Revert array in the Spec.
To describe the Cloud/Edge Node lists, we can try the following struct
type YurtClusterSpec struct{
// optional
CloudNodes ItemList
// optional
EdgeNodes ItemList
TunnelService ComponentConfig
AppManager ComponentConfig
}
// You may consider different configs for Tunnel and AppManager separately to
// allow customization for each component
type ComponentConfig{
Enable bool
Version string
}
type ItemList {
//optional
Names []string
//optional
Pattern string // For regular expression
//optional
ExcludedNames []string
//optional
ExcludedPattern string
}
It seems that user only needs to specify either CloudNodes or EdgeNodes, not both.
Please clarify this point in the Spec comment if it is true.
| Reverted []string `json:"reverted,omitempty"` | ||
| Excepted []string `json:"excepted,omitempty"` | ||
| Failed []string `json:"failed,omitempty"` | ||
| } |
There was a problem hiding this comment.
Please update the Status based on the SPEC modifications suggested above accordingly.
For failed nodes, the names are not enough. We need to expose the error msgs in the Status as well.
|
Also，please think about the following questions:
|
|
Hi Fei, |
|
If the cluster nodes have multiple different deployment methods, what will happen at this time? E.g. kubeadm binary. And does it mean that there will be two sets of code for convert/revert? |
I think the converted effect is added necessary changes to the node to be an OpenYurt node. If nodes are added through different methods, maybe there would be specific code for each case. This should be transparent to user. The Operator needs do all dirty works necessary. :) |
|
Hi @Fei-Guo,
Please see if anything is missing. Thanks! |
|
/assign @rambohe-ch |
|
the refinement link is here: |
|
sorry, one more refinement: use camelCase for json tags: |
|
Please fix markdownlint error. Overall, LG. I don't worry the performance too much since the node components installation can be done in parallel. You probably need to maintain an internal state machine to track the covert progress for each node to deal with frequent switches between edge mode and cloud mode for any reason. @rambohe-ch Please take a look as well. |
Thanks Fei!
|
| ### Goals | ||
| * Convert nodes automatically | ||
| * Revert nodes automatically | ||
| * Translated OpenYurt cluster should be the same as through Yurtcutl or doing manually |
|
|
||
| CRD's definition: | ||
|
|
||
| type ComponentConfig struct { |
There was a problem hiding this comment.
please define all of structs in code block(please use ```), so definition will be more readable
| YurtTunnelVersion ComponentConfig `json:"yurtTunnelVersion,omitempty"` | ||
| AppManagerVersion ComponentConfig `json:"appMangerVersion,omitempty"` | ||
| ExcludedCloudNodes []string `json:"excludedCloudNodes,omitempty"` | ||
| ExcludedEdgeNodes []string `json:"excludedEdgeNodes,omitempty"` |
There was a problem hiding this comment.
How about merge ExcludedCloudNodes and ExcludedEdgeNodes as ExcludedNodes filed? because both fields maybe contain same excluded nodes.
| } | ||
|
|
||
| // YurtClusterSpec defines the desired state of YurtCluster | ||
| type YurtClusterSpec struct { |
There was a problem hiding this comment.
User maybe want to define the version of yurthub and yurt-controller-manager component, how about add fields to define yurthub and yurt-controller-manager?
|
@gnunu please merge all of six commits into one commit. |
Hi @rambohe-ch, thanks, have made revision on your suggestions. will merge all the commits nto one. |
|
I created a new pull request, for my bad not created a branch in the first place, sorry! |
Signed-off-by: nunu <shaoqiang.chen@intel.com>
|
just pushed the merged proposal. thanks @rambohe-ch for guide to do this! |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnunu, rambohe-ch 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 |
…rtio#389) Signed-off-by: nunu <shaoqiang.chen@intel.com>
Signed-off-by: nunu shaoqiang.chen@intel.com
What type of PR is this?
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