-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: get node name from guest agent #38
Conversation
Signed-off-by: PoAn Yang <poan.yang@suse.com>
27a112d
to
447fef2
Compare
if _, err := i.vmClient.Get(i.namespace, node.Name, metav1.GetOptions{}); err != nil && !errors.IsNotFound(err) { | ||
return false, err | ||
} else if errors.IsNotFound(err) { | ||
if _, err := i.getVM(node); err != nil { |
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.
The original method was using vmClient.Get
to get data from remote instead of from cache; now the getVM
is checking data from sync.Map
, it will have such race:
when the sync.Map
is not synced fully in OnVmiChanged
; the InstanceExists
will not work as expected.
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.
Good catch. Reverted vmCache/vmiCache to vmClient/vmiClient. Thanks.
447fef2
to
fbbcb3f
Compare
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.
Another question about the map, please double check, thanks.
"hostname": guestAgentInfo.Hostname, | ||
}).Info("get agent info success, using hostname as node name") | ||
nodeName = guestAgentInfo.Hostname | ||
h.nodeToVMName.Store(nodeName, vmi.Name) |
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.
The vmi.Namespace
is not stored in the map, if VMs from different namespace have same name, will them overwrite each other?
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 better way is to drop events from other namespaces because the cloud provider is running in the guest cluster. It only needs to care VMI in that namespace. We can get the namespace like instance manager. WDYT?
cloud-provider-harvester/pkg/cloud-controller-manager/ccm.go
Lines 85 to 89 in 9daecd1
cp.instances = &instanceManager{ | |
vmClient: cp.kubevirtFactory.Kubevirt().V1().VirtualMachine(), | |
vmiClient: cp.kubevirtFactory.Kubevirt().V1().VirtualMachineInstance(), | |
namespace: namespace, | |
} |
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.
It is reasonable to drop other namespaces.
Signed-off-by: PoAn Yang <poan.yang@suse.com>
fbbcb3f
to
e698f84
Compare
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.
LGTM, thanks.
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.
lgtm.
if the change is not compatible with Harvester v1.2.x, be sure to bump the minor version when doing a new tag. (like from v0.2.0 -> v0.3.0).
Hi @w13915984028, I think this is not a breaking change, so I will create a new tag v0.2.1. |
harvester/harvester#4947
Test steps
cluster_name
is guest cluster name which we will create later. Thenamespace
is guest cluster VM namespace.External
cloud provider.write_files
from the step 4 to User Data. Also, setting ahostname
.waiting for cluster agent to connect
.ssh
to login the VM and installhelm
.