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

Ignore hybrid-overlay nodes from EgressIP controller #4093

Merged
merged 1 commit into from Jan 19, 2024

Conversation

kyrtapz
Copy link
Contributor

@kyrtapz kyrtapz commented Jan 16, 2024

EgressIP is not supported on hybrid-overlay nodes.
Exclude hybrid-overlay nodes from EgressIP controller to avoid issues related to missing annotations.

/cc @martinkennelly

EgressIP is not supported on hybrid-overlay nodes.
Exclude hybrid-overlay nodes from EgressIP controller to avoid
issues related to missing annotations.

Signed-off-by: Patryk Diak <pdiak@redhat.com>
@tssurya
Copy link
Member

tssurya commented Jan 18, 2024

/assign @tssurya
I will look at this this week.

Copy link
Member

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

thanks @kyrtapz for the PR, Overall lgtm, left some inline questions.

@@ -80,6 +85,12 @@ func (h *egressIPClusterControllerEventHandler) UpdateResource(oldObj, newObj in
case factory.EgressNodeType:
oldNode := oldObj.(*v1.Node)
newNode := newObj.(*v1.Node)

// EgressIP is not supported on hybrid overlay nodes
if util.NoHostSubnet(newNode) {
Copy link
Member

Choose a reason for hiding this comment

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

do we need to handle case where old node == linux and new node == windows so we need to tear down the EIP configs? asking because I came across aeb9817 and assuming it is needed somehow for live migration? Seems like nodes can switch between HO and not? Wondering if that logic needs to be considered for EIPs or we stick to the this is not supported script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EgressIP is not currently expected to work during live migration. The feature should still work after the migration is completed and the changes in the PR don't affect that.
Handling nodes moving to HO for egressIP requires more effort, especially configuring the appropriate OVN flows etc.

Copy link
Member

Choose a reason for hiding this comment

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

ack agreed as of now that this is not in this PR's scope then.

Copy link
Member

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

LGTM.
little blind here since we don't have e2e's for EIP+Windows but the fix seems non-risky and straight forward. Gonna merge this.

@tssurya tssurya merged commit e7acee6 into ovn-org:master Jan 19, 2024
30 checks passed
@tssurya tssurya self-assigned this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants