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

Backport of #9543: Remove the checksum workaround for Flannel VXLAN #9548

Closed

Conversation

hakman
Copy link
Member

@hakman hakman commented Jul 10, 2020

Backports: #9543
Reverts: #9074

@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jul 10, 2020
@k8s-ci-robot k8s-ci-robot added 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 Jul 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hakman
To complete the pull request process, please assign mikesplain
You can assign the PR to them by writing /assign @mikesplain in a comment when ready.

The full list of commands accepted by this bot can be found 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

@hakman
Copy link
Member Author

hakman commented Jul 10, 2020

/assign @KashifSaadat

@johngmyers
Copy link
Member

I'm concerned with doing this so close to 1.18 release. And it looks like the backports of kubernetes/kubernetes#92035 haven't made their way to stable releases?

@hakman
Copy link
Member Author

hakman commented Jul 10, 2020

Next wave of patch release is planned for 2020-07-15.
I think it is reasonable that, together with the release notes mentioning the patch versions, this will not be a problem.

@johngmyers
Copy link
Member

It will be some time after 07-15 before those versions are in the stable channel. I don't think we should hold the kops 1.18 release for this.

@hakman
Copy link
Member Author

hakman commented Jul 10, 2020

I didn't want to hold the release for this. Let me ask it this way, would you rather close the PR or hold for 1.18.1. Either way is good for me. I just want to remove it from master.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@rifelpet
Copy link
Member

Maybe its worth asking "What is the downside of not removing this workaround in 1.18 (or 1.18.0)?

I think we should wait for the new k8s releases to be in the stable channel and for the removal to bake in master for a while so we get clear CI signal before merging the cherry-picks. Depending on the answer to my question, that would either mean having it potentially target 1.18.1 or delaying the 1.18.0 release.

@hakman
Copy link
Member Author

hakman commented Jul 10, 2020

The workaround means disabling hardware TX checksums, so more work for the CPU. I doubt most people would notice it though.

@justinsb
Copy link
Member

Hmmm ... this is indeed a tough one. I now see the appeal of changing the behaviour on the kubernetes patch version, though this isn't something we've done before to my knowledge. It does seem odd to have the workaround when it's not needed within days of the kops release, but most of the k8s 1.18 series has the bug where it is needed. I do also agree that most people won't notice the CPU hit.

I'm inclined right now to say that we don't backport to 1.18 either, but I very much agree with first evaluating it on master!

@hakman
Copy link
Member Author

hakman commented Jul 17, 2020

At the moment there isn't a strong reason to add this change to 1.18.
/close

@k8s-ci-robot
Copy link
Contributor

@hakman: Closed this PR.

In response to this:

At the moment there isn't a strong reason to add this change to 1.18.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@hakman hakman deleted the remove-flannel-workaround-2 branch July 17, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants