design: resource dependency in OAM

[hongchaodeng]

Coming from oam-dev/spec#326.
Similar to #23.

Since it's no change on OAM spec and more implementation focused, we are moving the design into here.

[wonderflow]

I think this CRD need to be named as DataInput. Because it is bind to components who need to consume these data.

[hongchaodeng]

The name DataInput is already taken in the input resource side. To correspond to that, this is named to DataOutput. I don't think we should change that.

[hasheddan]

This is a little bit difficult to understand because we haven't implemented support for ServiceBindingTrait outside of https://github.com/oam-dev/trait-injector, and it is not immediately clear how compatible it is if installed using trait-injector. Maybe we should prioritize adding initial support for it here?

[hongchaodeng]

Maybe we should prioritize adding initial support for it here?

This is just an example use case. The dependency feature is not tightly bound to it.

[hasheddan]

Makes sense :) It just makes it a little hard to reason about when the example is not currently supported, especially for folks that are not familiar with trait-injector. I get the idea though 👍

[hasheddan]

If a user does not provide a DataOutput trait, could they provide a parameter value here to set this explicitly?

[hongchaodeng]

Currently parameterValue does not support fields like this.
We model it as a trait to avoid changing the spec first.

[negz]

I think what @hasheddan is asking is whether this parameter acts like a "normal" parameter if there is no corresponding DataOutput. i.e. Will the user be able to supply a parameter value for mysql-conn-secret explicitly when they write their ApplicationConfiguration?

[hasheddan]

Suggested change

	fieldPath: "status.connSecret"
	fieldPath: "spec.writeConnectionSecretToRef"

https://doc.crds.dev/github.com/crossplane/provider-alibaba/config/crd/database.alibaba.crossplane.io_rdsinstances.yaml#spec-writeConnectionSecretToRef

[hongchaodeng]

Well, this is an example to show field passing.
We have many use cases similar to this that passes secret name from status, which also means secret names are not available at the beginning.

[hasheddan]

In this case I am not sure how much using the DataOutput is helping the end user. They still have to be aware of the connection secret field of the RDSInstance which is a level lower than the Component they are using. It seems like a better experience would be for the Component author to surface a secretName parameter for the my-rds and my-web Components, then the end user could just provide the same value for each. The user would only be required to know about the components rather than knowing about the underlying resource types, which seems like the abstraction that we want at the ApplicationConfiguration level.

[hasheddan]

Consider the experience here vs this from the quick start with service tracker:

spec:
  components:
    - componentName: data-api-database
      parameterValues:
        - name: database-secret
          value: tracker-database-secret
    - componentName: data-api
      parameterValues:
        - name: database-secret
          value: tracker-database-secret

[hongchaodeng]

I totally understand. But this is something we could not enforce users to adopt. For example, in Alibaba cloud, the ID of RDS would not be available at the beginning until RDS instance is created. This has to be passed from the status.

[hongchaodeng]

Updated to address comments and improve sentences.

[negz]

I'm finding it a bit hard to understand this proposal. Is the below understanding correct?

1. We'll overload the existing concept of component parameters to also reference a DataOutput
2. The AppConfig controller will discover params that are actually data inputs (i.e. references to a DataOutput).
3. The AppConfig controller will will wait until the referenced DataOutput is ready, then inject the value from the DataOutput into the component's workload template before rendering the workload.

If so, my primary thoughts are:

• What controller will be responsible for writing DataOutput CRs?
• Why do we need to annotate workloads with this information after they're created?
• The implicit param to data output relationship feels potentially surprising to users. Could we make it explicit with a small, backward compatible spec change?

[negz]

Could we make this more explicit, for example by adding an optional type field to the parameter? Something like:

parameters:
- name: mysql-conn-secret
  type: DataOutput
  fieldPaths: ["spec.writeConnectionSecretToRef.name"]

I believe making the field optional, with the default behaviour unchanged, would make it a backward compatible change to the spec.

[negz]

You state that this needs to be marshalled and put into an annotation key. Could you explain (ideally in the document) why? What controller would read this annotation after it is written?

[negz]

I think what @hasheddan is asking is whether this parameter acts like a "normal" parameter if there is no corresponding DataOutput. i.e. Will the user be able to supply a parameter value for mysql-conn-secret explicitly when they write their ApplicationConfiguration?

[negz]

I'm having a hard time understanding what CR(s) you're proposing will support this datainputs field. Does it ever exist in the YAML format you show here, or is it only ever persisted as an annotation?

[hongchaodeng]

Hi @negz @hasheddan . Thanks for the comments. Let me answer them all here:

Is the below understanding correct?

1. We'll overload the existing concept of component parameters to also reference a DataOutput
2. The AppConfig controller will discover params that are actually data inputs (i.e. references to a DataOutput).
3. The AppConfig controller will will wait until the referenced DataOutput is ready, then inject the value from the DataOutput into the component's workload template before rendering the workload.

Correct.

What controller will be responsible for writing DataOutput CRs?

The OAM user will write it.
Let me give an example. In a PaaS platform like Elastic Beanstalk, it will ship the web application and RDS database together. The PaaS platform then prepares an ApplicationConfiguration yaml file with components (incl. Parameters) and traits (incl. DataOutput).

Why do we need to annotate workloads with this information after they're created?

Parameter is Component specific. Nonetheless, we also have use cases for Component2trait, trait2trait, and Parameter doesn't work for them. In fact, their underlying implementation are the same.

As a result, the generic resource dependency engine would make most sense here. By separating the core implementation from the representation layer, we could reuse the core service and have different API layers.

The core engine includes the dependency graph, DataInputs, DataOutputs. The representation layer includes Component parameters and other similar APIs.

The implicit param to data output relationship feels potentially surprising to users. Could we make it explicit with a small, backward compatible spec change?

The main point is not to change spec at all to move forward and validate the idea first. In the community meeting that we discussed and agreed to have implementation first and validate the ideas before making any spec changes.

You state that this (DataInputs) needs to be marshalled and put into an annotation key. Could you explain (ideally in the document) why? What controller would read this annotation after it is written?

This is a compromise for initial implementation. For experimental features we will all do it in annotations in oam-kubernetes-runtime. Once validates, these schema will be added to the spec.

I think what @hasheddan is asking is whether this parameter acts like a "normal" parameter if there is no corresponding DataOutput. i.e. Will the user be able to supply a parameter value for mysql-conn-secret explicitly when they write their ApplicationConfiguration?

Yeah of course.
However, there is another background information that we are actually dropping use cases for parameterValues. See this. We would model this as traits.

I'm having a hard time understanding what CR(s) you're proposing will support this datainputs field. Does it ever exist in the YAML format you show here, or is it only ever persisted as an annotation?

It is persisted as an annotation.
Put it as yaml for better reading.

[negz]

For experimental features we will all do it in annotations in oam-kubernetes-runtime.

I'm concerned about this approach.

I've heard two reasons that we may want to add annotations that contain serialised JSON data. The first reason is that we have CRs that are in our control, but whose schema we feel we cannot update without also updating the OAM spec. This seems like we're working around a shortcoming of the OAM spec - namely that it requires us to version all of our API types monolithically. I believe it would be better to allow API types to support fields that are not defined by the spec, and to determine a strategy that allows us to iterate on individual API types at the runtime level without needing to rev the entire spec. Using annotations seems to have all the downsides of allowing runtimes to add their own fields, and none of the upsides (automatic API server schema validation, readability).

The second reason I've heard is that we increasingly want to attach OAM related metadata to CRs that are not aware of OAM. I wonder if there's a reason other than convenience that we have not pursued the alternative approach @ryanzhang-oss mentioned in which we would implement a separate type that stored this metadata? It seems obvious to me that annotations are a suboptimal place to store anything more than simple string values - even in this PR we felt it was better to represent the data types as YAML because the serialised JSON is less readable.

Parameter is Component specific. Nonetheless, we also have use cases for Component2trait, trait2trait, and Parameter doesn't work for them. In fact, their underlying implementation are the same.

I see. In that case, could we find a place next to rather than inside the CR that declares data inputs for each of these cases?

If I understand correctly, some entity must process the data inputs before the annotated CR is ever created, right? You include a MyWorkload example in this design - am I correct in my understanding that:

• We would add the core.oam.dev/datainputs annotation to the MyWorkload template inside the Component (i.e. the spec.workload.metadata.annotations field path). We do this because we want to block creation of a MyWorkload until its data inputs are resolved.
• After the MyWorkload is created the controller that reconciles it would not be aware of the core.oam.dev/datainputs annotation, so it doesn't really need to be there after creation.

If this is the case, and if we ignored our concerns about OAM spec changes for a moment couldn't we address any combination of component to component, trait to trait, component to trait, etc, etc at the AppConfig level? For example:

apiVersion: core.oam.dev/v1alpha1
kind: ApplicationConfiguration
metadata:
  name: example
spec:
  components:
  - componentName: my-containerized-workload
     parameterValue:
     - name: database-secret
       valueFrom:
         dataOutputName: my-database-output
   - componentName: my-etcd-cluster-workload
      traits:
      - trait:
          apiVersion: example.oam.dev/v1alpha1
          kind: EtcdBackup
          metadata:
            name: very-reliable-backup
          spec:
            # etc...
        parameters:
        - valueFrom:
            dataOutputName: etcd-cluster-secret
          toFieldPath: spec.etcdSecret

[negz]

Introducing conditions here is probably a good idea, but the point I attempted to make in my previous comment was that we should avoid the Status bool field, and instead use Status StatusType, where StatusType is a subtype of string with values True, False, Unknown. This is described in the API conventions section on status conditions.

[negz]

This field should be named resourceRef per API conventions.

[negz]

I think we move this into the below ResourceReference struct. API conventions state that references to other objects should be strict subsets of ObjectReference. ObjectReference includes a fieldPath, so ResourceReference would still be API conventions compliant.

[hongchaodeng]

Hi @negz. Thanks for the reply.

I would prefer NOT to make DataInput as another CR which would involve another resource ref as below:

kind: DataInput
spec:
  resourceRef: # resource that needs input
  fromDataOutput: ...

I do agree with you on the ParameterValue solution. Could we make it a step-by-step progress? For example, in alpha we make it as annotations to validate the idea. In beta after like two or three months, we make it into the spec in ParameterValue. How does it sound?

The major blocker is that the spec is just released as v1alpha2. It might take another cycle and we don't want to make major changes to it for now.

[negz]

For example, in alpha we make it as annotations to validate the idea. In beta after like two or three months, we make it into the spec in ParameterValue. How does it sound?

Should we discuss this at the next community meeting - or setup a call later next week (after Weds)? I apologise for missing the discussion that happened at the last one! I agree that we shouldn't block on updating the spec, but I think we can find a better solution than using annotations, and I don't want to set a precedent if we agree that we don't really like that solution long term.

[hongchaodeng]

Sure. Will set up a meeting next week to involve @vturecek and @artursouza too

[artursouza]

The latest version for this proposal (using new CRDs for DataOutput) is super confusing. As an app developer I would simply use another framework if I was comparing solutions to model my app on K8s. Just my opinion here.

On practical terms: if we need all this workaround to make this work without changing the spec, we should probably step back and change the spec to have a first-class experience. Also, the way we reference params via fieldPaths is really confusing, even more now - where we mix how things are referenced (from consumer to resource vs from resource to consumers).

Edit: this changes the spec, but using traits is making it complex and not simpler. My feedback is about a deeper change to the spec to have data input/output as a first-class feature.

[wonderflow]

@artursouza Totally agree if we could change spec and make it as experimental feature on v1alpha2. For component params(parameters), we are planning to remove it. As it not only confusing on fieldPath, but also confusing on component versioning mechanism.

[hongchaodeng]

After discussion with @artursouza , we will also change the DataOutput to be (experimental) fields.

[resouer]

A offline discussion, how about we add experimental field in OAM spec like this:

Attribute	Type	Required	Default Value	Description
workload	Workload	Y		Declaration of settings that should be passed to the workload runtime
parameters	[]Parameter	N		The component's configuration options.
policies	[]Policy	N		EXPERIMENTAL The operational policies for this component.

[hongchaodeng]

how about we add experimental field in OAM spec like this:

Sounds great!

[wonderflow]

LGTM, this proposal looks really more comfortable when we can change in spec instead of using trait.

[artursouza]

Much better. The fieldPath thing is still something to be discussed further but in another proposal.

[resouer]

To echo @ryanzhang-oss 's comment, I'd propose we put data input/output model alongside with parameters/parameterValues, then we will gradually deprecate the later feature in the upcoming releases. In that case, we will have a consistent model for both component and trait in dependency description. My assumption is developer doesn't need to declare parameters in component to make data input/output model work.

But what I'm not sure is: who should declare data input/output? In this proposal, it's operator/system. I have feeling that maybe it should be developer? In that case, we should define a Application or ComponentGroup first and move data input/output. Let's discuss this part.

Note that, I still think it's a valid case for developer to claim parameters as "overridable", but it's very tricky to guarantee this in implementation (AppConfig controller). I'd suggest we merge this use case into "policies" and enforce it by policy engine (if possible).

[rynowak]

But what I'm not sure is: who should declare data input/output? In this proposal, it's operator/system. I have feeling that maybe it should be developer? In that case, we should define a Application or ComponentGroup first and move data input/output. Let's discuss this part.

100% agree this needs more thought.

I'd make the case that it's the developer - developers already think of applications this way and already solve these problems.

Ask the question - if I'm the developer - how do I run my application with dependencies today to test a change? They are likely writing a mock, using docker-compose, or just launching multiple containers/processes in on their own machine. For a database, they are likely running one in a container or against a staging instance in the cloud.

You can't get very far as a developer without knowing the identity of the other services your component talks to. Most often (based on my customer conversations) the components they communicate with are also written by them/their-team.

It's a big win if we can keep these concepts high level where possible - "I need to talk to weather-api" allows us to build a better system than "I need to declare a parameter to set an environment variable so that someone else with a different job can satisfy a binding from another component...".

[resouer]

We had a offline but similar discussion around the persona here. So one reasonable approach is:

kind: Component
spec:
  workload:
      ...
  dataOutputs:
  - name: etcd-conn-secret
     ...

kind: ApplicationConfiguration
spec:
  components:
  - componentName: my-etcd-cluster
    traits:
    - trait:
        apiVersion: example.io/v1alpha1
        kind: EtcdBackup
        metadata:
          name: my-etcd-backup
          ...
      dataInputs:
      - from: etcd-conn-secret
        toFieldPaths:
        - spec.connSecret

The idea is Component and Trait (AppConfig) has its own data inputs/outputs claim individually. So Dev will handle data inputs/outputs for components and Ops will handle that for traits.

/cc @hongchaodeng

[hongchaodeng]

Shouldn't Component's data outputs also sit in ApplicatoinConfiguration:

kind: ApplicationConfiguration
spec:
  components:
  - componentName: my-etcd-cluster
    dataOutputs:
      - name: etcd-conn-secret
        ...
    traits:
    - trait:
        apiVersion: example.io/v1alpha1
        kind: EtcdBackup
        metadata:
          name: my-etcd-backup
          ...
      dataInputs:
      - from: etcd-conn-secret
        toFieldPaths:
        - spec.connSecret

[resouer]

I assume @rynowak 's point is component dependency is knowledge of developers. So the components' data inputs/outputs should be part of kind:component object.

[hongchaodeng]

We can definitely add data inputs in Component definition.
But in this proposal, we would solve the Component configuration part first.
For the use cases described here, it is the operator role that knows the relationship between Components and traits.

[zzxwill]

dependency graph means DAG(directed acyclic graph)?

[hongchaodeng]

yes.

[rynowak]

There's a big semantic difference between "this component cannot start until its dependencies are fully ready" and "this component cannot start until required information about its dependencies is known". The former category is close to the workflow engine example, and the latter covers things like URIs and database connection strings.

This line seems to undo that difference. Would it be better to explicit about the cases where readiness ordering is the requirement?

---

I would argue that the data-passing dependency case is incredibly common (every database, every REST api), and the actual need for the readiness case is somewhat rare. Relying on readiness of downstream component for an application developer provides false security - what if the other service has an outage? It leads you to believe that the infrastructure provides a guarantee about availability - it doesn't 😆

[hongchaodeng]

I totally understand your point, and this issue has been discussed in last few community meetings.

The reason is that we can't enforce all applications to be fault tolerant and keep retrying. There are many scenarios discussed below that needs such ordering capability. But they can tolerate if the service has a later outage which is infrequent.

[rynowak]

Is this really sufficient to model with ApplicationConfiguration? Just adding ordering to OAM doesn't seem like a good replacement for a workflow engine. Looking at Argo Workflow there's a ton of features it offers.

[rynowak]

I think there are some good examples here already (4 & 5) that describe scenarios where ordering matters. The workflow example seems like it's better solved by making it possible to just use a workflow engine (possibly as an extended OAM workload type).

[hongchaodeng]

This proposal gives a representation/model to do the work in OAM.
Of course, the underlying runtime can implement it using external workflow engine.

[rynowak]

We should also consider traditional REST APIs (need the URIs of my other services) as a keystone scenario for any depenedency use case. It's exceedingly common (we already have a sample) and it seems like we could avoid the incredible level of coupling and boilerplate it requires today: https://github.com/oam-dev/samples/blob/master/2.ServiceTracker_App/ApplicationConfiguration/tracker-app-config.yaml#L34

The proposal here seems like it could already help with this, but I want to make sure we're considering it as a headline scenario.

[hongchaodeng]

That's the purpose of this proposal.

[rynowak]

But what I'm not sure is: who should declare data input/output? In this proposal, it's operator/system. I have feeling that maybe it should be developer? In that case, we should define a Application or ComponentGroup first and move data input/output. Let's discuss this part.

100% agree this needs more thought.

I'd make the case that it's the developer - developers already think of applications this way and already solve these problems.

Ask the question - if I'm the developer - how do I run my application with dependencies today to test a change? They are likely writing a mock, using docker-compose, or just launching multiple containers/processes in on their own machine. For a database, they are likely running one in a container or against a staging instance in the cloud.

You can't get very far as a developer without knowing the identity of the other services your component talks to. Most often (based on my customer conversations) the components they communicate with are also written by them/their-team.

It's a big win if we can keep these concepts high level where possible - "I need to talk to weather-api" allows us to build a better system than "I need to declare a parameter to set an environment variable so that someone else with a different job can satisfy a binding from another component...".

[hongchaodeng]

Update the proposal to use native fields dataInputs and dataOutputs after syncing offline.
We will discuss this in tomorrow's community meeting again.

[zzxwill]

/lgtm

[wonderflow]

I think this proposal is almost good, but we'd better give some convert ability in Service Binding trait.

For example, convert key/value in K8s secret to status. And let datainput/dataoutput to pass through.

   kind: ApplicationConfiguration
    spec:
      components:
      - componentName: my-web
        dataInputs:
        - fieldPaths: [] # This data input is for ordering purpose only
          valueFrom:
            dataOutputName: my-secret-key
        traits:
        - trait:
            apiVersion: core.oam.dev/v1alpha1
            kind: ServiceBinding
            metadata:
              name: my-secret-binding
            spec:
              from:
                secret:
                  name: my-secret
              to:
                status: true                
          dataOutputs:
            - name: my-secret-key
              fieldPath: status.map.key

[hongchaodeng]

Merging this proposal as it gets majority consensus (Alibaba+Microsoft).
I have tried to ping more folks for review but haven't received any further feedback yet.
We are going to do implementation as it is.
Any further feedback is welcome and I will adapt implementation accordingly.