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

move node ip save farther down stack #8168

Merged
merged 8 commits into from
May 20, 2020
Merged

move node ip save farther down stack #8168

merged 8 commits into from
May 20, 2020

Conversation

sharifelgamal
Copy link
Collaborator

We need to capture the new IP address for a node as soon as it saved so that anything calling it will automatically get the new IP.

@sharifelgamal sharifelgamal requested a review from medyagh May 16, 2020 00:11
@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 May 16, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sharifelgamal

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 May 16, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2020
@sharifelgamal
Copy link
Collaborator Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 18, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
docker Driver

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [65.25947209099999 64.94961483199998 65.87454]
Average time for minikube: 65.36120897433334

Times for Minikube (PR 8168): [63.552341406 66.457912779 65.29785197499999]
Average time for Minikube (PR 8168): 65.10270205333335

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8168) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.067104 |           0.072335 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.021865 |           0.023017 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.003565 |           0.004575 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 40.020110 |          39.959794 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 22.991591 |          23.104133 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.468302 |           1.541310 |
| components...                  |           |                    |
| * Enabled addons:              |  0.705662 |           0.312591 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.079619 |           0.076243 |
| configured to use "minikube"   |           |                    |
|                                |  0.003391 |           0.008706 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [30.136694082 29.388690302999994 28.966837778]
Average time for minikube: 29.497407387666666

Times for Minikube (PR 8168): [28.744829417000002 29.854167033 31.333961858999995]
Average time for Minikube (PR 8168): 29.977652769666665

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8168) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.079642 |           0.085671 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.003473 |           0.003368 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.066254 |           0.067706 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  8.177622 |           8.397985 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.154314 |           0.133183 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 19.659366 |          20.224777 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  1.268687 |           0.988610 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.003366 |           0.003416 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.081017 |           0.067803 |
| configured to use "minikube"           |           |                    |
|                                        |  0.003666 |           0.005134 |
+----------------------------------------+-----------+--------------------+

@priyawadhwa priyawadhwa mentioned this pull request May 18, 2020
Closed
@kubernetes kubernetes deleted a comment from TravisBuddy May 19, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [75.786310529 69.166820688 69.94986407500001]
Average time for minikube: 71.63433176400001

Times for Minikube (PR 8168): [67.063088094 67.491121267 64.729216426]
Average time for Minikube (PR 8168): 66.42780859566666

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8168) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.073215 |           0.069009 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.024149 |           0.023808 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.014700 |           0.003898 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 44.565176 |          41.210394 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 24.170986 |          23.028664 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.908670 |           1.622340 |
| components...                  |           |                    |
| * Enabled addons:              |  0.788335 |           0.383239 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.084540 |           0.080105 |
| configured to use "minikube"   |           |                    |
|                                |  0.004562 |           0.006352 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [29.653383554 27.501074412 27.525249131000002]
Average time for minikube: 28.226569032333334

Times for Minikube (PR 8168): [29.509745304999996 28.959007016 28.755515646000003]
Average time for Minikube (PR 8168): 29.074755989

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8168) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.078748 |           0.078894 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.003059 |           0.003154 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.065443 |           0.063203 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  8.213775 |           8.513501 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.132321 |           0.135974 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 18.599516 |          18.994031 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  1.009079 |           1.208296 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.055040 |           0.003624 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.066283 |           0.067880 |
| configured to use "minikube"           |           |                    |
|                                        |  0.003304 |           0.006201 |
+----------------------------------------+-----------+--------------------+

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #8168 into master will increase coverage by 0.01%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8168      +/-   ##
==========================================
+ Coverage   34.51%   34.52%   +0.01%     
==========================================
  Files         147      147              
  Lines        9400     9408       +8     
==========================================
+ Hits         3244     3248       +4     
- Misses       5759     5761       +2     
- Partials      397      399       +2
Impacted Files Coverage Δ
pkg/minikube/machine/machine.go 7.40% <50.00%> (+7.40%) ⬆️
pkg/minikube/machine/fix.go 38.31% <55.55%> (ø)
pkg/minikube/machine/start.go 62.67% <66.66%> (ø)

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [72.13940130099999 72.28343812600001 72.73660143000001]
Average time for minikube: 72.38648028566666

Times for Minikube (PR 8168): [67.42977709600001 70.62731533600001 72.08818329]
Average time for Minikube (PR 8168): 70.04842524066667

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8168) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.082802 |           0.071029 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.025401 |           0.023238 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.004323 |           0.004284 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 45.404613 |          42.344687 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 24.425276 |          25.213695 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.591088 |           1.713972 |
| components...                  |           |                    |
| * Enabled addons:              |  0.765554 |           0.584615 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.083217 |           0.084808 |
| configured to use "minikube"   |           |                    |
|                                |  0.004206 |           0.008097 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [30.99538899 30.415502318999998 30.952856432]
Average time for minikube: 30.787915913666666

Times for Minikube (PR 8168): [30.48519997 30.725132732 28.921184596000003]
Average time for Minikube (PR 8168): 30.04383909933333

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8168) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.087697 |           0.091868 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.003428 |           0.003990 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.068331 |           0.072715 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  8.893943 |           8.974842 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.171409 |           0.140886 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 20.421327 |          19.607501 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  1.053158 |           1.002834 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.006493 |           0.069128 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.076127 |           0.073448 |
| configured to use "minikube"           |           |                    |
|                                        |  0.006004 |           0.006627 |
+----------------------------------------+-----------+--------------------+

@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 19, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 19, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [65.61158228500001 74.419190892 66.543783687]
Average time for minikube: 68.85818562133333

Times for Minikube (PR 8168): [66.970489075 67.081753545 63.305544953]
Average time for Minikube (PR 8168): 65.785929191

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8168) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.070877 |           0.069223 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.023707 |           0.022862 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.003943 |           0.003810 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 42.827413 |          19.906559 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 23.414661 |          21.990117 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  2.178683 |           1.485973 |
| components...                  |           |                    |
| * Enabled addons:              |  0.255108 |           0.685099 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.078512 |           0.077923 |
| configured to use "minikube"   |           |                    |
|                                |  0.005282 |          20.477989 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [28.444378598000004 27.927856256000002 29.606462918]
Average time for minikube: 28.659565924000002

Times for Minikube (PR 8168): [33.466827545 29.711702368999998 29.511170401999998]
Average time for Minikube (PR 8168): 30.896566772

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8168) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.083824 |           0.085436 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.002925 |           0.002757 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.069660 |           0.068583 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  8.469604 |           8.382220 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.140839 |           0.164958 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 18.467831 |          20.036104 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  1.335862 |           1.562631 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.003377 |           0.068087 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.081463 |           0.073935 |
| configured to use "minikube"           |           |                    |
|                                        |  0.004180 |           0.445586 |
+----------------------------------------+-----------+--------------------+

@medyagh
Copy link
Member

medyagh commented May 20, 2020

average on this PR is 2 seconds slower

Average time for minikube: 28.659565924000002
Average time for Minikube (PR 8168): 30.896566772

I wonder if that is related.

@medyagh
Copy link
Member

medyagh commented May 20, 2020

@sharifelgamal do you mind pulling upstream and let the test run one more time

@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [66.839387294 71.583995484 63.933215389]
Average time for minikube: 67.452199389

Times for Minikube (PR 8168): [64.153784798 71.471857242 67.47686760100001]
Average time for Minikube (PR 8168): 67.70083654700001

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8168) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.067941 |           0.067858 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.021561 |           0.022693 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.005539 |           0.003542 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 42.112204 |          41.922548 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 22.680126 |          23.698754 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.569300 |           1.498420 |
| components...                  |           |                    |
| * Enabled addons:              |  0.911603 |           0.405350 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.078303 |           0.077665 |
| configured to use "minikube"   |           |                    |
|                                |  0.005624 |           0.004007 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [28.628200852 27.688131349000003 27.517838163000004]
Average time for minikube: 27.94472345466667

Times for Minikube (PR 8168): [28.815339003000002 28.169677369 26.532205619999996]
Average time for Minikube (PR 8168): 27.839073997333333

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8168) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.079235 |           0.078758 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.003082 |           0.002748 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.063911 |           0.062181 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  7.980244 |           7.877701 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.131767 |           0.123359 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 18.665166 |          18.321803 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  0.880811 |           1.258401 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.070229 |           0.044367 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.066330 |           0.066154 |
| configured to use "minikube"           |           |                    |
|                                        |  0.003948 |           0.003604 |
+----------------------------------------+-----------+--------------------+

@sharifelgamal
Copy link
Collaborator Author

average on this PR is 2 seconds slower

Average time for minikube: 28.659565924000002
Average time for Minikube (PR 8168): 30.896566772

I wonder if that is related.

Unlikely, the times were totally sunk by the one run that was 33 seconds. We should do something slightly better with the timings, like run 5 and drop the highest and lowest number.

medyagh
medyagh approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

looks good to me

@medyagh medyagh merged commit 07c7b7c into kubernetes:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medyagh medyagh medyagh approved these changes

@afbjorklund afbjorklund Awaiting requested review from afbjorklund

@tstromberg tstromberg Awaiting requested review from tstromberg

Assignees
No one assigned
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sharifelgamal @k8s-ci-robot @minikube-pr-bot @codecov-commenter @medyagh