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

Regression with --extra-clusterup-flags #2318

Closed
gbraad opened this Issue Apr 19, 2018 · 12 comments

Comments

Projects
None yet
6 participants
@gbraad
Member

gbraad commented Apr 19, 2018

With 8508137 the flags support check for oc got moved to the pre-flight, but we miss an essential part:

// Deal with extra flags (remove from cluster up params)
var extraFlags string
if val, ok := clusterUpParams[configCmd.ExtraClusterUpFlags.Name]; ok {
extraFlags = val
delete(clusterUpParams, configCmd.ExtraClusterUpFlags.Name)
}

This strips the --extra-clusterup-flags from being passed to the oc cluster up support check. After the check, the flags get passed in for execution:

extraFlagFields := strings.Fields(extraFlags)
for _, extraFlag := range extraFlagFields {
cmdArgs = append(cmdArgs, extraFlag)
}

Without this delete, the following error occurs:

-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.0. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported

For more info, see: https://gist.github.com/cmoulliard/039e85e17ee99ad8c9e25b975d3f3a8a#gistcomment-2561177

@gbraad gbraad changed the title from Regression with --extra-cluserup-flags to Regression with --extra-clusterup-flags Apr 19, 2018

@budhram budhram added this to the v1.17.0 milestone Apr 19, 2018

@gbraad

This comment has been minimized.

Member

gbraad commented Apr 19, 2018

The code in error is:

func checkOcFlag() bool {
clusterUpParams := determineInitialClusterupParameters()
for _, key := range clusterUpParams {
if !oc.SupportFlag(key, ocPath, &util.RealRunner{}) {
fmt.Printf("Flag '%s' is not supported for oc version %s. Use 'openshift-version' flag to select a different version of OpenShift.\n", key, viper.GetString(configCmd.OpenshiftVersion.Name))
return false
}
}
return true
}


I also miss an essential SKIP option for this. Every test we perform before a deployment should be allowed to skip.

@LalatenduMohanty

This comment has been minimized.

Member

LalatenduMohanty commented Apr 19, 2018

It effects #2309

$ minishift start --extra-clusterup-flags "--service-catalog"
-- Starting profile 'minishift'
-- Checking if https://github.com is reachable (using proxy: "No") ... OK
-- Checking if requested OpenShift version 'v3.9.0' is valid ... OK
-- Checking if requested OpenShift version 'v3.9.0' is supported ... OK
-- Checking if requested hypervisor 'xhyve' is supported on this platform ... OK
-- Checking if xhyve driver is installed ...
   Driver is available at /usr/local/bin/docker-machine-driver-xhyve
   Checking for setuid bit ... OK
-- Checking the ISO URL ... OK
-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.0. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported
@amitkrout

This comment has been minimized.

Contributor

amitkrout commented Apr 19, 2018

@LalatenduMohanty Then this will be KI for 3.4 downstream right ?

@amitkrout

This comment has been minimized.

Contributor

amitkrout commented Apr 19, 2018

I think its in CDK as this is a regression of v1.15.1

@amitkrout

This comment has been minimized.

Contributor

amitkrout commented Apr 19, 2018

Tested on CDK 3.4 darwin stage bit

$ ./cdk-3.4.0-2-minishift-darwin-amd64 start --extra-clusterup-flags "--service-catalog"
-- Starting profile 'minishift'
-- Checking if requested OpenShift version 'v3.9.14' is valid ... OK
-- Checking if requested OpenShift version 'v3.9.14' is supported ... OK
-- Checking if requested hypervisor 'xhyve' is supported on this platform ... OK
-- Checking if xhyve driver is installed ...
   Driver is available at /usr/local/bin/docker-machine-driver-xhyve
   Checking for setuid bit ... OK
-- Checking the ISO URL ... OK
-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.14. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported

$ ./cdk-3.4.0-2-minishift-darwin-amd64 start --extra-clusterup-flags "--logging"
-- Starting profile 'minishift'
-- Checking if requested OpenShift version 'v3.9.14' is valid ... OK
-- Checking if requested OpenShift version 'v3.9.14' is supported ... OK
-- Checking if requested hypervisor 'xhyve' is supported on this platform ... OK
-- Checking if xhyve driver is installed ...
   Driver is available at /usr/local/bin/docker-machine-driver-xhyve
   Checking for setuid bit ... OK
