Issue #345 docker-env doesn't behave as expected #350

praveenkumar · 2017-01-31T09:16:27Z

This patch will fix docker-env issue and it does require latest version of pflag which comes from viper plugin and since spf13/viper#205 is already merged which was holding us to use fork of this project.

./minikube start --docker-env HTTP_PROXY=http://fedora:fedora123@10.70.49.109:3128 --docker-env NO_PROXY="*xip.io,localhost,172.11.2.1" --iso-url=file:/Users/prkumar/minishift-centos7.iso --show-libmachine-logs --cpus 4 --memory 4096
Starting local OpenShift cluster using 'virtualbox' hypervisor...
Docker Config Options [HTTP_PROXY=http://fedora:fedora123@10.70.49.109:3128 NO_PROXY=*xip.io,localhost,172.11.2.1]

hferentschik · 2017-01-31T11:51:28Z

@praveenkumar seems CI is failing

Also, how did you do the change in version to Viper. When I just take your change to glide.yaml and delete the lock file to re-generate the lock file I get:

CGO_ENABLED=0 GOARCH=amd64 GOOS=darwin go build --installsuffix cgo     -ldflags="-X github.com/minishift/minishift/pkg/version.version=1.0.0-beta.3     -X github.com/minishift/minishift/pkg/version.isoVersion=v1.0.0 -X github.com    /minishift/minishift/pkg/version.openshiftVersion=v1.4.1 -s -w -extldflags     '-static'" -o ./out/darwin-amd64/minishift ./cmd/minikube
# github.com/minishift/minishift/vendor/github.com/spf13/viper
vendor/github.com/spf13/viper/viper.go:741: undefined:     mapstructure.StringToTimeDurationHookFunc
make: *** [out/darwin-amd64/minishift] Error 2
# github.com/minishift/minishift/vendor/github.com/spf13/viper
vendor/github.com/spf13/viper/viper.go:741: undefined:     mapstructure.StringToTimeDurationHookFunc
make: *** [test] Error 2

That's by the way pretty much expected and inline with what we have seen before. We need to figure out what's going on here first.

praveenkumar · 2017-01-31T12:24:20Z

@hferentschik yes I am working on that and your observation is as expected, I put it WIP for this reason. How did you generated glide.yaml at first place using glide init and then put required version?

hferentschik · 2017-01-31T12:38:38Z

Got you.

praveenkumar · 2017-02-01T08:06:19Z

@hferentschik So finally I ended up updating complete glide.yaml file. I followed below process and then add some of the bits which required to take downstream changes.

I also changed origin git version from 1.3. to 1.4.1

# rm glide.yaml glide.lock
# glide create => created new glide.yaml file
Updated glide.yaml with downstream bits like kubernetes suppose to come down from forked repo
# glide --debug up --force => created new lock file.

praveenkumar · 2017-02-01T08:23:21Z

retest this please

LalatenduMohanty · 2017-02-01T10:10:40Z

@praveenkumar doing make clean ;make vendor is giving me below error

[INFO]	--> Setting version for google.golang.org/cloud to 975617b05ea8a58727e6c1a06b6161ff4185a9f2.
[INFO]	--> Setting version for golang.org/x/text to ceefd2213ed29504fff30155163c8f59827734f3.
[INFO]	--> Setting version for golang.org/x/net to 4f2fc6c1e69d41baf187332ee08fbd2b296f21ed.
[INFO]	--> Setting version for golang.org/x/sys to d9157a9621b69ad1d8d77a1933590c416593f24f.
[INFO]	--> Setting version for gopkg.in/inf.v0 to 3887ee99ecf07df5b447e9b00d9c0b2adaa9f3e4.
[INFO]	--> Setting version for k8s.io/client-go to d72c0e162789e1bbb33c33cfa26858a1375efe01.
[INFO]	--> Setting version for gopkg.in/yaml.v2 to 4c78c975fe7c825c6d1466c42be594d1d6f3aba6.
[ERROR]	Failed to set references: Unable to update checked out version (Skip to cleanup)
make: *** [vendor] Error 1

LalatenduMohanty · 2017-02-01T10:43:41Z

$ glide cc made make vendor; make to work fine

hferentschik · 2017-02-01T12:03:08Z

cmd/minikube/cmd/start.go

@@ -187,7 +187,7 @@ func initStartFlags() {
 	startFlagSet.Int(cpus, constants.DefaultCPUS, "Number of CPU cores to allocate to the Minishift VM.")
 	startFlagSet.String(humanReadableDiskSize, constants.DefaultDiskSize, "Disk size to allocate to the Minishift VM. Use the format <size><unit>, where unit = b, k, m or g.")
 	startFlagSet.String(hostOnlyCIDR, "192.168.99.1/24", "The CIDR to be used for the minishift VM. (Only supported with VirtualBox driver.)")
-	startFlagSet.StringSliceVar(&dockerEnv, "docker-env", nil, "Environment variables to pass to the Docker daemon. Use the format <key>=<value>.")
+	startFlagSet.StringArrayVar(&dockerEnv, "docker-env", nil, "Environment variables to pass to the Docker daemon. Use the format <key>=<value>.")


hferentschik · 2017-02-01T12:05:44Z

docs/minishift_config_get.md

-      --username string                User name for the virtual machine.
-  -v, --v value                        log level for V logs
-      --vmodule value                  comma-separated list of pattern=N settings for file-filtered logging
+      --alsologtostderr                  log to standard error as well as files


What's the change in this generated files? I don't seem to see it?

--log-flush-frequency duration is not any more option in updated glog nothing else changed.

cool, thanks. I could not spot the difference ;-) Fair enough

hferentschik · 2017-02-01T12:06:51Z

docs/minishift_start.md

-      --vm-driver string           The driver to use for the Minishift VM. Possible values: [virtualbox vmwarefusion kvm xhyve hyperv] (default "kvm")
+      --cpus int                        Number of CPU cores to allocate to the Minishift VM. (default 2)
+      --disk-size string                Disk size to allocate to the Minishift VM. Use the format <size><unit>, where unit = b, k, m or g. (default "20g")
+      --docker-env stringArray          Environment variables to pass to the Docker daemon. Use the format <key>=<value>.


"stringArray", something missing in the command setup?

No it's not missing when we updated viper and pflag now doc generation include the type also. check https://github.com/kubernetes/minikube/blob/master/docs/minikube_start.md also have same.

I see. That sucks imo. How does "stringSlice" or "stringArray" any sense to the user. For him these are strings. Is there a bug report for this in Viper? If not we should create one. Seems also Viper is quite inactive :-(

hferentschik · 2017-02-01T12:07:32Z

docs/minishift_start.md

+      --iso-url string                  Location of the minishift ISO. (default "https://github.com/minishift/minishift-b2d-iso/releases/download/v1.0.0/minishift-b2d.iso")
+      --memory int                      Amount of RAM to allocate to the Minishift VM. (default 2048)
+      --metrics                         Install metrics (experimental)
+  -e, --openshift-env stringSlice       Specify key-value pairs of environment variables to set on the OpenShift container.


"stringSlice"? Towards the docs we should not expose things like string array or string slice.

This is auto-generated as per doc build. I am not sure if we can make any changes here.

We should investigate. IMO this is a bug in Viper then. If there is nothing in Viper, we can always control the generation and fix afterwards, but that's not something worth doing right now.

Actually, I have issues with many of the autogenerated documentation... :-s

hferentschik · 2017-02-01T12:49:17Z

glide.yaml

-  version: 152183b11abcd2b07ee814c8da82296340949747
-  repo: https://github.com/openshift/go-restful.git
-  vcs: git
- package: github.com/docker/distribution


nice to get rid of this

praveenkumar · 2017-02-01T14:32:36Z

@hferentschik @LalatenduMohanty @budhrg @gbraad please make sure you compile minishift using this PR before we merge I am seeing something interesting.

# Compiled binary with this PR  (check the oc output)

$ ./minishift start --show-libmachine-logs --iso-url=file:/Users/prkumar/minishift-b2d.iso --openshift-version v1.4.1
...
Provisioning OpenShift via '/Users/prkumar/.minishift/cache/oc/v1.4.1/oc [cluster up --use-existing-config --forward-ports false --host-config-dir /var/lib/minishift/openshift.local.config --host-data-dir /var/lib/minishift/hostdata --host-volumes-dir /var/lib/origin/openshift.local.volumes --metrics false --env  --public-hostname  --routing-suffix  --server-loglevel 0 --skip-registry-check false]'
-- Checking OpenShift client ... OK
-- Checking Docker client ... OK
-- Checking Docker version ... OK
-- Checking for existing OpenShift container ... OK
-- Checking for openshift/origin:v1.4.1 image ... 
   Pulling image openshift/origin:v1.4.1
   Pulled 0/3 layers, 3% complete
   Pulled 1/3 layers, 49% complete
   Pulled 2/3 layers, 83% complete
   Pulled 3/3 layers, 100% complete
   Extracting
   Image pull complete
-- Checking for available ports ... OK
-- Checking type of volume mount ... 
   Using Docker shared volumes for OpenShift volumes
-- Creating host directories ... OK
-- Finding server IP ... 
   Using 127.0.0.1 as the server IP
-- Starting OpenShift container ... 
   Creating initial OpenShift configuration
   Starting OpenShift using container 'origin'
   Waiting for API server to start listening
   OpenShift server started
-- Adding default OAuthClient redirect URIs ... OK
-- Installing registry ... OK
-- Installing router ... OK
-- Installing metrics ... OK
-- Importing image streams ... OK
-- Importing templates ... OK
-- Login to server ... OK
-- Creating initial project "myproject" ... OK
-- Removing temporary directory ... OK
-- Server Information ... 
   OpenShift server started.
   The server is accessible via web console at:
       https://127.0.0.1:8443

   The metrics service is available at:
       https://metrics-openshift-infra.127.0.0.1.xip.io

   You are logged in as:
       User:     developer
       Password: developer

   To login as administrator:
       oc login -u system:admin


# Using released version of minishift which also have oc-1.4.1 (see the end output)
$ ./minishift start --show-libmachine-logs --iso-url=file:/Users/prkumar/minishift-b2d.iso
....
Provisioning OpenShift via '/Users/prkumar/.minishift/cache/oc/v1.4.1/oc [cluster up --use-existing-config --host-config-dir /var/lib/minishift/openshift.local.config --host-data-dir /var/lib/minishift/hostdata]'
-- Checking OpenShift client ... OK
-- Checking Docker client ... OK
-- Checking Docker version ... OK
-- Checking for existing OpenShift container ... OK
-- Checking for openshift/origin:v1.4.1 image ... 
   Pulling image openshift/origin:v1.4.1
   Pulled 0/3 layers, 3% complete
   Pulled 0/3 layers, 39% complete
   Pulled 1/3 layers, 76% complete
   Pulled 2/3 layers, 81% complete
   Pulled 3/3 layers, 100% complete
   Extracting
   Image pull complete
-- Checking Docker daemon configuration ... OK
-- Checking for available ports ... OK
-- Checking type of volume mount ... 
   Using Docker shared volumes for OpenShift volumes
-- Creating host directories ... OK
-- Finding server IP ... 
   Using 192.168.64.2 as the server IP
-- Starting OpenShift container ... 
   Creating initial OpenShift configuration
   Starting OpenShift using container 'origin'
   Waiting for API server to start listening
   OpenShift server started
-- Adding default OAuthClient redirect URIs ... OK
-- Installing registry ... OK
-- Installing router ... OK
-- Importing image streams ... OK
-- Importing templates ... OK
-- Login to server ... OK
-- Creating initial project "myproject" ... OK
-- Removing temporary directory ... OK
-- Server Information ... 
   OpenShift server started.
   The server is accessible via web console at:
       https://192.168.64.2:8443

   You are logged in as:
       User:     developer
       Password: developer

   To login as administrator:
       oc login -u system:admin

hferentschik · 2017-02-01T15:09:59Z

@praveenkumar I noticed the same thing. It seems

minishift/cmd/minikube/cmd/start.go

Line 231 in 0fc98b2

if viper.IsSet(flag.Name) {

is broken. The idea here is to only pass options which were set by the user (which ever way). viper.IsSet(flag.Name) used to do that. Now this seems to return true regardless. We need to investigate this. Provided this is what you wanted to point out.

hferentschik · 2017-02-01T15:12:35Z

Maybe related to this - spf13/viper#276

praveenkumar · 2017-02-02T06:21:39Z

Maybe related to this - spf13/viper#276

@hferentschik yes so I used a specific version which have Jimmy changes plus logs info in the glide.yaml file.

praveenkumar · 2017-02-02T06:22:56Z

glide.yaml

@@ -1,7 +1,7 @@
 package: github.com/minishift/minishift
 import:
 - package: github.com/docker/machine
-  version: 91e368eb7422665278b1d5665139ee0f9980b588
+  version: 15fd4c70403bab784d91031af02d9e169ce66412


Updated docker-machine release - 0.9

can we add an actual version here, instead of the sha?

@hferentschik we can it was before set as sha so I just did same. Can we do it later sometime if that is not so blocker? Problem is my net connection take more than 30-40 mins after glide clear cache and regenerating lock file from clean setup :( I can put a issue if that make you happy :) and track it.

I'd prefer doing it now. It seems connected really. You also should not have to clear the caches completely. Once everything works maybe for good measure.

For me it does not take so long to regenerate. I can otherwise try tomorrow. I'd really like to get way from depending on sha's like this

@hferentschik done.

praveenkumar · 2017-02-02T06:23:25Z

glide.yaml

 - package: github.com/spf13/viper
-  repo: https://github.com/jimmidyson/viper.git
-  vcs: git
+  version: 382f87b929b84ce13e9c8a375a4b217f224e6c65


Added specific version because latest one is broken.

+1, any chance we can depend on an actual version/tag. I really dislike depending on commits like this.

Does this fix the issue with not able to bootstrap OpenShift 1.3.1 as well, or does this just address the problem with the isSet method?

it fix the openshift 1.3.1 also now. I also dislike on the commit but sadly this doesn't have any tag/release version :(

I also dislike on the commit but sadly this doesn't have any tag/release version

:-(, ok nothing we can do then. Maybe a comment of sorts why we take this commit

@hferentschik I did add a comment see L#36

praveenkumar · 2017-02-02T06:32:17Z

retest this please

coolbrg · 2017-02-02T06:54:56Z

retest this please

…he string

coolbrg · 2017-02-02T14:54:29Z

Hi @praveenkumar , I am unable to see that metrics log as per your comment above after compiling the binary with your PR.
I did glide cc && make vendor && make.

Note, I have changed to version to *beta4.dev to make sure it was compiled binary.

$ minishift start --show-libmachine-logs [--openshift-version v1.4.1] (used both)
Starting local OpenShift cluster using 'kvm' hypervisor...
Running pre-create checks...
Creating machine...
(minishift) Downloading /home/budhram/.minishift/cache/boot2docker.iso from file:///home/budhram/.minishift/cache/iso/minishift-b2d.iso...
Waiting for machine to be running, this may take a few minutes...
Detecting operating system of created instance...
Waiting for SSH to be available...
Detecting the provisioner...
Waiting for SSH to be available...
Detecting the provisioner...
Provisioning with boot2docker...
Copying certs to the local machine directory...
Copying certs to the remote machine...
Setting Docker configuration on the remote daemon...
Checking connection to Docker...
Docker is up and running!
Provisioning OpenShift via '/home/budhram/.minishift/cache/oc/v1.4.1/oc [cluster up --use-existing-config --host-config-dir /var/lib/minishift/openshift.local.config --host-data-dir /var/lib/minishift/hostdata]'
-- Checking OpenShift client ... OK
-- Checking Docker client ... OK
-- Checking Docker version ... OK
-- Checking for existing OpenShift container ... OK
-- Checking for openshift/origin:v1.4.1 image ... 
   Pulling image openshift/origin:v1.4.1
   Pulled 0/3 layers, 3% complete
   Pulled 1/3 layers, 60% complete
   Pulled 1/3 layers, 97% complete
   Pulled 2/3 layers, 98% complete
   Pulled 3/3 layers, 100% complete
   Extracting
   Image pull complete
-- Checking Docker daemon configuration ... OK
-- Checking for available ports ... OK
-- Checking type of volume mount ... 
   Using Docker shared volumes for OpenShift volumes
-- Creating host directories ... OK
-- Finding server IP ... 
   Using 192.168.42.245 as the server IP
-- Starting OpenShift container ... 
   Creating initial OpenShift configuration
   Starting OpenShift using container 'origin'
   Waiting for API server to start listening
   OpenShift server started
-- Adding default OAuthClient redirect URIs ... OK
-- Installing registry ... OK
-- Installing router ... OK
-- Importing image streams ... OK
-- Importing templates ... OK
-- Login to server ... OK
-- Creating initial project "myproject" ... OK
-- Removing temporary directory ... OK
-- Server Information ... 
   OpenShift server started.
   The server is accessible via web console at:
       https://192.168.42.245:8443

   You are logged in as:
       User:     developer
       Password: developer

   To login as administrator:
       oc login -u system:admin

$ git diff | tee
diff --git a/Makefile b/Makefile
index 8e4cf21..db498ca 100644
--- a/Makefile
+++ b/Makefile
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-VERSION = 1.0.0-beta.3
+VERSION = 1.0.0-beta.4.dev
 OPENSHIFT_VERSION = v1.4.1
 ISO_VERSION = v1.0.0
 
$ git log -n 1 | tee
commit 69302e2a081ba98648ac951c7ea302affdd153b2
Author: Praveen Kumar <kumarpraveen.nitdgp@gmail.com>
Date:   Tue Jan 31 14:43:35 2017 +0530

    Fix #345 docker-env doesn't behave as expected when ',' in the string

$ minishift version
Minishift version: 1.0.0-beta.4.dev

$ sha256sum ~/.minishift/cache/iso/minishift-b2d.iso 
ddbd2a36c37eabef2a28f0a075b78d32bdbd3d15d3603856fe4521f8465260a7  /home/budhram/.minishift/cache/iso/minishift-b2d.iso

@LalatenduMohanty @hferentschik @gbraad Are you able to see?

gbraad · 2017-02-02T15:18:50Z

will verify ASAP

praveenkumar · 2017-02-03T04:48:08Z

@budhrg I updated the PR after that it was viper which causing the issue, now everything works as expected.

coolbrg · 2017-02-03T05:02:08Z

@budhrg I updated the PR after that it was viper which causing the issue, now everything works as expected.

Then, LGTM 👍

gbraad · 2017-02-03T07:01:18Z

LGTM

hferentschik · 2017-02-03T09:35:39Z

glide.yaml

@@ -1,7 +1,7 @@
 package: github.com/minishift/minishift
 import:
 - package: github.com/docker/machine
-  version: 91e368eb7422665278b1d5665139ee0f9980b588
+  version: 0.9.0


Thanks :-). Eventually I would like to switch as many as dependencies to using this form of version.

hferentschik · 2017-02-03T09:36:21Z

glide.yaml

@@ -27,26 +28,35 @@ import:
  subpackages:
  - github
 - package: github.com/inconshreveable/go-update
+- package: github.com/mitchellh/mapstructure
+  version: db1efb556f84b25a0a13a04aad883943538ad2e0


As said, we should aim for versions/tags, but let's go ahead for now

@hferentschik Sadly no release and tag for this also :(

hferentschik · 2017-02-03T09:37:13Z

glide.yaml

 # The following repositories need to be specified explicitly, since the versions referenced by the openshift/origin dependency
 # refer to forks of these projects within the openshift organizaton
 # This needs to be manually updated after upgrading the openshift/origin dependnecy
 - package: k8s.io/kubernetes
-  version: 52492b4bff99ef3b8ca617d385a3ff0612f9402d
+  version: a9e9cf3b407c1d315686c452bdb918c719c3ea6e


hopefully we can get soon rid of this dependency now. It takes most time to checkout and compile and is useless for us

hferentschik · 2017-02-03T09:46:08Z

Merged, thanks.

praveenkumar changed the title ~~Fix #345 docker-env doesn't behave as expected when ',' in the string~~ [WIP] Fix #345 docker-env doesn't behave as expected when ',' in the string Jan 31, 2017

praveenkumar force-pushed the issue_345 branch 2 times, most recently from aa4b475 to e990b01 Compare February 1, 2017 07:27

praveenkumar changed the title ~~[WIP] Fix #345 docker-env doesn't behave as expected when ',' in the string~~ Fix #345 docker-env doesn't behave as expected when ',' in the string Feb 1, 2017

praveenkumar force-pushed the issue_345 branch from 08e1d76 to 9be5ea5 Compare February 1, 2017 08:42

hferentschik self-assigned this Feb 1, 2017

hferentschik requested changes Feb 1, 2017

View reviewed changes

praveenkumar force-pushed the issue_345 branch from 9be5ea5 to 9bd56ea Compare February 2, 2017 06:16

praveenkumar commented Feb 2, 2017

View reviewed changes

Fix minishift#345 docker-env doesn't behave as expected when ',' in t…

69302e2

…he string

praveenkumar force-pushed the issue_345 branch from 9bd56ea to 69302e2 Compare February 2, 2017 11:37

LalatenduMohanty mentioned this pull request Feb 2, 2017

Isssue #235 Replacing jimmidyson with minishift #361

Closed

hferentschik approved these changes Feb 3, 2017

View reviewed changes

hferentschik changed the title ~~Fix #345 docker-env doesn't behave as expected when ',' in the string~~ Issue #345 docker-env doesn't behave as expected Feb 3, 2017

hferentschik closed this Feb 3, 2017

hferentschik mentioned this pull request Feb 3, 2017

docker-env doesn't behave as expected when ',' in the string #345

Closed

praveenkumar deleted the issue_345 branch June 20, 2017 09:59

Issue #345 docker-env doesn't behave as expected #350

Issue #345 docker-env doesn't behave as expected #350

Conversation

praveenkumar commented Jan 31, 2017 • edited Loading

hferentschik commented Jan 31, 2017 • edited Loading

praveenkumar commented Jan 31, 2017

hferentschik commented Jan 31, 2017

praveenkumar commented Feb 1, 2017

praveenkumar commented Feb 1, 2017

LalatenduMohanty commented Feb 1, 2017

LalatenduMohanty commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar commented Feb 1, 2017 • edited Loading

hferentschik commented Feb 1, 2017

hferentschik commented Feb 1, 2017

praveenkumar commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar commented Feb 2, 2017

coolbrg commented Feb 2, 2017

coolbrg commented Feb 2, 2017

gbraad commented Feb 2, 2017 via email

praveenkumar commented Feb 3, 2017

coolbrg commented Feb 3, 2017

gbraad commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hferentschik commented Feb 3, 2017

praveenkumar commented Jan 31, 2017 •

edited

Loading

hferentschik commented Jan 31, 2017 •

edited

Loading

praveenkumar commented Feb 1, 2017 •

edited

Loading

praveenkumar Feb 2, 2017 •

edited

Loading