kedgeproject / kedge

Kedge : Simple, Concise & Declarative Kubernetes Applications
Apache License 2.0
298 stars 41 forks source link

Changing structure? (Kedge v2) - Creating controllers section #541

Closed kadel closed 6 years ago

kadel commented 6 years ago

We are starting to see some cases that are complicated because of our decisions to have controller objects on the root level.

I was thinking if it would make sense to change the structure a bit, and put controllers in separate sections the same way we have services, ingresses, configMaps....., This will also make easier to distinguish between global and local (object specific) values.

Generic structure would look like this:

<ObjectMeta>
appVersion

<kind>:
   - <ObjectMeta> merged with <kindSpec>

Root level ObjectMeta will be always merged with local ObjectMeta for given object. That way we can define the common name, global labels and annotations.

It would look like this:

name: httpd
appVersion: 0.1

deployments:
- containers:
  - image: centos/httpd

services:
- type: LoadBalancer
  portMappings: 
   - 8080:80

Benefits:

We should also introduce formatVersion, or kedgeVersion field to allow kedge spec versioning.

kadel commented 6 years ago

/cc @pradeepto @containscafeine

pradeepto commented 6 years ago
cdrage commented 6 years ago

I agree with all the points.

I actually can't find the issue at the moment. But I believe we talked about this a while while back when we were discussing the general YAML format for the files.

I totally agree that we should do deployments: key instead of controller: deployments.

Benefits:

We already use this for configMaps, secrets, etc, so why not controllers too?

I do agree however that some things should stay as a global variable. For example, including namespace as a global variable to define where to deploy.

As a release manager, we should make this a breaking change in whatever next release we do and make sure that we update all possible examples and documentation.

surajnarwade commented 6 years ago

/me also agree with all the points quoted above, @kadel @cdrage , I have a question, with current approach, we have different schema for different controller. As per above approach, will it be possible to have only one schema file which can validates all the controllers ?

concaf commented 6 years ago

Introducing this would mean that we -

I'm not sure if this betters the user experience. The technical problems that we're facing should not deter the user experience. I'm torn on this one :confused:

kadel commented 6 years ago

/me also agree with all the points quoted above, @kadel @cdrage , I have a question, with current approach, we have different schema for different controller. As per above approach, will it be possible to have only one schema file which can validates all the controllers ?

Yes with new approach it should be possible to have only one schema file.

Introducing this would mean that we -

increase the level of indentation for all the root level fields

Problem is that they are not root level fields, but fields for controller object.

move away from the concept of "one application per file" that we've been sticking on to from Day 1

We had to hack around one file per application anyway (with ---) as we want to support both approaches. We just encourage one file per application as best practices. This doesn't change that. It just brings consistency into the whole structure. And makes it a bit easier to have more stuff in one file. And we already know that it is not true that one application means one controller. This will allow multiple controllers in one file, without workarounds like --- and still do one file per application.

I'm not sure if this betters the user experience. The technical problems that we're facing should not deter the user experience. I'm torn on this one

My arguments are mostly from user experience view. Solving some of the technicals problems is just nice side-effect.

I'm actually convinced that this will be better user experience. It will be more consistent and easier to understand.

cdrage commented 6 years ago

This will also bring clarity on Kubernetes-specific parameters that are added, for example:

controller: job
name: pival
containers:
- image: perl
  command: ["perl",  "-Mbignum=bpi", "-wle", "print bpi(2000)"]
restartPolicy: Never
parallelism: 3

Would become:

name: pival

jobs:
  containers:
  - image: perl
    command: ["perl",  "-Mbignum=bpi", "-wle", "print bpi(2000)"]
  restartPolicy: Never
  parallelism: 3

# Ignore this, just an example of formatting
services:
- name: wordpress
  ports:
  - port: 8080
    targetPort: 80
  portMappings:
  - 90:9090/tcp
concaf commented 6 years ago

Related to older discussions at #119

concaf commented 6 years ago

I'm not very opposed to having the controller fields separate from other objects, e.g. deployments and jobs and services and ingresses are all at the same level, unlike our discussions previously in #119

We need to be careful and brainstorm if we're breaking any shortcuts or auto populations.

kadel commented 6 years ago

Another point for discussion should be (if we are going to change structure) how and if we are going to ensure backward compatibility.

kadel commented 6 years ago

We will discuss this issue in an online meeting on Monday, January 8th, 2018 at 13:30 GMT

Anyone interested can join via https://bluejeans.com/980816040

kadel commented 6 years ago

The conclusion is that we are doing this. There were no arguments against this. It will have a lot of benefits, the only downside is that the structure of a file will change. But we are still at the point where we don't have any users, we should do changes like this until we can.

cdrage commented 6 years ago

So going through your examples in your PR @kadel and got a bit confused when containers is being defined.. ex:

name: web

deployments:
- containers:
  - image: nginx
    # Extending the Kedge file using:
    # https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#container-v1-core
    livenessProbe:
      httpGet:
        path: /
        port: 80
      initialDelaySeconds: 20
      timeoutSeconds: 5

Wouldn't this formatting be better (in terms of controller object, etc.). At the moment we use the key containers in order to initiate the "array".

EDIT: We should use -name: nginx instead of - containers like above when showing off the examples.

Example:

name: web

deployments:
- name: nginx
  containers:
  - image: nginx
    # Extending the Kedge file using:
    # https://v1-6.docs.kubernetes.io/docs/api-reference/v1.6/#container-v1-core
    livenessProbe:
      httpGet:
        path: /
        port: 80
      initialDelaySeconds: 20
      timeoutSeconds: 5

services:
- name: nginx
  type: NodePort
  # Using a Kedge-specific 'shortcut'
  portMappings:
  - 8080:80
kadel commented 6 years ago

At the moment we use the key containers in order to initiate the "array". What do you mean?

I'm a bit confused here. deployments is an array of objects and containers is also an array of objects. Both examples in you post has an identical structure, you just added name. Or am I missing something?

cdrage commented 6 years ago

@kadel Yeah, sorry, I ended up clarifying later in the examples but I didn't update the descriptions. Yes, it should be an array, not an actual key (defining multiple). For the examples however, we should use - name: nginx instead of -containers: in order to make it cleaner / easier to understand (see above).

kadel commented 6 years ago

I agree that it looks better, but name in deployments is optional. In simples cases where deployment name matches application name, you don't have to provide a name for deployment. It would be just unnecessary duplication.