-- Checking the ISO URL ... OK
-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.14. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported

$ ./cdk-3.4.0-2-minishift-darwin-amd64 start --extra-clusterup-flags "--metrics"
-- Starting profile 'minishift'
-- Checking if requested OpenShift version 'v3.9.14' is valid ... OK
-- Checking if requested OpenShift version 'v3.9.14' is supported ... OK
-- Checking if requested hypervisor 'xhyve' is supported on this platform ... OK
-- Checking if xhyve driver is installed ...
   Driver is available at /usr/local/bin/docker-machine-driver-xhyve
   Checking for setuid bit ... OK
-- Checking the ISO URL ... OK
-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.14. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported
@amitkrout

This comment has been minimized.

Contributor

amitkrout commented Apr 19, 2018

It works if i am directly passing the experimental flag like minishift start --lgging but i cannot access the logging route as there is no such application is deployed.

@LalatenduMohanty

This comment has been minimized.

Member

LalatenduMohanty commented Apr 19, 2018

@LalatenduMohanty Then this will be KI for 3.4 right ?

Yes, it is an known issue for 3.4

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 19, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 19, 2018

Issue minishift#2318 Adding a skip option for artifact preflight check
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 19, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 19, 2018

Issue minishift#2318 Adding a skip option for artifact preflight check
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Adding a skip option for artifact preflight check
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Renamed artifact_preflight to prefilght_artifact…
…s.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Adding a skip option for artifact preflight check
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Renamed artifact_preflight to prefilght_artifact…
…s.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Added config options to skip or warn for the preflight check.
Also renamed cmd/minishift/cmd/artifact_preflight.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Added config options to skip or warn for the preflight check.
Also renamed cmd/minishift/cmd/artifact_preflight.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

LalatenduMohanty added a commit to LalatenduMohanty/minishift that referenced this issue Apr 20, 2018

Issue minishift#2318 Fix preflight check for extra-clusterup-flags
Added config options to skip or warn for the preflight check.
Also renamed cmd/minishift/cmd/artifact_preflight.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

budhram pushed a commit that referenced this issue Apr 20, 2018

Issue #2318 Fix preflight check for extra-clusterup-flags
Added config options to skip or warn for the preflight check.
Also renamed cmd/minishift/cmd/artifact_preflight.go

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
@robin-owen

This comment has been minimized.

Contributor

robin-owen commented Apr 20, 2018

@minishift/minishift-dev Is there a workaround for this issue that we can use downstream?

@gbraad

This comment has been minimized.

Member

gbraad commented Apr 20, 2018

@robin-owen

This comment has been minimized.

Contributor

robin-owen commented Apr 20, 2018

@gbraad Are you sure? I'm seeing this:

% minishift version
minishift v1.15.1+f19ac09
CDK v3.4.0-2
% MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --extra-clusterup-flags "--logging"
-- Starting profile 'minishift'
-- Checking if requested OpenShift version 'v3.9.14' is valid ... OK
-- Checking if requested OpenShift version 'v3.9.14' is supported ... OK
-- Checking if requested hypervisor 'kvm' is supported on this platform ... OK
-- Checking if KVM driver is installed ... 
   Driver is available at /usr/local/bin/docker-machine-driver-kvm ... 
   Checking driver binary is executable ... OK
-- Checking if Libvirt is installed ... OK
-- Checking if Libvirt default network is present ... OK
-- Checking if Libvirt default network is active ... OK
-- Checking the ISO URL ... OK
-- Checking if provided oc flags are supported ... Flag 'extra-clusterup-flags' is not supported for oc version v3.9.14. Use 'openshift-version' flag to select a different version of OpenShift.
FAIL
Provided oc flag not supported

Note: I get the same result when trying MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --extra-clusterup-flags "--service-catalog".

@gbraad

This comment has been minimized.

Member

gbraad commented Apr 20, 2018

@LalatenduMohanty

This comment has been minimized.

Member

LalatenduMohanty commented Apr 20, 2018

So the downstream code is older than upstream code and here is the expected behavior.

  • use of MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --extra-clusterup-flags will be an issue (as mentioned in this issue) in downstream too.

  • but for logging/metrics/service-catalog we have dedicated flags under experimental features in downstream i.e. you should use

MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --logging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment