Skip to content
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

WIP - support creating k8s built-in resources #4908

Closed
wants to merge 1 commit into from
Closed

WIP - support creating k8s built-in resources #4908

wants to merge 1 commit into from

Conversation

dharmit
Copy link
Member

@dharmit dharmit commented Jul 13, 2021

What type of PR is this?
/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jul 13, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 13, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

return
}

apiVersionList := strings.SplitN(apiVersion, "/", 2)
Copy link
Member

Choose a reason for hiding this comment

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

you should use methods from apimachinery to parse groupvesrions instead of writing your own https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime/schema#ParseGroupVersion

@dharmit
Copy link
Member Author

dharmit commented Jul 15, 2021

@kadel we discussed relying more on GVK instead of GVR in our code, and you also showed how we can modify the IsResourceSupported function to use a Kind instead of a Resource.

Another place where I'm hitting a wall is the dynamic client. In our code we create the dynamic resources on k8s cluster with the help of a resource. https://github.com/openshift/odo/blob/9237d8d21f4e99021cdaef94bd8b8bac922d33a0/pkg/kclient/deployments.go#L333

I'm finding it difficult to find a way to convert GVK to GVR without creating a function on my own. But it doesn't look like I should need to. Something should be already there in client-go. It's just that I'm not able to find it. 😕

@openshift-ci
Copy link

openshift-ci bot commented Jul 21, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from dharmit after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@dharmit
Copy link
Member Author

dharmit commented Jul 21, 2021

Current state of things.

The devfile I'm using:

devfile.yaml
commands:
- exec:
    commandLine: npm install
    component: runtime
    group:
      isDefault: true
      kind: build
    workingDir: /project
  id: install
- exec:
    commandLine: npm start
    component: runtime
    group:
      isDefault: true
      kind: run
    workingDir: /project
  id: run
- exec:
    commandLine: npm run debug
    component: runtime
    group:
      isDefault: true
      kind: debug
    workingDir: /project
  id: debug
- exec:
    commandLine: npm test
    component: runtime
    group:
      isDefault: true
      kind: test
    workingDir: /project
  id: test
components:
- container:
    endpoints:
    - name: http-3000
      targetPort: 3000
    image: registry.access.redhat.com/ubi8/nodejs-14:latest
    memoryLimit: 1024Mi
    mountSources: true
    sourceMapping: /project
  name: runtime
- kubernetes:
    inlined: |
      apiVersion: v1
      kind: Pod
      metadata:
        labels:
          name: nginx
        name: nginx
      spec:
        containers:
        - image: nginx
          name: nginx
          ports:
          - containerPort: 80
  name: nginx
metadata:
  description: Stack with Node.js 14
  displayName: Node.js Runtime
  language: nodejs
  name: nodejs
  projectType: nodejs
  tags:
  - NodeJS
  - Express
  - ubi8
  version: 1.0.1
schemaVersion: 2.0.0
starterProjects:
- git:
    remotes:
      origin: https://github.com/odo-devfiles/nodejs-ex.git
  name: nodejs-starter

On CRC, when I create a fresh component, odo push works fine; but odo list fails:

$ odo push                                                                                                                          
                                                                                                                                    
Validation                                                                                                                          
 ✓  Validating the devfile [60686ns]                                                                                                
                                                                                                                                    
Updating services                                                                                                                   
 ✓  Created service "Pod/nginx" on the cluster; refer "odo link -h" to know how to link it to the component                         
                                                                                                                                    
Creating Kubernetes resources for component nodejs-dharmit-molo                                                                     
 ✓  Waiting for component to start [6s]                                                                                             
 ✓  Waiting for component to start [11ms]                                                                                           
                                                                                                                                    
Applying URL changes                                                                                                                
 ✓  URL http-3000: http://http-3000-app-myproject.apps-crc.testing/ created                                                         
                                                                                                                                    
Syncing to component nodejs-dharmit-molo                                                                                            
 ✓  Checking files for pushing [5ms]                                                                                                
 ✓  Syncing files to the component [550ms]                                                                                          
                                                                                                                                    
Executing devfile commands for component nodejs-dharmit-molo                                                                        
 ✓  Waiting for component to start [5ms]                                                                                            
 ✓  Executing install command "npm install" [5s]                                                                                    
 ✓  Executing run command "npm start" [2s]                                                                                          
                                                                                                                                    
Pushing devfile component "nodejs-dharmit-molo"                                                                                     
 ✓  Changes successfully pushed to component

$ kubectl get pods
NAME                                      READY   STATUS             RESTARTS   AGE
nginx                                     0/1     CrashLoopBackOff   6          10m
nodejs-dharmit-molo-app-cff46999b-flvlp   1/1     Running            0          10m

$ odo list
 ✗  multiple Pods exist for the selector: app.kubernetes.io/part-of=app,app.kubernetes.io/instance=nodejs-dharmit-molo. Only one must be present

On minikube, odo push fails but the pod has been successfully created:

$ odo push

Validation
 ✓  Validating the devfile [71138ns]

Updating services
 ✓  Services and Links are in sync with the cluster, no changes are required

Creating Kubernetes resources for component nodejs-dharmit-ujei
 ✗  Failed to start component with name "nodejs-dharmit-ujei". Error: Failed to create the component: unable to create or update component: pvc not found for mount path kube-api-access-gcgph

$ kubectl get pods
NAME    READY   STATUS    RESTARTS   AGE
nginx   1/1     Running   0          17m

@dharmit
Copy link
Member Author

dharmit commented Jul 21, 2021

✗ multiple Pods exist for the selector: app.kubernetes.io/part-of=app,app.kubernetes.io/instance=nodejs-dharmit-molo. Only one must be present

What's the reason behind expecting only one pod for the component? Any pointers @kadel @mik-dass?

✗ Failed to start component with name "nodejs-dharmit-ujei". Error: Failed to create the component: unable to create or update component: pvc not found for mount path kube-api-access-gcgph

Why is odo trying to find a PVC here?

@mik-dass
Copy link
Contributor

What's the reason behind expecting only one pod for the component? Any pointers @kadel @mik-dass?

Since odo was designed earlier to work with only one pod, it currently expects only one pod from this call and two or more pods can confuse the tool in regards to the steps preformed in odo push etc.

Why is odo trying to find a PVC here?

Because a volume was probably mounted on the deployment.

@dharmit
Copy link
Member Author

dharmit commented Jul 22, 2021

Since odo was designed earlier to work with only one pod, it currently expects only one pod from this call and two or more pods can confuse the tool in regards to the steps preformed in odo push etc.

Yeah, I thought so. I was wondering if fixing this would be as simple as removing this check or should I consider anything else as well?

https://github.com/openshift/odo/blob/27892c8e4213573220890e438378d384af5e686c/pkg/kclient/pods.go#L215-L217

Because a volume was probably mounted on the deployment.

I wasn't purposefully creating a volume. odo preference file has Ephemeral set to true on my setup. What else could be creating a volume for a deployment?

@openshift-ci
Copy link

openshift-ci bot commented Jul 28, 2021

@dharmit: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/v4.8-integration-e2e 0c59691 link /test v4.8-integration-e2e

Full PR test history. Your PR dashboard.

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.

@mik-dass
Copy link
Contributor

On minikube, odo push fails but the pod has been successfully created:

It seems something was mounted on the pod's spec for which no pvc existed. On minikube I cannot reproduce it.

@openshift-ci
Copy link

openshift-ci bot commented Aug 2, 2021

@dharmit: PR needs rebase.

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.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 2, 2021
@sonarcloud
Copy link

sonarcloud bot commented Aug 2, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@netlify
Copy link

netlify bot commented Aug 2, 2021

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 92ce9ba

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/610819682f788700088db49b

😎 Browse the preview: https://deploy-preview-4908--odo-docusaurus-preview.netlify.app

@dharmit
Copy link
Member Author

dharmit commented Aug 3, 2021

Closing in favour of #4964

/close

@openshift-ci openshift-ci bot closed this Aug 3, 2021
@openshift-ci
Copy link

openshift-ci bot commented Aug 3, 2021

@dharmit: Closed this PR.

In response to this:

Closing in favour of #4964

/close

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.

@dharmit dharmit deleted the fix-4685 branch September 28, 2021 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants