bugfix: device model sync #5065
Conversation
kubeedge-bot
added
kind/bug
|
@fisherxu: GitHub didn't allow me to assign the following users: wbc6080. Note that only kubeedge 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. |
|
Please make the pipeline requested checks successful |
Signed-off-by: cl2017 <chenlin.liu@daocloud.io>
| if deviceItem.Spec.NodeName == "" { | ||
| return true | ||
| } | ||
| if deviceItem.Spec.NodeName == targetNode && deviceItem.Spec.DeviceModelRef.Name == modelName { |
There was a problem hiding this comment.
Because the device info has been added to deviceMap at L125, so this range will always compare will itself, and set res to true? @cl2017
There was a problem hiding this comment.
Because the device info has been added to
deviceMapat L125, so this range will always compare will itself, and set res to true? @cl2017
This logic seems to expect that the same device model will be reused at the same node, right? @cl2017
So the res is not always true, it returns a false value when the device model first binds the node.
There was a problem hiding this comment.
In L179, If the currently index device in the map is the device just added to the map, we will directly skip the check of this index device. Therefore, this range will not be affected. Is it right? @cl2017
There was a problem hiding this comment.
Because the device info has been added to
deviceMapat L125, so this range will always compare will itself, and set res to true? @cl2017This logic seems to expect that the same device model will be reused at the same node, right? @cl2017 So the res is not always true, it returns a false value when the device model first binds the node.
Yes, this function is used to find wether the same devicemodel already exists at the targetNode.
At this fuction, we use res variable as a result, rather than the return value in Range method, return true means continue Range function.
@fisherxu @WillardHu @wbc6080
There was a problem hiding this comment.
In L179, If the currently index device in the map is the device just added to the map, we will directly skip the check of this index device. Therefore, this range will not be affected. Is it right? @cl2017
Make sense, if device name == k, it means this is itself. Then return. Right? @wbc6080
And another question, only compare name maybe not enough, it can be same with device in another namespace.. ptal @cl2017
There was a problem hiding this comment.
Well,this is indeed a problem. I also ignored that device instance and device model are namespace level resources.This may also cause another problem. If two namespaces have a model with the same name, the model in namespace B may interfere with the model referenced by the device in namespace A during range. @fisherxu @cl2017
There was a problem hiding this comment.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fisherxu 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 |
kubeedge-bot
added
the
approved
|
/lgtm |
|
I have open an issue with #5220 and cherry pick to release-1.15 with #5221 @cl2017 @WillardHu @fisherxu |
…upstream-release-1.15 Automated cherry pick of #5065: bugfix: device model sync
What type of PR is this?
/kind bug
What this PR does / why we need it:
bugfix: device model sync.
When added or deleted device, need to determine whether the device model needs to be sync to edgecore.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: