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

Mitogen: deprecate the use of mitogen and remove coverage from CI #8147

Merged

Conversation

cristicalin
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Mitogen upsteam is not seeing any new releases and our continued support for it hinders adoption of newer Ansible versions that would in turn give us support for newer Enterprise Linux versions like Rocky. This PR deprecates the use of mitogen in our CI and explicitly marks it as deprecated in the docs. The install playbook as well as the 3 workarounds are still left in place and I plan to remove them after we tag 2.18.

Related discussion in #7887 and #8125

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Mitogen: support for the mitogen playbook accelerator is now deprecated in preparation of ansible upgrades, please clean up your playbooks that depend on it.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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 Oct 31, 2021
@oomichi
Copy link
Contributor

oomichi commented Nov 1, 2021

The reason is written well on the pull request message, thanks for doing this.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2021
docs/mitogen.md Outdated
@@ -1,5 +1,7 @@
# Mitogen

*Warning:* Mitogen support is now deprecated in kubespray due to upstream not releasing an updated version to support ansible 4.x (ansible-base 2.11.x) and above. The CI support has been stripped for mitogen and we are no longer validating any support or regressions for it. The supported playbook and documentation will be removed in a kubespray 2.19.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe don't be that specific ? The supported playbook and documentation will be removed in a future kubespray release for example ? WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted the message to be loud and clear but yes, it's better to temper it down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the wording, also better English language, but kept the warning note.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2021
@oomichi
Copy link
Contributor

oomichi commented Nov 4, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2021
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

Thanks and agreed with Mitogen becoming harder to support as no real activity on their end 😢

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut

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 Nov 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8d553f7 into kubernetes-sigs:master Nov 5, 2021
@Payback159
Copy link
Contributor

Hi guys, I understand that maintenance is more difficult to impossible if nothing happens on the part of upstream mitogen but I am concerned that this will slow down the execution time of Kubespray.

How do you guys see this or maybe you have already tested a run without mitogen?

@cristicalin
Copy link
Contributor Author

@Payback159 see #7887 (comment) for some comparisons between ansible with ssh pipelining (current kubespray default) and mitogen, in our tests mitogen with modern ansible is actually slower.

That said, the current deprecation does not remove the capability to use mitogen and if you source kubespray in your own playbooks you can always re-enable mitogen and submit issues and/or fixes to kubespray if you encounter issues.

Secondly, I see there was a recent merge in mitogen master branch to make it work with modern ansible and should a new tag be added and released, we will happily update it in our playbooks and give up on the plan to fully drop it.

To putting it back in CI, we expect mitogen upstream to have a firm commitment to maintain their project or have someone more willing to fork and maintain it.

@floryut floryut mentioned this pull request Dec 21, 2021
@maxpain
Copy link
Contributor

maxpain commented Jan 7, 2022

in our tests mitogen with modern ansible is actually slower.

In our case, upgrade-cluster.yml on a cluster with 2 nodes without mitogen takes 1 hour 20 minutes.
With mitogen about 20 minutes.

The difference is noticeable when you have a high latency between ansible host and target nodes, in my case I have latency 270ms (from Russia, Moscow to Buenos Aires, Argentina).

@maxpain
Copy link
Contributor

maxpain commented Jan 12, 2022

By the way, Mitogen 0.3.1 release is here with Ansible 3, 4 and 5 support.

Is it possible to roll back this MR?

@cristicalin
Copy link
Contributor Author

@maxpain this PR disables the CI coverage so it does not interfere with you deploying mitogen via the embedded mitogen.yml playbook. The only thing you need to continue using mitogen is to modify ansible.cfg according to the docs.

I'm hesitant to roll this change back since a minor release is not quite proof of long term commitment. We should continue to keep an eye out for mitogen evolution and reconsider adding CI coverage back after it has proven its capability to keep up to date with changes.

I think we can still merge fixes for corner cases tripped by the use of mitogen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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
Development

Successfully merging this pull request may close these issues.

6 participants