Skip to content

hyperkit driver should be happy with current minimum verison #9365

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

Merged
merged 27 commits into from
Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
2423f81
Introduce minHyperkitVersion constant to keep the minimum compatible …
ilya-zuyev Sep 30, 2020
d124f29
Update comments
ilya-zuyev Sep 30, 2020
8ba7478
Add copyrights
ilya-zuyev Sep 30, 2020
334779c
Change required min hyperkit version to 1.11.0
ilya-zuyev Sep 30, 2020
11b8534
Fix unit tests error messages
ilya-zuyev Sep 30, 2020
c82e327
Do not panic if min version constant is invalid
ilya-zuyev Oct 1, 2020
6ca9402
Extract a function to download driver file, but don't change ownershi…
ilya-zuyev Oct 6, 2020
2641e3d
Add hyperkit driver v1.11.0 to testdata
ilya-zuyev Oct 6, 2020
7322fdd
Add an integration test for hyperkit driver upgrades
ilya-zuyev Oct 6, 2020
08da827
Rename minDriverVersion => minAcceptableDriverVersion. Add comments
ilya-zuyev Oct 9, 2020
9b70ec8
Update integration test:
ilya-zuyev Oct 13, 2020
786037e
Fix error message in int test
ilya-zuyev Oct 13, 2020
1381394
Fix comments
ilya-zuyev Oct 13, 2020
0a9cbbb
Use "--interactive=false" to avoid asking for sudo password
ilya-zuyev Oct 14, 2020
bfca3a6
Reorganize code - move tests related to hyperkit to driver_install_or…
ilya-zuyev Oct 15, 2020
489b59b
Integration tests: fix the default test driver perms
ilya-zuyev Oct 15, 2020
eef3fd8
Integration tests: add comments to internal functions
ilya-zuyev Oct 15, 2020
4eb6f9a
Integration tests: fix naming
ilya-zuyev Oct 15, 2020
3941018
Integration tests: update README in testsdata
ilya-zuyev Oct 15, 2020
2b32f6f
Integration tests: add copyright header
ilya-zuyev Oct 15, 2020
63ca225
Integration tests: fix driver permission
ilya-zuyev Oct 15, 2020
4c5ac5a
Integration tests: remove driver_install_or_update_darwin_test.go
ilya-zuyev Oct 15, 2020
a072e89
Merge branch 'master' into gh_7422-reuse_hyperkit_driver
ilya-zuyev Oct 15, 2020
8db8f55
replace glog -> klog
ilya-zuyev Oct 15, 2020
57a0e40
Reuse $MINIKUBE_HOME/cache files;
ilya-zuyev Oct 15, 2020
5fc73c7
Add a comment
ilya-zuyev Oct 15, 2020
6c65c5d
Log driver version when validating driver
ilya-zuyev Oct 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions pkg/minikube/driver/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,30 +41,37 @@ func InstallOrUpdate(name string, directory string, v semver.Version, interactiv
if name != KVM2 && name != HyperKit {
return nil
}
path, err := AssureDriverBinary(name, directory, v, autoUpdate)
if err != nil {
return err
}
return fixDriverPermissions(name, path, interactive)
}

// AssureDriverBinary does the same as InstallOrUpdate
// but doesn't set root permissions in case the driver is downloaded
func AssureDriverBinary(name string, directory string, v semver.Version, autoUpdate bool) (string, error) {
executable := fmt.Sprintf("docker-machine-driver-%s", name)

// Lock before we check for existence to avoid thundering herd issues
spec := lock.PathMutexSpec(executable)
spec.Timeout = 10 * time.Minute
glog.Infof("acquiring lock: %+v", spec)
releaser, err := mutex.Acquire(spec)
if err != nil {
return errors.Wrapf(err, "unable to acquire lock for %+v", spec)
return "", errors.Wrapf(err, "unable to acquire lock for %+v", spec)
}
defer releaser.Release()

exists := driverExists(executable)
path, err := validateDriver(executable, v)
path, err := validateDriver(executable, minAcceptableDriverVersion(name, v))
if !exists || (err != nil && autoUpdate) {
glog.Warningf("%s: %v", executable, err)
path = filepath.Join(directory, executable)
derr := download.Driver(executable, path, v)
if derr != nil {
return derr
if err := download.Driver(executable, path, v); err != nil {
return "", err
}
}
return fixDriverPermissions(name, path, interactive)
return path, nil
}

// fixDriverPermissions fixes the permissions on a driver
Expand Down Expand Up @@ -133,6 +140,7 @@ func validateDriver(executable string, v semver.Version) (string, error) {
if err != nil {
return path, errors.Wrap(err, "can't parse driver version")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we allow version mismatches, could you please add a log sttatement declaring what driver version we are using? This will help for future debugging;

klog.Infof("%s version is %s", path, driverVersion)

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

if driverVersion.LT(v) {
return path, fmt.Errorf("%s is version %s, want %s", executable, driverVersion, v)
}
Expand Down
52 changes: 52 additions & 0 deletions pkg/minikube/driver/version.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
Copyright 2020 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package driver

import (
"github.com/blang/semver"
"github.com/golang/glog"
)

// minHyperkitVersion is the minimum version of the minikube hyperkit driver compatible with the current minikube code
var minHyperkitVersion *semver.Version

const minHyperkitVersionStr = "1.11.0"

func init() {
v, err := semver.New(minHyperkitVersionStr)
if err != nil {
glog.Errorf("Failed to parse the hyperkit driver version: %v", err)
} else {
minHyperkitVersion = v
}
}

// minAcceptableDriverVersion is the minimum version of driver supported by current version of minikube
func minAcceptableDriverVersion(driver string, mkVer semver.Version) semver.Version {
switch driver {
case HyperKit:
if minHyperkitVersion != nil {
return *minHyperkitVersion
}
return mkVer
case KVM2:
return mkVer
default:
glog.Warningf("Unexpected driver: %v", driver)
return mkVer
}
}
52 changes: 52 additions & 0 deletions pkg/minikube/driver/version_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
Copyright 2020 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package driver

import (
"testing"

"github.com/blang/semver"
)

func Test_minDriverVersion(t *testing.T) {

tests := []struct {
desc string
driver string
mkV string
want semver.Version
}{
{"Hyperkit", HyperKit, "1.1.1", *minHyperkitVersion},
{"Invalid", "_invalid_", "1.1.1", v("1.1.1")},
{"KVM2", KVM2, "1.1.1", v("1.1.1")},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
if got := minAcceptableDriverVersion(tt.driver, v(tt.mkV)); !got.EQ(tt.want) {
t.Errorf("Invalid min supported version, got: %v, want: %v", got, tt.want)
}
})
}
}

func v(s string) semver.Version {
r, err := semver.New(s)
if err != nil {
panic(err)
}
return *r
}
83 changes: 83 additions & 0 deletions test/integration/driver_install_or_update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/blang/semver"

"k8s.io/minikube/pkg/minikube/driver"
"k8s.io/minikube/pkg/version"
)

func TestKVMDriverInstallOrUpdate(t *testing.T) {
Expand Down Expand Up @@ -173,3 +174,85 @@ func TestHyperKitDriverInstallOrUpdate(t *testing.T) {
}
}
}

func TestHyperKitDriverSkipUpgrade(t *testing.T) {
if runtime.GOOS != "darwin" {
t.Skip("Skip if not darwin.")
}
MaybeParallel(t)
tests := []struct {
name string
path string
downloadExpected bool
}{
{
name: "upgrade-v1.11.0-to-current",
path: filepath.Join(*testdataDir, "hyperkit-driver-version-1.11.0"),
downloadExpected: false,
},
{
name: "upgrade-v1.2.0-to-current",
path: filepath.Join(*testdataDir, "hyperkit-driver-older-version"),
downloadExpected: true,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
originalPath := os.Getenv("PATH")
defer func() {
if err := os.Setenv("PATH", originalPath); err != nil {
t.Errorf("failed to restore PATH: %v", err)
}
}()

dir, err := ioutil.TempDir("", tc.name)
if err != nil {
t.Fatalf("Expected to create tempdir. test: %s, got: %v", tc.name, err)
}
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Errorf("Failed to remove dir %q: %v", dir, err)
}
}()

pwd, err := os.Getwd()
if err != nil {
t.Fatalf("Error not expected when getting working directory. test: %s, got: %v", tc.name, err)
}

driverPath := filepath.Join(pwd, tc.path, "docker-machine-driver-hyperkit")
if _, err = os.Stat(driverPath); err != nil {
t.Fatalf("Expected driver to exist. test: %s, got: %v", tc.name, err)
}

// change permission to allow driver to be executable
if err = os.Chmod(driverPath, 0700); err != nil {
t.Fatalf("Expected not expected when changing driver permission. test: %s, got: %v", tc.name, err)
}

if err = os.Setenv("PATH", filepath.Dir(driverPath)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

instead of setting Env variable for the whole test (that could conflict the other parallel tests)
consider using the env only for the Comand
for example

	cmd.Env = append(os.Environ(), "DOCKER_HOST=/does/not/exist")
	if _, err := Run(t, cmd); err != nil {
//...
	}

here is an example here https://github.com/medyagh/minikube/blob/32922f4184c31fb405fb8dab7a1d7a8ef120b47a/test/integration/aaa_download_only_test.go#L170

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

t.Fatalf("Unexpected error on SetEn, got: %v", err)
}
curVersion, err := version.GetSemverVersion()
if err != nil {
t.Fatalf("Expected new semver. test: %v, got: %v", tc.name, err)
}

if _, err = driver.AssureDriverBinary("hyperkit", dir, curVersion, true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

how about instead of using "driver.AssureDriverBinary" use

minikube start --download-only --driver=hyperkit

similar to
https://github.com/medyagh/minikube/blob/32922f4184c31fb405fb8dab7a1d7a8ef120b47a/test/integration/aaa_download_only_test.go#L61

Copy link
Contributor Author

@ilya-zuyev ilya-zuyev Oct 9, 2020

Choose a reason for hiding this comment

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

I really like the idea of running minikube binary instead of just calling internal functions in integration tests.
But there is is a couple of issues here.

When minikube start --download-only --driver=hyperkit actually downloads the driver it tries to set driver permissions as well, asking for sudo. make integration is failing in this case.
Cannot run as sudo make integration too, because of
X Exiting due to DRV_AS_ROOT: The "hyperkit" driver should not be used with root privileges.

Another issue is that minikube start --download-only --driver=hyperkit downloads not only the driver, but also preloaded-images archive and minikube.iso. It's ~700Mb per test run.

Choose a reason for hiding this comment

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

We have at least one integration test that runs --download-only (TestDownloadOnly) to make sure it's working properly or uses it as part of a larger test. I'm guessing all of these tests would fail on hyperkit if this doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like my issue is specific to the external drivers only.
TestDownloadOnly uses the default driver, which is usually docker. In this case minikube does not try to install an external driver binary and skips the sudo step.

I tried to forcibly use hyperkit in TestDownloadOnly passing TEST_ARGS="-minikube-start-args --driver=hyperkit" and got the same error:

izuyev@izuyev-macbookpro --- z/minikube ‹gh_7422-reuse_hyperkit_driver* M› » make integration TEST_ARGS="-run TestDownloadOnly -minikube-start-args --driver=hyperkit"    
go test -ldflags="-X k8s.io/minikube/pkg/version.version=v1.13.1 -X k8s.io/minikube/pkg/version.isoVersion=v1.13.1 -X k8s.io/minikube/pkg/version.isoPath=minikube/iso -X k8s.io/minikube/pkg/version.gitCommitID="08da82702d38d646df313d793ab9ea0f1a4fa46d-dirty" -X k8s.io/minikube/pkg/version.storageProvisionerVersion=v3" -v -test.timeout=90m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp go_getter_nos3 go_getter_nogcs" -run TestDownloadOnly -minikube-start-args --driver=hyperkit 2>&1 | tee "./out/testout_08da82702.txt"
ERROR: logging before flag.Parse: I1012 13:09:00.486559   47903 translate.go:64] Getting system locale failed: Could not detect Language
ERROR: logging before flag.Parse: I1012 13:09:00.486698   47903 translate.go:106] Setting Language to en-US ...
Found 12 cores, limiting parallelism with --test.parallel=6
=== RUN   TestDownloadOnly
=== RUN   TestDownloadOnly/crio
=== RUN   TestDownloadOnly/crio/v1.13.0
    aaa_download_only_test.go:65: (dbg) Run:  out/minikube start --download-only -p crio-20201012130900-47903 --force --alsologtostderr --kubernetes-version=v1.13.0 --container-runtime=crio --driver=hyperkit
Password: 

[test infinitely waits for password]

Choose a reason for hiding this comment

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

I'm not super familiar with this part of the code, but do you know if we try to set the permissions before asking for root access?

My guess is that we don't need a password to set the permissions in our testing environment. If it doesn't already work like this, we could try to set the driver permissions first and then ask for root access if that fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I checked CI scripts, looks like out test environment just allows sudo without a password.

Updated the test to skip if sudo needs a password.

t.Fatalf("Failed to update driver to %v. test: %s, got: %v", curVersion, tc.name, err)
}

_, err = os.Stat(filepath.Join(dir, "docker-machine-driver-hyperkit"))
if tc.downloadExpected {
if err != nil {
t.Fatalf("driver downlad expected. test: %s, got: %v", tc.name, err)
}
} else {
if err == nil {
t.Fatalf("driver downlad NOT expected. test: %s", tc.name)
}
}
})
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Starting minikube version 1.13.1 we do not update the installed hyperkit driver if its version is 1.11.0 or higher.
Have the hyperkit driver v1.11.0 here to test this behaviour.
Binary file not shown.