-
Notifications
You must be signed in to change notification settings - Fork 405
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
EdgeX integration with OpenYurt proposal #357
Conversation
@yixingjia: 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yixingjia 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 |
|
||
```go | ||
type EdgeX struct { | ||
//TODO |
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 think we need add selector of nodePool into EdgeX struct
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.
yes, Wuming is working on the EdgeX struct definition stuff.
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.
We also need to add the number of replicas for component that defined by CR resource in this struct.
|
||
![EdgeX Instance](../img/edgex/edgexinstance.png) | ||
As the EdgeX instance diagram shows, the following 8 Components are required for EdgeX Hanoi version | ||
- Core Data service: The core data micro service provides centralized persistence for data collected by devices. |
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.
Device Service components are forgotten?
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.
Device service is optional since different usage scenario will require different device service. But you reminder me that it should include the device controller :) as a required component.
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.
Device controller is yurt-device-controller? if Device controller is yurt-device-controller
, we should not deploy yurt-device-controller
with EdgeX components because some other iot system maybe integrated into yurt-device-controller
in the future.
@yixingjia please fix markdownlint error |
listKind: EdgeXList | ||
plural: edgexs | ||
singular: edgex | ||
scope: Cluster |
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 think the scope should be Namespaced
, so keep consistent with UnitedDeployment
type EdgeXSpec struct { | ||
Version string `json:"version,omitempty"` | ||
|
||
Pool PoolSpec `json:"pool,omitempty"` |
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.
how about use Pool
definition of UnitedDeployment
directly?
Spec appsv1.DeploymentSpec `json:"spec"` | ||
} | ||
|
||
// DeploymentTemplateSpec defines the pool template of Deployment. |
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.
ServiceTemplateSpec?
} | ||
|
||
// EdgeXStatus defines the observed state of EdgeX | ||
type EdgeXStatus struct { |
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.
What's the meaning of fields Initialized
, Reason
and Message
?
Spec appsv1.DeploymentSpec `json:"spec"` | ||
} | ||
|
||
type ComponetSpec struct { |
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.
typo error? componet --> component
|
||
``` | ||
|
||
|
||
```go | ||
type PoolSpec struct { |
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.
in UnitedDeployment definition, field Replicas
and Patch
is used for one kind Deployment, but in EdgeX there are several Deployments. I think it's not fitted to all deployments with same Replicas
and Patch
.
Deployment DeploymentTemplateSpec `json:"deploymentspec,omitempty"` | ||
|
||
// +optional | ||
Service ServiceTemplateSpec `json:"servicespec,omitempty"` |
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 think that all CRs for one kind Deployment will share the same service, so maybe it's not suitable to define Service in ComponentSpec
.
@yixingjia @lwmqwer please merge 3 commits into 1 commit. |
8f68ba5
to
9b89e72
Compare
/lgtm |
Signed-off-by: Yixing Jia<yixingjia@gmail.com>
Signed-off-by: Yixing Jia<yixingjia@gmail.com>
Signed-off-by: Yixing Jiayixingjia@gmail.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