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.

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

Add condition to skip processing for externally managed VMIs #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattmattox
Copy link

This is to address harvester/harvester#6305

TLDR; It adds support for the label harvesterhci.io/external-managed-node=true to force the cloud provider to skip checking that node.

@mattmattox mattmattox changed the title "Add condition to skip processing for externally managed VMIs" Add condition to skip processing for externally managed VMIs Aug 9, 2024
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, please confirm the question, thanks.

@@ -75,6 +76,15 @@ func (h *Handler) OnVmiChanged(_ string, vmi *kubevirtv1.VirtualMachineInstance)
return vmi, nil
}

// Skip processing if the external-managed-node label is present
Copy link
Member

Choose a reason for hiding this comment

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

@mattmattox Thanks for your committing and sorry for the later review, as no one was assigned to review the PR.

The PR looks good, and I only have this question:

L88 has the check if creator := vmi.Labels[builder.LabelKeyVirtualMachineCreator]; creator != virtualmachine.VirtualMachineCreatorNodeDriver to skip the VM.

If you add external VMIs to the cluster, does the related VMI still happens to have LabelKeyVirtualMachineCreator label? could you utilize this existing label to skip it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants