Fix to delete context when delete minikube #6541

anencore94 · 2020-02-07T07:31:30Z

When minikube delete, minikube must delete its contexts which are set when minikube start.
But, there was some mistakes in the function that does this logic.
DeleteContext(machineName string, configPath ...string) in pkg/minikube/kubeconfig/context.go was not matching with usage in deleteContext.
- it calls the function in the wrong order of parameter :
- kubeconfig.DeleteContext(constants.KubeconfigPath, machineName)

k8s-ci-robot · 2020-02-07T07:31:38Z

Welcome @anencore94!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-02-07T07:31:38Z

Hi @anencore94. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2020-02-07T07:31:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anencore94
To complete the pull request process, please assign afbjorklund
You can assign the PR to them by writing /assign @afbjorklund 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

minikube-bot · 2020-02-07T07:39:03Z

Can one of the admins verify this patch?

TravisBuddy · 2020-02-07T07:40:55Z

Travis tests have failed

Hey @anencore94,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 2a329880-497d-11ea-bd91-d1e011516349

medyagh · 2020-02-07T08:13:57Z

the code doesnt build ! @anencore94

anencore94 · 2020-02-07T08:19:30Z

@medyagh Thanks for checking !!
The error message of travis ci build failed is as follows :

$ make
which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
# cd .; git clone -- https://github.com/jteeuwen/go-bindata /home/travis/gopath/src/github.com/jteeuwen/go-bindata
Cloning into '/home/travis/gopath/src/github.com/jteeuwen/go-bindata'...
fatal: unable to access 'https://github.com/jteeuwen/go-bindata/': Failed to connect to github.com port 443: Connection timed out

I think it doesn't build since some connection error occured ?

medyagh · 2020-02-07T08:28:05Z

I restarted the build ! this does look like a LEGIT bug ! thank you for findind this ! @anencore94

medyagh · 2020-02-07T08:31:04Z

@anencore94 how do you feel about adding an integeration test for this here https://github.com/kubernetes/minikube/blob/master/test/integration/start_stop_delete_test.go#L153
so it never happens again ?
+10 points to you if you sign up for this challenge !

TravisBuddy · 2020-02-07T08:40:23Z

Travis tests have failed

Hey @anencore94,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 7916d940-4985-11ea-83a0-77379528e44b

anencore94 · 2020-02-07T08:45:10Z

@medyagh
I'd love too :) I'll add commit on this pr later.

- when `minikube delete`, minikube must delete its config which are set when `minikube start`. - But, there was some mistakes in the function that does this logic Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

anencore94 · 2020-02-07T11:07:27Z

@medyagh
I've been force-updated my commit. Just added simple if-else assertions.
Please re-check this pr and review on my test code!! Thanks :)

Also, the travis-ci failed on lint by this error:

./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.23.2'
golangci/golangci-lint crit unable to find 'v1.23.2' - use 'latest' or see https://github.com/golangci/golangci-lint/releases for details
Makefile:350: recipe for target 'out/linters/golangci-lint-v1.23.2' failed
make[1]: *** [out/linters/golangci-lint-v1.23.2] Error 1

I guess something happened in the travisci-running-node..?

codecov-io · 2020-02-07T11:18:15Z

Codecov Report

Merging #6541 into master will increase coverage by 0.13%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #6541      +/-   ##
==========================================
+ Coverage   37.94%   38.08%   +0.13%     
==========================================
  Files         138      137       -1     
  Lines        8701     8684      -17     
==========================================
+ Hits         3302     3307       +5     
+ Misses       4982     4962      -20     
+ Partials      417      415       -2

Impacted Files	Coverage Δ
cmd/minikube/cmd/delete.go	`25.12% <0%> (ø)`	⬆️
pkg/minikube/cluster/start.go	`52.33% <0%> (ø)`	⬆️
pkg/minikube/bootstrapper/certs.go	`75.24% <0%> (ø)`	⬆️
pkg/minikube/cluster/docker_env.go
cmd/minikube/cmd/start.go	`22.17% <0%> (+0.07%)`	⬆️
cmd/minikube/cmd/env.go	`57.37% <0%> (+0.46%)`	⬆️
pkg/minikube/bootstrapper/bsutil/kubeadm.go	`82.6% <0%> (+0.51%)`	⬆️
pkg/minikube/bootstrapper/bsutil/extraconfig.go	`80.82% <0%> (+1.71%)`	⬆️
pkg/minikube/cruntime/docker.go	`42.4% <0%> (+4.79%)`	⬆️
pkg/minikube/cruntime/crio.go	`53.12% <0%> (+6.25%)`	⬆️

anencore94 · 2020-02-07T11:33:05Z

By the way, before this commit merged, the master branch source code always goes into this way :
cmd/minikube/cmd/delete.go 240th line => 241th line
=> pkg/minikube/kubeconfig/context.go 66th line => 76th line (since api.IsConfigEmpty(kcfg) is true) => 78th line => finally, return nil (not error)

Is it ok to just changing the wrong-ordered-parameter? Or does the condition should be changed somehow ?

medyagh · 2020-02-07T18:44:47Z

/ok-to-test

minikube-pr-bot · 2020-02-07T19:04:10Z

All Times minikube: [ 92.002662 96.278031 92.898417]
All Times Minikube (PR 6541): [ 92.912047 89.861729 94.974701]

Average Minikube (PR 6541): 92.582826
Average minikube: 93.726370

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6541) |
+----------------------+-----------+--------------------+
| minikube v           |  0.210857 |           0.198073 |
| Creating kvm2        | 20.288932 |          19.955933 |
| Preparing Kubernetes | 49.835244 |          49.878295 |
| Pulling images       |           |                    |
| Launching Kubernetes | 21.524991 |          20.760728 |
| Waiting for cluster  |  0.053681 |           0.232708 |
+----------------------+-----------+--------------------+

cmd/minikube/cmd/delete.go

tstromberg

The unit test looks good, but for some reason it's failing. I added some comments to hopefully make it easier to debug.

test/integration/start_stop_delete_test.go

tstromberg · 2020-02-07T21:17:02Z

Thank you so much for fixing this. It's been broken for so long that honestly I had imagined it as a feature.

tstromberg

Added minor requests for test debuggability.

tstromberg · 2020-02-07T23:49:49Z

/ok-to-test

minikube-pr-bot · 2020-02-07T23:52:59Z

All Times minikube: [ 90.458863 93.497989 90.886117]
All Times Minikube (PR 6541): [ 91.657923 94.693938 89.698456]

Average minikube: 91.614323
Average Minikube (PR 6541): 92.016773

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6541) |
+----------------------+-----------+--------------------+
| minikube v           |  0.128959 |           0.127530 |
| Creating kvm2        | 19.832374 |          19.590505 |
| Preparing Kubernetes | 49.038904 |          49.942938 |
| Pulling images       |           |                    |
| Launching Kubernetes | 20.881879 |          20.399429 |
| Waiting for cluster  |  0.069997 |           0.213291 |
+----------------------+-----------+--------------------+

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 7, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 7, 2020

k8s-ci-robot requested review from medyagh and priyawadhwa February 7, 2020 07:32

Fix bug to delete context when delete minikube

460f8d0

- when `minikube delete`, minikube must delete its config which are set when `minikube start`. - But, there was some mistakes in the function that does this logic Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

anencore94 force-pushed the deleteContext branch from 57550bd to 460f8d0 Compare February 7, 2020 11:03

k8s-ci-robot added 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 Feb 7, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 7, 2020

medyagh requested changes Feb 7, 2020

View reviewed changes

cmd/minikube/cmd/delete.go Outdated Show resolved Hide resolved

tstromberg suggested changes Feb 7, 2020

View reviewed changes

test/integration/start_stop_delete_test.go Show resolved Hide resolved

test/integration/start_stop_delete_test.go Show resolved Hide resolved

tstromberg suggested changes Feb 7, 2020

View reviewed changes

Let DeleteContext determine the KUBECONFIG path

2c08f62

tstromberg merged commit 8762c48 into kubernetes:master Feb 8, 2020

fmotrifork mentioned this pull request Feb 8, 2020

minikube 1.7.2 fishworks/fish-food#564

Merged

anencore94 deleted the deleteContext branch February 10, 2020 00:52

This was referenced Feb 10, 2020

improve integration test for delete kubeconfig context #6567

Merged

Fix kubeConfigPath #6568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to delete context when delete minikube #6541

Fix to delete context when delete minikube #6541

anencore94 commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

minikube-bot commented Feb 7, 2020

TravisBuddy commented Feb 7, 2020

medyagh commented Feb 7, 2020

anencore94 commented Feb 7, 2020

medyagh commented Feb 7, 2020

medyagh commented Feb 7, 2020

TravisBuddy commented Feb 7, 2020

anencore94 commented Feb 7, 2020

anencore94 commented Feb 7, 2020

codecov-io commented Feb 7, 2020 •

edited

Loading

anencore94 commented Feb 7, 2020

medyagh commented Feb 7, 2020

minikube-pr-bot commented Feb 7, 2020

tstromberg left a comment

tstromberg commented Feb 7, 2020

tstromberg left a comment •

edited

Loading

tstromberg commented Feb 7, 2020

minikube-pr-bot commented Feb 7, 2020

Fix to delete context when delete minikube #6541

Fix to delete context when delete minikube #6541

Conversation

anencore94 commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

minikube-bot commented Feb 7, 2020

TravisBuddy commented Feb 7, 2020

Travis tests have failed

TravisBuddy Request Identifier: 2a329880-497d-11ea-bd91-d1e011516349

medyagh commented Feb 7, 2020

anencore94 commented Feb 7, 2020

medyagh commented Feb 7, 2020

medyagh commented Feb 7, 2020

TravisBuddy commented Feb 7, 2020

Travis tests have failed

TravisBuddy Request Identifier: 7916d940-4985-11ea-83a0-77379528e44b

anencore94 commented Feb 7, 2020

anencore94 commented Feb 7, 2020

codecov-io commented Feb 7, 2020 • edited Loading

Codecov Report

anencore94 commented Feb 7, 2020

medyagh commented Feb 7, 2020

minikube-pr-bot commented Feb 7, 2020

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg commented Feb 7, 2020

tstromberg left a comment • edited Loading

Choose a reason for hiding this comment

tstromberg commented Feb 7, 2020

minikube-pr-bot commented Feb 7, 2020

codecov-io commented Feb 7, 2020 •

edited

Loading

tstromberg left a comment •

edited

Loading