new-app: verify if API has required resources enabled #20020

wozniakjan · 2018-06-15T13:26:31Z

fixes #20002 by checking the kind of each resource

now it fails very early, produces following output if builds are disabled

oc new-app https://github.com/wozniakjan/oc_newapp_expose#env_dot_syntax                  
error: Missing required resources: Build, BuildConfig

ptal @openshift/sig-developer-experience, @mfojtik

bparees

one nit on the text and lgtm.

bparees · 2018-06-15T14:55:56Z

pkg/oc/cli/cmd/newapp.go

+	}
+
+	if len(missingResources) > 0 {
+		return fmt.Errorf("Missing required resources: %v", strings.Join(missingResources, `, `))


"API server is missing required resources: "

Hmmm... I'm not sure I want this to fail entirely. Why just not create the BC, the oc new-app is still useful without builds, and in a build-disabled cluster you're making it entirely unusable, yet I can still create an app, just without the build, as in oc new-app openshift/hello-openshift should still create the DC, IS and SVC for me. I don't see any reason that would fail me with information that BC is missing, even if I don't care about one.

maybe. If new-app generated a buildconfig, we definitely want this to fail, we don't want to create some, but not all, of the resources. Creating a DC when the BC that generates the image the DC deploys, doesn't exist, would be bad.

But if you're actually just using new-app to create a DC (oc new-app centos/mysql-56-centos7, or oc new-app -f sometemplatewithoutaBC.yaml) I suppose it's fair to allow that to proceed.

So I think the ideal solution would iterate the objects we are going to attempt to create, ensure those types all exist in the api server, and if not, abort indicating which resources that new-app wanted to create, were not available in the api server (abort before creating any resources).

Yup, that last sentence is exactly what I'd expect to happen. The current approach fails always, but like Ben said oc new-app centos/mysql-56-centos7 should work.

bparees · 2018-06-15T14:56:37Z

/cc @openshift/cli-review

adambkaplan · 2018-06-15T15:26:03Z

pkg/oc/cli/cmd/newapp.go

+		}
+	}
+
+	requiredResources := []string{"BuildConfig", "ImageStream"}


based on this our objects are "native" and not namespaced CRD-style Kinds?

we actually have both the legacy apis and the grouped apis. This should probably check to see if either exists, in case we eventually remove the legacy api.

I don't think you have to check for the ImageStream resource as there is no way today to disable image apis. So this can be simplified to just check the existence of BuildConfigs.

Also I don't think we support legacy (oapi) in CLI anymore, right @deads2k / @soltysh ?

Also I don't think we support legacy (oapi) in CLI anymore, right @deads2k / @soltysh ?

No legacy in the CLI.

Thanks, I will mark the PR as [WIP] for now and address the comments when I'm back from PTO next week.

What does "no legacy in the cli" mean? In this case we're simply checking what the api server supports, we're not using these apis to call the server.

soltysh · 2018-06-19T10:25:17Z

pkg/oc/cli/cmd/newapp.go

