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

Jinja2 template system #278

Merged
merged 26 commits into from Dec 2, 2019
Merged

Jinja2 template system #278

merged 26 commits into from Dec 2, 2019

Conversation

jkfran
Copy link
Contributor

@jkfran jkfran commented Nov 5, 2019

Description

This PR will change completely the structure of our deployments configs, and we will no longer save Kubernetes objects directly in this repo.

Instead, we will have small files with variables values per each project, and we will generate the Kubernetes output when needed.

List of changes

  • Remove all Kubernetes manifests.
  • Jinja templates under templates folder.
  • All projects config values under sites folder.
  • konf.py script to render the templates per project
  • qa-templates-checker.sh to make sure we are not breaking the current Kubernetes objects
  • Updated qa-deploy to test projects locally
  • Updated README.md

New requirements

We have to be sure that the following system dependencies are installed:

  • python3-jinja2
  • python3-yaml

How it works

Take a look to the updated README.md file to understand how konf.py works.

QA

Download this gist and place it on the root of this branch. qa-templates-checker.sh is a script that I created in order to verify that this template system will not produce any unwanted changes to our current configs. Feel free to take a look into it before using it.

How the QA script works

Basically we are using kubectl diff to compare against all our configuration in the git branch "master". We first load all the Kubernetes objects in our system and then we do kubectl diff for each project.

QA Instructions

  • Make sure you have the latest version of the master branch (It's important because we do the diff using this branch)
  • Do a git checkout to this branch
  • run ./qa-templates-checker.sh
  • It will output all projects found and if there are changes detected or not
  • Look the output and the diff.log file

Interpret the output

We can see if there are changes by the environment (production or staging) or missing projects.
If there are changes a new file called diff.log has been generated where you can see these changes.

The generation property in the diff.log file can be ignored, it's going to appear always that we made a change in any Kubernetes object:

-  generation: 1
+  generation: 2

It doesn't make sense to keep this QA script in the repository after the merge but it has been necessary for the development and also to validate this PR

Next steps

When the approval of this PR occurs, it will be necessary to start working on the necessary changes in Jenkins

sites/canonical-com.yaml Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
sites/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

Thanks for this @jkfran, overall it looks great.

I've made a lot of comments. Please don't be put off by the number of comments, but this is a large PR with massive changes, so I suspect it will take a few rounds of review to get it ready.

@jkfran
Copy link
Contributor Author

jkfran commented Nov 13, 2019

Hi @nottrobin ,

I have made the changes we agreed. It would be great if you could check this again.

I have seen that usn.ubuntu.com have this customization to stop de container:
https://github.com/canonical-web-and-design/deployment-configs/blob/e2758acc3f0f80e7d397cd5368ecf793817e30d4/services/usn-ubuntu-com.yaml#L40

And it's going to disappear with the templates, should we keep it?

Also, I have to add the property containerPort because the docker for IRC bot is running on the port 8080 instead of 80 like the other projects:
https://github.com/canonical-web-and-design/deployment-configs/blob/e2758acc3f0f80e7d397cd5368ecf793817e30d4/services/webteam-ircbot-canonical-com.yaml#L34

Let me know what do you think about this. Thank you.

@jkfran jkfran closed this Nov 20, 2019
@jkfran jkfran reopened this Nov 20, 2019
@nottrobin
Copy link
Contributor

nottrobin commented Nov 20, 2019

Issues I've found in the diff:

staging-api

You're creating staging-api.staging.snapcraft.io, which isn't needed - staging-api.snapcraft.io should already be in the staging namespace.

ubuntu.com/blog

The generated ubuntu-com ingress config looks like this:

  rules:                                                                                                                                                                                                           
  - host: ubuntu.com                                                                                                                                                                                               
    http: &http_service                                                                                                                                                                                            
      paths:                                                                                                                                                                                                       
      - path: /                                                                                                                                                                                                    
        backend:                                                                                                                                                                                                   
          serviceName: ubuntu-com                                                                                                                                                                                  
          servicePort: 80                                                                                                                                                                                          
      - path: /blog                                                                                                                                                                                                
        backend:                                                                                                                                                                                                   
          serviceName: ubuntu-com-blog                                                                                                                                                                             
          servicePort: 80                                                                                                                                                                                          

Whereas the current one looks like this:

  rules:
  - host: ubuntu.com
    http: &ubuntu-com_service
      paths:
      - path: /blog
        backend:
          serviceName: ubuntu-com-blog
          servicePort: 80
      - path: /
        backend:
          serviceName: ubuntu-com
          servicePort: 80

I believe the order of these paths is actually very important - it only works if /blog is before /. Could we preserve this, and make sure that all sub-paths appear before the root in all configs?

ubuntu.com SSL redirect

I forget what we discussed about this, but if we're to actually release the production configs this side of Christmas (before the content-cache is applied to ubuntu.com), we probably need to keep the nginx.ingress.kubernetes.io/ssl-redirect: "false" on the ubuntu-com ingress.

Extra 360 staging service

For some reason, just for 360, we seem to be creating a threesixty-staging-canonical-com service:

--- /tmp/LIVE-804280665/v1.Service.default.threesixty-staging-canonical-com	2019-11-20 14:29:32.651462463 +0000
+++ /tmp/MERGED-686682852/v1.Service.default.threesixty-staging-canonical-com	2019-11-20 14:29:32.651462463 +0000
@@ -0,0 +1,20 @@
+apiVersion: v1
+kind: Service
+metadata:
+  creationTimestamp: "2019-11-20T14:29:32Z"
+  name: threesixty-staging-canonical-com
+  namespace: default
+  selfLink: /api/v1/namespaces/default/services/threesixty-staging-canonical-com
+  uid: e79413ee-e6a6-493b-bd00-a945b57f4509
+spec:
+  ports:
+  - name: http
+    port: 80
+    protocol: TCP
+    targetPort: http
+  selector:
+    app: 360.canonical.com
+  sessionAffinity: None
+  type: ClusterIP
+status:
+  loadBalancer: {}

This should just be called threesixty-canonical-com, like with all the other services.

@nottrobin
Copy link
Contributor

The next test I'll do is to actually try deploying all the sites with these new configs on a local microk8s. But I'll do that after we've resolved the issues in my previous comment.

@jkfran
Copy link
Contributor Author

jkfran commented Nov 21, 2019

Hey @nottrobin

staging-api

For this site we are currently not using two environments, we just have staging so I have made some modifications to allow to specify the staging domain instead of generating a new one. The qa-templates-checker.sh runs for production and staging, but we will skip production on Jenkins, right?

ubuntu.com/blog

Done :)

