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

add strategy mitogen_linear when installed mitogen #5985

Merged
merged 9 commits into from
Apr 24, 2020
Merged

add strategy mitogen_linear when installed mitogen #5985

merged 9 commits into from
Apr 24, 2020

Conversation

LuckySB
Copy link
Contributor

@LuckySB LuckySB commented Apr 18, 2020

/kind feature

What this PR does / why we need it:

enable mitogen when installed via playbook

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LuckySB

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 18, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 20, 2020

/assign

EppO
EppO reviewed Apr 20, 2020
View reviewed changes
docs/mitogen.md Outdated Show resolved Hide resolved
@EppO
Copy link
Contributor

EppO commented Apr 20, 2020

I didn't see any differences on the CI jobs duration with these changes, is it expected?

@Miouge1
Copy link
Contributor

Miouge1 commented Apr 21, 2020

@EppO it's expected since there is no CI for mitogen. I'm wondering if we could use Mitogen in some (all?) CI jobs (maybe part1?) to accelerate things?

@Miouge1 Miouge1 added this to the 2.13 milestone Apr 21, 2020
@LuckySB LuckySB changed the title add strategy mitogen_linear when installed mitogen WIP add strategy mitogen_linear when installed mitogen Apr 21, 2020
@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 21, 2020
@LuckySB LuckySB changed the title WIP add strategy mitogen_linear when installed mitogen add strategy mitogen_linear when installed mitogen Apr 21, 2020
@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 Apr 21, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 22, 2020 •
edited
Loading

So I did some tests between this PR (a2f4eaa) and master (d7df577). All done using vagrant and the Kubespray Vagrantfile:

test master mitogen
cluster.yml with 5 instances 16min 20s 17min 38s
scale.yml from 5 to 10 instances 11min 25s 16min 05s

In those test conditions (Vagrant with libvirt driver, 2 different servers with each test in parallel), mitogen was a little slower :(

Not sure if/what I am doing wrong, but if we don't see a performance improvement (yet?) in CI, then I suggest to enable mitogen only on 1 job?

@LuckySB
Copy link
Contributor Author

LuckySB commented Apr 22, 2020

3 master + 3 worker node
play cluster.yml with mitogen (install cluster) - 17:26

try to play to avoid heavy time tasks on installed cluster:
play cluster.yml again with mitogen - 12:14
play cluster,yml without mitogen - 15:11

@LuckySB
Copy link
Contributor Author

LuckySB commented Apr 22, 2020

Not sure if/what I am doing wrong, but if we don't see a performance improvement (yet?) in CI, then I suggest to enable mitogen only on 1 job?

ok
what is job you whant run with mitogen ?

deploy1 packet_ubuntu18-calico-aio ?
or something from deploy2 stage ?

@Miouge1
Copy link
Contributor

Miouge1 commented Apr 22, 2020

Interesting! I didn't check on a retry of cluster.yml, a speed improvement there can be useful for people running cluster.yml in CI (infra as code) or changing some settings.

@LuckySB which ever job you think is most suitable.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Apr 23, 2020

After looking further in molecule, it looks like it helps when there is a latency between the ansible host and the inventory nodes. In my testing procedure that latency was minimal since running with vagrant, so that could explain why I didn't see any improvement.

@LuckySB
add small docs
97eac96 
Rename playbook file

The raw action executes as a regular Mitogen connection, which requires Python on the target, so add strategy: linear to bootstrap-os role playbook.
@LuckySB
add mitogen to CI test
4b10cfb 
fix typo
@LuckySB
enable mitogen test on deploy-part1 tests
2554838 
change version from master to release
download tar.gz archive
@LuckySB
run all CI tests with mitogen
60b2b06
@LuckySB
disable mitogen with upgrade CI tests
9a47bd5
@LuckySB
enable mitogen on CI tests via env vars
81696b5
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2020
@LuckySB
disable mitogen CI test on centos8
90dfe1e 
(get error  /usr/bin/python: No such file or directory)
@LuckySB
Copy link
Contributor Author

LuckySB commented Apr 24, 2020

Ansible 2.8 interpreter discovery and become plugins are not yet supported.
so disable mitogen on centos 8 ci.

todo: set ansible_python_interpreter=/usr/bin/python3 to CI test with mitogen and centos8

@Miouge1
Copy link
Contributor

Miouge1 commented Apr 24, 2020

I did another set of tests with 30ms between the Ansible host and the cluster:

  • No mitogen: 12min 27s
  • With mitogen: 10min 33s

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 69603ae into kubernetes-sigs:master Apr 24, 2020
@huxcrux huxcrux mentioned this pull request Apr 26, 2020
Merged
@Miouge1 Miouge1 mentioned this pull request Apr 27, 2020
Closed
LuckySB added a commit to southbridgeio/kubespray that referenced this pull request May 29, 2020
@LuckySB
add strategy mitogen_linear when installed mitogen (kubernetes-sigs#5985
3f17da1 
)

* add strategy mitogen_linear when installed mitogen

* add small docs

Rename playbook file

The raw action executes as a regular Mitogen connection, which requires Python on the target, so add strategy: linear to bootstrap-os role playbook.

* add mitogen to  CI test
fix typo

* enable mitogen test on deploy-part1 tests
change version from master to release
download tar.gz archive

* run all CI tests with mitogen

* disable mitogen with upgrade CI tests

* enable mitogen on CI tests via env vars

* disable mitogen on CI test by default, enable on some different OS

* disable mitogen CI test on centos8
(get error  /usr/bin/python: No such file or directory)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EppO EppO EppO left review comments

@bozzo bozzo Awaiting requested review from bozzo

@holmsten holmsten Awaiting requested review from holmsten

Assignees

@Miouge1 Miouge1

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/feature Categorizes issue or PR as related to a new feature. 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
2.13
Development

Successfully merging this pull request may close these issues.
4 participants
@LuckySB @k8s-ci-robot @Miouge1 @EppO