+	}
+
+	if len(missingResources) > 0 {
+		return fmt.Errorf("Missing required resources: %v", strings.Join(missingResources, `, `))


Hmmm... I'm not sure I want this to fail entirely. Why just not create the BC, the oc new-app is still useful without builds, and in a build-disabled cluster you're making it entirely unusable, yet I can still create an app, just without the build, as in oc new-app openshift/hello-openshift should still create the DC, IS and SVC for me. I don't see any reason that would fail me with information that BC is missing, even if I don't care about one.

wozniakjan · 2018-06-25T12:32:21Z

So I think the ideal solution would iterate the objects we are going to attempt to create, ensure those types all exist in the api server, and if not, abort indicating which resources that new-app wanted to create, were not available in the api server (abort before creating any resources).

@soltysh, @bparees, @adambkaplan I would like to try to obtain a list/map of required resources (the code is in a second commit c0ab188), but I suppose iterating over the result.List.Items from config.Run() is probably incorrect.

origin/pkg/oc/cli/cmd/newapp.go

Lines 171 to 174 in c0ab188

    
           declaredResources := make(map[schema.GroupVersionKind]bool) 
        
           for _, item := range items { 
        
           	declaredResources[item.GetObjectKind().GroupVersionKind()] = true 
        
           }

this appears to only yeild /, Kind=

Should I rather type-switch on the items like it is done here and care only about missing BuildConfig?

origin/pkg/oc/cli/cmd/newapp.go

Lines 400 to 406 in c0ab188

    
           for _, item := range result.List.Items { 
        
           	switch t := item.(type) { 
        
           	case *kapi.Pod: 
        
           		if t.Annotations[newcmd.GeneratedForJob] == "true" { 
        
           			installing = append(installing, t) 
        
           		} 
        
           	case *buildapi.BuildConfig:

bparees · 2018-06-25T19:20:37Z

Should I rather type-switch on the items like it is done here and care only about missing BuildConfig?

i'd say that's an acceptable path of least resistance at this point.

soltysh · 2018-06-27T11:07:37Z

Hold with any actions until after the rebase, there will be some significant changes in the apimachinery that will change some bits of your code.

wozniakjan · 2018-06-27T14:15:18Z

EDIT: I spent so much time writing below that I missed @soltysh comment. Thanks and will wait!

@bparees, @adambkaplan trying to wrap my head around the comment regarding CRD and this entire issue. Please correct the following, I will try to summarize my understanding here

there are two ways oc new-app populates the list of objects that are later sent to the API server
1. oc new-app [input_app_url] - which inspects the input and generates BuildConfigs, Services, DeploymentConfigs, and ImageStreams
2. oc new-app -f [template].yaml - the above-mentioned objects + any type of object including CRD
the issue was that when disabling builds in master-config.yaml, we should not create any resources and fail loud and early
there is no easy way to iterate over the list and generically check that any type of resource is going to be understood by the API because neither oc new-app nor kubernetes during conversions from typed to untyped populate runtime.Object.GetObjectKind().GroupVersionKind(). Maybe this Conversion from typed to unstructured should set GVK kubernetes/kubernetes#59264 and [WIP] add apiversion and kind to list subitems kubernetes/kubernetes#63972 will allow that at some point
so we will check only for the regular BuildConfig and expect that users understand their [template].yaml and API well enough

@mfojtik shouldn't this rather go to release-3.9 only given #19070 removed the ability to disable Builds?

bparees · 2018-06-27T14:32:34Z

@mfojtik shouldn't this rather go to release-3.9 only given #19070 removed the ability to disable Builds?

i don't think this is worth backporting.

wozniakjan · 2018-06-29T13:32:37Z

i don't think this is worth backporting.

in that case, is it even worth having this? Post 3.9 releases don't have the disableFeatures option so now build API gets disabled the same way as any other API afaik, and we currently don't have any nice and elegant way how to generically test if any GroupVersionKind from a list of runtime objects is enabled in the API as mentioned above (the runtime.Object.GetObjectKind().GroupVersionKind() doesn't get populated here)

Rebased, pushed changes as last 'WIP' commit for review. If this is indeed what we would like to have, I will squash all commits into one

bparees · 2018-06-29T15:30:20Z

can't we attempt to look up the kind/convert it from unstructured?

soltysh · 2018-07-02T11:12:19Z

and we currently don't have any nice and elegant way how to generically test if any GroupVersionKind from a list of runtime objects is enabled in the API as mentioned above

We do, check HasResource in kubectl's run how this is handled.

wozniakjan · 2018-07-02T11:23:18Z

We do, check HasResource in kubectl's run how this is handled.

@soltysh but isn't this assuming runtime.Object.GetObjectKind().GroupVersionKind() are populated in the list of resources? They aren't here

origin/pkg/oc/cli/cmd/newapp.go

Lines 349 to 353 in c0ab188

    
           result, err := config.Run() 
        
           if err := handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformRunError); err != nil { 
        
           	return err 
        
           } 
        
           if err := checkResources(config.DiscoveryClient, result.List.Items); err != nil {

soltysh · 2018-07-03T12:48:02Z

@wozniakjan we should be setting those with the current state of oc. If it does not lemme know, I might end up looking into myself.

wozniakjan · 2018-07-04T13:19:29Z

@soltysh added few debug statements wozniakjan#4 and rebased on current master

$ oc version                           
oc v3.11.0-alpha.0+464544f-212-dirty                
kubernetes v1.11.0+d4cacc0                          
features: Basic-Auth GSSAPI Kerberos SPNEGO         

Server https://127.0.0.1:8443                       
openshift v3.11.0-alpha.0+464544f-212-dirty         
kubernetes v1.11.0+d4cacc0

$ oc new-app https://github.com/wozniakjan/oc_newapp_expose#env_dot_syntax                  
--> Found Docker image bb81a09 (17 months old) from Docker Hub for "openshift/base-centos7"              

    * An image stream tag will be created as "base-centos7:latest" that will track the source image      
    * A Docker build using source code from https://github.com/wozniakjan/oc_newapp_expose#env_dot_syntax will be created                                                                                          
      * The resulting image will be pushed to image stream tag "ocnewappexpose:latest"                   
      * Every time "base-centos7:latest" changes a new build will be triggered                           
    * This image will be deployed in deployment config "ocnewappexpose"                                  
    * Port 8080/tcp will be load balanced by service "ocnewappexpose"                                    
      * Other containers can access this service through the hostname "ocnewappexpose"                   
    * WARNING: Image "openshift/base-centos7" runs as the 'root' user which may not be permitted by your cluster administrator                                                                                     

######### [0xc420094840 0xc420fb9e40 0xc4200d0780 0xc4202cca00 0xc4207f41e0]                             
######### map[/, Kind=:true]                        
error: Missing declared resources: /, Kind=

So I still think it doesn't populate GVK in oc new-app, unless I am missing something terribly obvious

soltysh · 2018-07-05T11:16:35Z

See wozniakjan#4 (comment) for my answer

wozniakjan · 2018-07-09T15:48:19Z

@bparees with @soltysh recommendation, it now checks generically all resources and reports if any required are missing. I like this a lot better, a review is greatly appreciated

bparees

one message nit and lgtm.

bparees · 2018-07-09T18:47:06Z

pkg/oc/cli/cmd/newapp.go

+	}
+
+	if len(missingResources) > 0 {
+		return fmt.Errorf("Missing declared resources: %v", strings.Join(missingResources, `, `))


"API server is missing declared resources:"

corrected, thanks!

wozniakjan · 2018-07-10T11:58:02Z

/retest

FORWARD PARAMETERS TO THE REMOTE HOST

bparees · 2018-07-10T13:59:19Z

/approve

@soltysh lgty?

wozniakjan · 2018-07-16T10:27:54Z

@bparees @soltysh rebased on top of master (no actual conflict), let me know if you have any other concerns or if we can get this merged

wozniakjan · 2018-07-16T11:50:59Z

/test gcp

wozniakjan · 2018-07-16T13:12:14Z

/test gcp

bparees · 2018-07-16T15:02:22Z

@wozniakjan appears to be merge conflicted again :(

wozniakjan · 2018-07-16T15:11:56Z

@bparees thanks, you are faster than our merge bot :)
rebased on top of master, this time no conflict and 3-way merge took care of it

bparees · 2018-07-16T18:33:52Z

/lgtm

openshift-ci-robot · 2018-07-16T18:33:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, wozniakjan

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:

~~pkg/oc/OWNERS~~ [bparees]

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

openshift-bot · 2018-07-16T19:25:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2018-07-16T20:08:01Z

@wozniakjan: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/verify	`26f8b9e`	link	`/test verify`
ci/prow/e2e-gcp	`c0ab188`	link	`/test e2e-gcp`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

openshift-bot · 2018-07-16T21:29:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

deads2k · 2018-08-28T17:11:40Z

pkg/oc/cli/newapp/newapp.go

+
+func checkResources(discoveryClient discovery.DiscoveryInterface, items []runtime.Object) error {
+	resources, err := discoveryClient.ServerResources()
+	if err != nil {


@wozniakjan @soltysh I got here because this particular line didn't tolerate partial discovery. All CLI commands should tolerate partial discovery and only fail if they cannot retrieve sufficient information. There may be cases where all of discovery must match, but this isn't one of them.

deads2k · 2018-08-28T17:14:27Z

As an overall experience with kubectl and oc CLIs, the commands create as much as possible and continue on failures. The commands are idempotent so they can be retried and they are pipeable to delete for cleanup. This change run afoul of those principles. Also, in 3.11 (maybe 3.10 too), the disableFeature that led to this is gone.

I'm reverting this because it's leading to behavioral problems in our CLI.

@wozniakjan @soltysh @juanvallejo @mfojtik

bparees · 2018-08-28T17:15:33Z

revert is here: #20780

(unless @deads2k also created one)

bparees · 2018-08-28T17:44:48Z

new revert PR: #20781

wozniakjan added component/cli sig/developer-experience labels Jun 15, 2018

wozniakjan requested review from mfojtik, soltysh and bparees June 15, 2018 13:26

openshift-ci-robot requested review from enj and juanvallejo June 15, 2018 13:26

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 15, 2018

bparees requested changes Jun 15, 2018

View reviewed changes

bparees self-assigned this Jun 15, 2018

adambkaplan reviewed Jun 15, 2018

View reviewed changes

wozniakjan changed the title ~~new-app: verify if API has BC and IS resources enabled~~ [WIP] new-app: verify if API has BC and IS resources enabled Jun 18, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2018

soltysh suggested changes Jun 19, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2018

wozniakjan force-pushed the issue20002/oc_new-app/resource_check branch from c0ab188 to 3ed3e06 Compare June 29, 2018 14:02

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018

wozniakjan force-pushed the issue20002/oc_new-app/resource_check branch 2 times, most recently from 878012b to 6832a9c Compare July 9, 2018 15:50

bparees requested changes Jul 9, 2018

View reviewed changes

wozniakjan force-pushed the issue20002/oc_new-app/resource_check branch from 6832a9c to aad0a92 Compare July 10, 2018 09:43

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2018

wozniakjan force-pushed the issue20002/oc_new-app/resource_check branch from aad0a92 to 996a2fe Compare July 16, 2018 08:42

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2018

new-app: verify if API has required resources enabled

3011e56

wozniakjan force-pushed the issue20002/oc_new-app/resource_check branch from 996a2fe to 3011e56 Compare July 16, 2018 15:10

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2018

openshift-merge-robot merged commit 71b595b into openshift:master Jul 17, 2018

bparees mentioned this pull request Aug 28, 2018

Revert "new-app: verify if API has required resources enabled" #20780

Closed

deads2k reviewed Aug 28, 2018

View reviewed changes

new-app: verify if API has required resources enabled #20020

new-app: verify if API has required resources enabled #20020

Conversation

wozniakjan commented Jun 15, 2018 • edited Loading

bparees left a 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

bparees commented Jun 15, 2018

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

wozniakjan commented Jun 25, 2018 • edited Loading

bparees commented Jun 25, 2018

soltysh commented Jun 27, 2018

wozniakjan commented Jun 27, 2018 • edited Loading

bparees commented Jun 27, 2018

wozniakjan commented Jun 29, 2018 • edited Loading

bparees commented Jun 29, 2018

soltysh commented Jul 2, 2018

wozniakjan commented Jul 2, 2018 • edited Loading

soltysh commented Jul 3, 2018

wozniakjan commented Jul 4, 2018

soltysh commented Jul 5, 2018

wozniakjan commented Jul 9, 2018

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wozniakjan commented Jul 10, 2018

bparees commented Jul 10, 2018

wozniakjan commented Jul 16, 2018

wozniakjan commented Jul 16, 2018

wozniakjan commented Jul 16, 2018

bparees commented Jul 16, 2018

wozniakjan commented Jul 16, 2018

bparees commented Jul 16, 2018

openshift-ci-robot commented Jul 16, 2018

openshift-bot commented Jul 16, 2018

openshift-ci-robot commented Jul 16, 2018 • edited Loading

openshift-bot commented Jul 16, 2018

Choose a reason for hiding this comment

deads2k commented Aug 28, 2018

bparees commented Aug 28, 2018

bparees commented Aug 28, 2018

wozniakjan commented Jun 15, 2018 •

edited

Loading

wozniakjan commented Jun 25, 2018 •

edited

Loading

wozniakjan commented Jun 27, 2018 •

edited

Loading

wozniakjan commented Jun 29, 2018 •

edited

Loading

wozniakjan commented Jul 2, 2018 •

edited

Loading

openshift-ci-robot commented Jul 16, 2018 •

edited

Loading