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

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

Closed
kadel opened this issue Dec 21, 2017 · 16 comments · Fixed by #565
Closed

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

kadel opened this issue Dec 21, 2017 · 16 comments · Fixed by #565

Comments

@kadel
Copy link
Member

kadel commented Dec 21, 2017

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

  • An easy way to define global annotations, labels #540 is one of the cases,
  • other is JSON schema with our current design it is not possible to describe Kedge spec with one JSON schema file. This will complicate doing LSP for editors.
  • we are also putting and treating controller objects as a special object that is more important than others

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:

  • clear boundaries between controller Kubernetes object and Kedge keys like appVersion (no need to mix keys like appVersion into Kubernetes controller)
  • one generic structure, no object has a special place - easier for users - better UX)
  • allows multiple controllers in one file (we would no longer need --- "hack") - better UX
  • easier to handle global labels, annotations, namespace etc... - easier to implement, better UX
  • can be described by one JSON Schema file, easier handling and generation, easier to implement smart completion for editors.

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

@kadel kadel changed the title Creating controllers section Changing structure? (Kedge v2) - Creating controllers section Dec 21, 2017
@kadel
Copy link
Member Author

kadel commented Jan 2, 2018

/cc @pradeepto @containscafeine

@pradeepto
Copy link
Member

@cdrage
Copy link
Collaborator

cdrage commented Jan 2, 2018

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:

  • Being able to use a single file
  • Better clarity

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
Copy link
Collaborator

/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
Copy link
Collaborator

concaf commented Jan 3, 2018

Introducing this would mean that we -

  • increase the level of indentation for all the root level fields
  • move away from the concept of "one application per file" that we've been sticking on to from Day 1

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 😕

@kadel
Copy link
Member Author

kadel commented Jan 3, 2018

/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
Copy link
Collaborator

cdrage commented Jan 3, 2018

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
Copy link
Collaborator

concaf commented Jan 4, 2018

Related to older discussions at #119

@concaf
Copy link
Collaborator

concaf commented Jan 4, 2018

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
Copy link
Member Author

kadel commented Jan 4, 2018

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

@kadel
Copy link
Member Author

kadel commented Jan 4, 2018

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
Copy link
Member Author

kadel commented Jan 9, 2018

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.

@kadel kadel added this to the 0.8.0 milestone Jan 9, 2018
@kadel kadel self-assigned this Jan 9, 2018
@kadel kadel added the size/XL label Jan 9, 2018
@kadel kadel mentioned this issue Jan 9, 2018
8 tasks
@cdrage
Copy link
Collaborator

cdrage commented Jan 11, 2018

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
Copy link
Member Author

kadel commented Jan 12, 2018

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
Copy link
Collaborator

cdrage commented Jan 12, 2018

@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
Copy link
Member Author

kadel commented Jan 15, 2018

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants