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.

Sign up for GitHub

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

Jump to bottom

fix(nodeup): set MACAddressPolicy=none when using AWS VPC CNI #16313

Merged
merged 1 commit into from
Feb 3, 2024
Merged

fix(nodeup): set MACAddressPolicy=none when using AWS VPC CNI #16313

merged 1 commit into from
Feb 3, 2024

Conversation

moshevayner
Copy link
Member

@moshevayner moshevayner commented Feb 2, 2024 •
edited
Loading

What this PR does / why we need it:
Following suggestion in aws/amazon-vpc-cni-k8s#2103 to get Amazon VPC to work on Ubuntu 22.04

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
fixes #16255
fixes #15720

Special notes for your reviewer:
/cc @hakman

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 2, 2024
@k8s-ci-robot k8s-ci-robot requested a review from zetaab February 2, 2024 17:13
moshevayner
moshevayner commented Feb 2, 2024
View reviewed changes
Copy link
Member Author

@moshevayner moshevayner left a comment

Choose a reason for hiding this comment

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

/test all

@hakman
Copy link
Member

hakman commented Feb 2, 2024

/test pull-kops-e2e-cni-amazonvpc-u2204

@moshevayner moshevayner marked this pull request as ready for review February 2, 2024 18:28
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@moshevayner
Copy link
Member Author

/cc @hakman

@k8s-ci-robot k8s-ci-robot requested a review from hakman February 2, 2024 19:27
hakman
hakman requested changes Feb 3, 2024
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

This is great progress. Thanks a lot @moshevayner!
Some minor issues, but it otherwise looks like this will fix AWS VPC CNI.

nodeup/pkg/model/sysctls.go Outdated Show resolved Hide resolved
nodeup/pkg/model/sysctls.go Outdated Show resolved Hide resolved
nodeup/pkg/model/sysctls.go Outdated Show resolved Hide resolved
@hakman hakman changed the title fix(nodeup): set MACAddressPolicy to none when using AWS CNI and Ubuntu 22.04 fix(nodeup): set MACAddressPolicy to none when using AWS VPC CNI and Ubuntu 22.04 Feb 3, 2024
@hakman hakman changed the title fix(nodeup): set MACAddressPolicy to none when using AWS VPC CNI and Ubuntu 22.04 fix(nodeup): set MACAddressPolicy=none when using AWS VPC CNI Feb 3, 2024
@moshevayner moshevayner force-pushed the aws-vpc-cni-ubuntu-22-04-macaddresspolicy branch 2 times, most recently from 4d827e2 to 22d6104 Compare February 3, 2024 03:45
@moshevayner
Copy link
Member Author

/test pull-kops-e2e-cni-amazonvpc-u2204

@moshevayner moshevayner requested a review from hakman February 3, 2024 03:51
@moshevayner
Copy link
Member Author

@hakman thanks for your comments/suggestions!
I believe I've addressed all of them (hopefully I didn't miss anything 😬 ), and force-pushed my branch after those changes to keep things tidy.
I also kicked off the e2e test for Ubuntu 22.04 with amazon vpc eni to make sure it passes.
LMK WDYT 🙏🏼

hakman
hakman reviewed Feb 3, 2024
View reviewed changes
nodeup/pkg/model/networking/amazon-vpc-routed-eni.go Outdated
Comment on lines 38 to 39
// Running Amazon VPC CNI on Ubuntu 22.04 and later requires setting MACAddressPolicy to `none` (ref: https://github.com/aws/amazon-vpc-cni-k8s/issues/2103 & https://github.com/kubernetes/kops/issues/16255)
if b.NodeupConfig.Networking.AmazonVPC != nil && b.Distribution.IsUbuntu() && b.Distribution.Version() >= 22.04 {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Running Amazon VPC CNI on Ubuntu 22.04 and later requires setting MACAddressPolicy to `none` (ref: https://github.com/aws/amazon-vpc-cni-k8s/issues/2103 & https://github.com/kubernetes/kops/issues/16255)
if b.NodeupConfig.Networking.AmazonVPC != nil && b.Distribution.IsUbuntu() && b.Distribution.Version() >= 22.04 {
// Running on Ubuntu 22.04 and later requires setting MACAddressPolicy=none`
// Ref: https://github.com/aws/amazon-vpc-cni-k8s/issues/2103 & https://github.com/kubernetes/kops/issues/16255
This should already be handled above
```suggestion
if b.Distribution.IsUbuntu() && b.Distribution.Version() >= 22.04 {

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh right, good point. Thanks for catching that!

@hakman
Copy link
Member

hakman commented Feb 3, 2024

@moshevayner One small nit. Thanks for updating so fast.

@moshevayner moshevayner force-pushed the aws-vpc-cni-ubuntu-22-04-macaddresspolicy branch from 22d6104 to 1342fd1 Compare February 3, 2024 04:03
@moshevayner
Copy link
Member Author

/test pull-kops-e2e-cni-amazonvpc-u2204

@moshevayner moshevayner requested a review from hakman February 3, 2024 04:05
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit 75059db into kubernetes:master Feb 3, 2024
22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Feb 3, 2024
@moshevayner moshevayner deleted the aws-vpc-cni-ubuntu-22-04-macaddresspolicy branch February 3, 2024 05:09
@hakman hakman mentioned this pull request Feb 3, 2024
Merged
k8s-ci-robot added a commit that referenced this pull request Feb 3, 2024
@k8s-ci-robot
Merge pull request #16319 from hakman/automated-cherry-pick-of-#16313…
f8ba7d5 
…-upstream-release-1.28

Automated cherry pick of #16313: fix(nodeup): set `MACAddressPolicy` to `none` when using AWS
moshevayner added a commit to moshevayner/kops that referenced this pull request Feb 5, 2024
@moshevayner
docs: remove warning about aws vpc cni not being compatible with Ubun…
1b5e6dd 
…tu 22.04, following kubernetes#16313
@moshevayner moshevayner mentioned this pull request Feb 5, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hakman hakman hakman approved these changes

@johngmyers johngmyers Awaiting requested review from johngmyers

@zetaab zetaab Awaiting requested review from zetaab

Assignees

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

AWS VPC CNI Ubuntu 22.04 MACAddressPolicy amazonvpc is not working with Ubuntu 22.04(Jammy)
3 participants
@moshevayner @k8s-ci-robot @hakman