ubuntu.com SSL redirect

I thought we had agreed to remove it. Okay, I just added a new property: nginxSSLRedirect.

Extra 360 staging service

I fixed it, it was only 360 because it had a name assigned since the name can't be generated with the domain.

@nottrobin
Copy link
Contributor

@jkfran thanks for all that.

ubuntu.com SSL redirect

I thought we had agreed to remove it.

Yeah, I remember that. I think my thinking then was that we would be moved to the content-cache pretty soon which will support HTTPS. But actually we should probably merge this one sooner than that.

@nottrobin
Copy link
Contributor

@jkfran when I do ./qa-deploy production sites/ubuntu-com.yaml, I end up with 1 replica of ubuntu-com, but 5 of ubuntu-com-blog:

NAME                                   READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/default-http-backend   1/1     1            1           2m11s
deployment.apps/ubuntu-com             1/1     1            1           100s
deployment.apps/ubuntu-com-blog        5/5     5            5           100s

sites/canonical-com.yaml Outdated Show resolved Hide resolved
@nottrobin
Copy link
Contributor

I believe you can now delete configmaps/global-envvars.yaml and configmaps/proxy-config.yaml.

sites/README.md Outdated Show resolved Hide resolved
@nottrobin
Copy link
Contributor

@jkfran okay I tested this on microk8s with both staging and production configs for ubuntu.com and docs.ubuntu.com. They both work well, apart from the issue with too many replicas for the subpaths. I think we're very close.

Once you've addressed my points above I think we can merge this, and then we just have to very cautiously roll it out next week, check each site as we go.

sites/juju.is.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

LGTM works great 👍 thanks for all the hard work

@jkfran jkfran merged commit d6130d4 into canonical:master Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants