Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Don't block if traitDefinition or workloadDefinition not exist #82

Closed
wonderflow opened this issue Jul 6, 2020 · 10 comments · Fixed by #267
Closed

Don't block if traitDefinition or workloadDefinition not exist #82

wonderflow opened this issue Jul 6, 2020 · 10 comments · Fixed by #267
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@wonderflow
Copy link
Member

wonderflow commented Jul 6, 2020

Now oam-k8s-runtime will check traitDefinition(for revisionAware) andworkloadDefinition(for workloadRef), but if these definition file not exist, application deploy will fail. However, the trait or workload can work without these information, so we shouldn't require definition files MUST exist.

In fact, we can automatically generate a basic definition file for those workload/trait, or we can regard there's one. Then the deploy won't fail.

For example, for ingress trait as below:

...
      traits:
        - trait:
            apiVersion: extensions/v1beta1
            kind: Ingress
            metadata:
              name: tracker-ingress
            spec:
                backend:
                  serviceName: web-ui
                  servicePort: 8080 

If we don't have a traitdefinition, we can assume it's a basic one like below:

apiVersion: core.oam.dev/v1alpha2
kind: TraitDefinition
metadata:
  name: ingresses.extensions
spec:
  definitionRef:
    name: ingresses.extensions 

All the information can be automatically generated. The main idea is to not block a normal deploy.

@wonderflow wonderflow added the good first issue Good for newcomers label Jul 6, 2020
@ryanzhang-oss
Copy link
Collaborator

I agree that a Trait/workload definition that is merely an empty envelope feels redundant. However, they are designed to act like "registration" of external CRDs on OAM runtime. One potential concern is how do we know if a typo in the appConfig results in the controller can't locate the definition of a trait/workload and creates a wrong definition by mistake?

@resouer
Copy link
Contributor

resouer commented Jul 6, 2020

Are we clear particular features in OAM rely on definition objects?

If that's clear, we can allow non-definition case and assume users know what's missing.

From my point of view, definition objects make more sense to an end-to-end app engine.

@wonderflow wonderflow added the enhancement New feature or request label Jul 7, 2020
@ryanzhang-oss
Copy link
Collaborator

what's the verdict?

@hongchaodeng
Copy link
Member

hongchaodeng commented Jul 9, 2020

I think the points that

  1. CRDs needs to be registered with OAM and
  2. users do not need to write XDefinitions

are not conflict.

One way to make it happen is that there is mega-definition that will cover CRDs not linked to explicit definitions. Just like PID 0.

@allenhaozi
Copy link

I met this error traitDefinition not found when switching from Crossplane to oam-kubernetes-runtime in my dev environment, I use Crossplane in production now.
so when I upgrade to oam-kubernetes-runtime I needed to design a low-cost way to add it

who and when to add it

the best way is that the CRD was produced, this XDefinitions was also generated at the same time, but kubebuilder can not
do it, we need to add it manually

Another way is that app-config controller is the entry to workload and trait instance generation, If it can be automatically detected and created XDefinitions, the upper usage cost can be reduced

the question is whether outside of the oam-runtime should be aware of xdefinition ?

@resouer
Copy link
Contributor

resouer commented Jul 17, 2020

who and when to add it

The platform admin is expected to register capability so users have a way to know what workloads/traits the system currently supported. And oam-k8s-runtime should report error (or warning at least) if definition object is missing.

@allenhaozi Do you think auto-generating definition objects for common k8s built-in resources help?

the question is whether outside of the oam-runtime should be aware of xdefinition ?

It seems the best answer for now is:

  1. auto-gen definitions for common k8s API resources, core and standard workloads/traits (also see: Better support for Kubernetes built-in resources #107)
  2. make oam-k8s-runtime allow "non-definition" use case but report warning if definition object is missing.

That means definitions will be left for external tools (like app engine) but a bunch of default definitions will be installed in oam-k8s-runtim.

@ryanzhang-oss
Copy link
Collaborator

LGTM, I will update the code

@wonderflow
Copy link
Member Author

Yes, "auto-generating definition objects" is the best answer now

@wonderflow
Copy link
Member Author

wonderflow commented Oct 22, 2020

After deep discussing with @hongchaodeng and @resouer , we have concluded that:

Besides "auto-generating definition objects", for CRDs that we can't automatically generate previously. We will not block app from running if no traitdefinition found, but we will output warning message to tell user that the trait will miss some capability such as applyTo check, workloadRefPath auto inject, conflictWith check, etc.

Proposal Detail

  1. Users who are configuring AppConfig can add a label trait.oam.dev/type on trait CR to indicate it's using a custom traitdefinition.
  2. If users define trait using the name and properties way, oam runtime will automatically add this label in mutate webhook. The name is the traitdefinition name.
  3. If oam runtime finally can't find the label, oam-runtime will try find definition by using CRD name of the trait CR (the old way for compatibility).
  4. If there's really no definition found, print warning message instead of block running.

Case 1: Normal Case in this design -- trait with definition have a label added, MUST have a definition

apiVersion: core.oam.dev/v1alpha2
kind: ApplicationConfiguration
metadata:
  name: myapp
spec:
  components:
    - componentName: mycomp
      traits:
        - trait:
            apiVersion: standard.oam.dev/v1alpha1
            kind: Route
            metadata:
              labels:
                trait.oam.dev/type: route
            spec:
              host: mycomp.mytest.com
              tls:
                issuerName: oam-env-default

Case 2: Compatible Case -- trait with no label.

apiVersion: core.oam.dev/v1alpha2
kind: ApplicationConfiguration
metadata:
  name: myapp
spec:
  components:
    - componentName: mycomp
      traits:
        - trait:
            apiVersion: standard.oam.dev/v1alpha1
            kind: Route
            spec:
              host: mycomp.mytest.com
              tls:
                issuerName: oam-env-default

oam runtime will check if there's a definition with name routes.standard.oam.dev in the system like below:

apiVersion: core.oam.dev/v1alpha2
kind: TraitDefinition
metadata:
  name: routes.standard.oam.dev
spec:
  workloadRefPath: spec.workloadRef
  definitionRef:
    name: routes.standard.oam.dev

If there is, runtime will use it. If not, oam runtime will complain warning or errors, but will not block app from running. (Currently, it will report error and block)

@Diddaa
Copy link
Contributor

Diddaa commented Oct 22, 2020

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants