Skip to content

Conversation

@paulyufan2
Copy link
Contributor

@paulyufan2 paulyufan2 commented Sep 23, 2024

Reason for Change:

This PR is to fix one issue in swiftv2 Windows scenario:
When a pod is created, the default route is added on infra vnet:
root@swiftv2-pod-3:/# ip route
default via 10.244.2.1 dev eth0 metric 1
It leads to ping a VM IP in the same VNET that cannot work.

There are two issues::
1.CNS does not provide the default route to CNI;
2.CNI should only add the default route to secondary interface customer vnet; on Swiftv2 scenario, skipDefaultRoutes is set to true for infraNIC interface and false for a secondary interface; so if !info.skipDefaultRoutes, then add default route.

Issue Fixed:

Requirements:

Notes:

@paulyufan2 paulyufan2 added cni Related to CNI. fix Fixes something. labels Sep 23, 2024
@paulyufan2 paulyufan2 marked this pull request as ready for review September 23, 2024 16:02
@paulyufan2 paulyufan2 requested a review from a team as a code owner September 23, 2024 16:02
@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch from 39e5a07 to ab9e2c7 Compare September 23, 2024 16:02
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

how it works for linux not windows? why we need this change specifically for windows. also feel this will regress linux swiftv2

@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch from ab9e2c7 to c0ef702 Compare September 24, 2024 14:42
@paulyufan2 paulyufan2 requested a review from a team as a code owner September 24, 2024 19:31
@paulyufan2 paulyufan2 requested a review from a team as a code owner September 24, 2024 22:01
@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch from 9fe4da9 to b14386f Compare September 25, 2024 01:16
@paulyufan2
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@paulyufan2 paulyufan2 changed the title fix: add default route on customer vnets fix: add default route on customer vnets and provide default route from CNS to CNI Sep 26, 2024
@paulyufan2 paulyufan2 added the cns Related to CNS. label Sep 26, 2024
@paulyufan2
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


// Create the HCN endpoint.
logger.Info("Creating hcn endpoint", zap.Any("hcnEndpoint", hcnEndpoint), zap.String("computenetwork", hcnEndpoint.HostComputeNetwork))
logger.Info("Creating hcn endpoint", zap.Any("hcnEndpoint", hcnEndpoint), zap.String("computenetwork", hcnEndpoint.HostComputeNetwork), zap.Any("routes", hcnEndpoint.Routes))
Copy link
Member

Choose a reason for hiding this comment

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

can skip logging routes if hcnendpoint object already logs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zap log does not log Routes field from hcnEndpoint

@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch 3 times, most recently from 24dc6b2 to 43cd9da Compare October 15, 2024 16:24
@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch 4 times, most recently from a14aeff to 2a7940f Compare November 4, 2024 15:33
@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch from 6a020d2 to 9f20666 Compare November 4, 2024 23:44
@github-actions
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Nov 19, 2024
@paulyufan2
Copy link
Contributor Author

will use another PR for long-term solution

@paulyufan2 paulyufan2 closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cni Related to CNI. cns Related to CNS. fix Fixes something. stale Stale due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants