[incubator/webpagetest-server] Add WebPageTest to k8s #3103

timothyclarke · 2017-12-19T17:57:48Z

This one currently relies on WPO-Foundation/webpagetest PR#1071

This is to run a webpagetest server instance in an existing k8s cluster. That server instance can spawn AMI's to run tests against the desired url

timothyclarke · 2017-12-20T09:44:55Z

55:4 error missing starting space in comment (comments) from #requests: was from helm create, aka upstream bug

…changed pod version

timothyclarke · 2017-12-21T09:43:17Z

/assign @mattfarina

timothyclarke · 2017-12-21T15:26:12Z

I've created an Agent chart in #3130 It would take additional work to embed. It may also take upstream pull requests to fully embed as such I did not create the optional dependency

mattfarina

This was a quick pass review. There are a few changes needed at first glance.

mattfarina · 2017-12-21T15:39:48Z

incubator/webpagetest-server/Chart.yaml

+apiVersion: v1
+description: A Helm chart for Kubernetes
+name: webpagetest-server
+version: 0.1.0


Please fill in the maintainers of the chart. Their names should be the GitHub usernames.

mattfarina · 2017-12-21T15:49:04Z

incubator/webpagetest-server/README.md

+| -------------------------------      | -------------------------------            | ---------------------------------------------------------- |
+| `image`                              | WebPageTest server image                   | `webpagetest/server:{VERSION}`                              |
+| `imagePullPolicy`                    | Image pull policy                          | `IfNotPresent`                                             |
+| `ec2Locations.enabled`               | Enables use of EC2 AMI's                   | `false`                                                     |


Can you fill in details on why ec2 is listed here? Why would a chart be spinning up AMI in one of the public clouds? Would it be better to provide direction to using the agent chart?

Webpage test uses various browsers to performance test a website.
As I'm sure you can appreciate people want to test their site using multiple browsers on different platforms rather than just chrome/firefox on linux.
To the best of my knowledge you cannot have a windows container running on a linux host as such testing with chrome/firefox/ie on windows needs a solution.

@timothyclarke First, I appreciate what webpagetest does. I'm a long time user. I remember when it launched. I appreciate the multi-platform capabilities.

That being said, Kubernetes is now adding support for Windows containers. This is a beta feature today. In the future (not expecting this now) you may be able to run windows containers to do that sort of testing.

I agree that it is desirable to have this completely within k8s, I do feel strongly that we should not enforce that limit.
The current implementation allows for both a exclusively k8s solution and a hybrid k8s / AMI solution. Granted the current implementations of both the server and agent charts, as well as the wpt agent containers are only possible if you're happy with linux browsers. If you want windows browsers further work is probably needed in the wptagent repo to create a windows container.

My personal belief is that the hybrid solution would be of more benefit to most users even when using windows containers. My reasons are as follows.

Your k8s cluster is in USA and you would like to perform some tests from Europe or AsiaPac.

Different providers would require VPN's etc to extend the k8s cluster over multiple regions / multiple providers.

Most places simply aren't that big, but may still have those requirements in an adhoc manner.

It is easier to run an AMI in a remote region than to extend your k8s cluster to that region to perform an adhoc test.

mattfarina · 2017-12-21T15:50:46Z

incubator/webpagetest-server/README.md

+This chart provides support for ingress resources. If you have an
+ingress controller installed on your cluster, such as [nginx-ingress](https://kubeapps.com/charts/stable/nginx-ingress)
+or [traefik](https://kubeapps.com/charts/stable/traefik) you can utilize
+the ingress controller to service your WordPress application.


Wordpress? Seems like a copy and paste error. Wordpress shows up a few times in this PR.

mattfarina · 2017-12-21T15:52:19Z

incubator/webpagetest-server/values.yaml

+    # ingress.kubernetes.io/force-ssl-redirect: "true"
+    # ingress.kubernetes.io/whitelist-source-range: '127.0.0.1/32,192.168.0.0/24'
+  tls:
+    # Secrets must be manually created in the namespace.


If you look at the Wordpress chart you'll see a pattern for passing in or letting Helm create the secret for you. Could you take a look at that pattern?

I did a helm create, then copied the readme from the chart I had previously made a pull request on.
Is the issue

I've added more example ingress annotations. I can remove those, however they are provided as an example due to the potential cost issue with using AMI's

Is it the structure of the tls section. This is what helm create produces as a template.
If it's the latter I think the issue should be raised in the helm project so that the helm create command produces the desired starting point.

mattfarina · 2017-12-21T15:53:58Z

incubator/webpagetest-server/values.yaml

+replicaCount: 1
+image:
+  repository: webpagetest/server
+  tag: latest


Can you use a tagged version? IfNotPresent for a pullPolicy with a latest tag can cause problems.

@pmeenan Can we get more tags on the webpagetest docker images? Currently there are only latest, release and fbpwa (which is 7 months old).

@mattfarina Can you clarify my understanding of the IfNotPresent and latest interaction. My understanding is that as soon as the cluster downloads a tag it will keep using that tag. In short If I get latest today and then it is rebuilt tomorrow, then I redeploy the day after I will still have today's latest, not tomorrow's latest. Do you request I change this to Always ?

@timothyclarke latest presents a number of challenges with IfNotPresent. For example, what if the test and production clusters are different. They could end up downloading and keeping two different versions. Imagine being able to debug issues there? Or, what if you want to use a newer version or even know what version you're using? The tag handling we've more recently talked about deals with this. Just want to make sure we share why it matters.

Can you update this to a tag in a similar manner to the agent?

pavelnikolov · 2017-12-27T11:26:41Z

incubator/webpagetest-server/values.yaml

+##
+persistence:
+  enabled: true
+  ## wordpress data Persistent Volume Storage Class


Does this comment need to mention wordpress? Copy/paste mistake?

timothyclarke · 2017-12-27T15:10:35Z

/assign mattfarina

mattfarina · 2018-01-09T18:52:07Z

@timothyclarke Thanks for being patient as we talked this out. We did just that on the charts chat call today where several maintainers weighed in. We're ok with the ec2 bits and the general flow here. I'm going to go through with one last nit-pick pass.

/ok-to-test

mattfarina

Thanks for being patient. This chart is almost there and what I had left were some nits.

mattfarina · 2018-01-09T18:56:36Z

incubator/webpagetest-server/templates/deployment.yaml

@@ -0,0 +1,63 @@
+apiVersion: extensions/v1beta1


As with the agent version of this chart, can you update to apps/v1beta2 for the API version? Link to the docs is in the agent PR comment on this.

mattfarina · 2018-01-09T18:57:48Z

incubator/webpagetest-server/values.yaml

+replicaCount: 1
+image:
+  repository: webpagetest/server
+  tag: latest


Can you update this to a tag in a similar manner to the agent?

mattfarina · 2018-01-09T19:13:39Z

incubator/webpagetest-server/templates/secrets-user-details.yaml

@@ -0,0 +1,37 @@
+{{- if .Values.ec2Locations.enabled }}
+apiVersion: v1
+kind: Secret


I'm glad you're passing these credentials as a secret. One pattern we would like to see is allowing someone to set the secret outside the chart if they want. If they want the chart to create it they can have that as an option. For example, see the wordpress chart with tls secrets... https://github.com/kubernetes/charts/blob/master/stable/wordpress/values.yaml#L145

Could you look at something like this?

…ption of user created secret

timothyclarke · 2018-01-12T11:40:29Z

Dockerhub is taking it's time creating the repo / build. As soon as I've got it working there I'll update

…t' tag

timothyclarke · 2018-01-12T16:11:41Z

OK everything should be good now.
@mattfarina when you have time please review etc.

Thank you

mattfarina · 2018-01-12T19:00:10Z

/lgtm

k8s-ci-robot · 2018-01-12T19:00:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattfarina, timothyclarke

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [mattfarina]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

timothyclarke added 2 commits December 19, 2017 17:52

[incubator/webpagetest-server] initial commit

884df1e

[incubator/webpagetest-server] additional readme content

1e57059

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 19, 2017

[incubator/webpagetest-server] Whitespace fix for linting

39dba4e

[incubator/webpagetest-server] Added PVC for storing results

0fde6a7

timothyclarke changed the title ~~[incubator/webpagetest-server] initial commit~~ [incubator/webpagetest-server] Add WebPageTest to k8s Dec 20, 2017

[incubator/webpagetest-server] Commits are in latest, not server, so …

96b87c6

…changed pod version

k8s-ci-robot assigned mattfarina Dec 21, 2017

webpagetest-server: Updated readme to add "Server"

36ea1ac

timothyclarke mentioned this pull request Dec 21, 2017

[incubator/webpagetest-agent] initial commit #3130

Merged

mattfarina suggested changes Dec 21, 2017

View reviewed changes

mattfarina assigned timothyclarke and unassigned mattfarina Dec 21, 2017

timothyclarke added 3 commits December 22, 2017 10:14

WebPageTest Server: Updates per code review comments

d301bee

Added namespaced templates per request in #3130

1fa7efe

WebPageTest Server namespaced chart

d1f0957

pavelnikolov reviewed Dec 27, 2017

View reviewed changes

k8s-ci-robot assigned mattfarina Dec 27, 2017

webpagetest-server Updated persistance comment in values.yaml

9dca5d4

timothyclarke mentioned this pull request Jan 9, 2018

[Proposal] New Docker startup setup catchpoint/WebPageTest.agent#70

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 9, 2018

mattfarina reviewed Jan 9, 2018

View reviewed changes

[incubator/webpagetest-server] Updated deployment for app/v1beta2 & o…

52569e9

…ption of user created secret

webpagetest server: changed image until official repo uses non 'lates…

7eed2a6

…t' tag

mattfarina approved these changes Jan 12, 2018

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2018

k8s-ci-robot merged commit 5092523 into helm:master Jan 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[incubator/webpagetest-server] Add WebPageTest to k8s #3103

[incubator/webpagetest-server] Add WebPageTest to k8s #3103

timothyclarke commented Dec 19, 2017

timothyclarke commented Dec 20, 2017

timothyclarke commented Dec 21, 2017

timothyclarke commented Dec 21, 2017

mattfarina left a comment

mattfarina Dec 21, 2017

mattfarina Dec 21, 2017

timothyclarke Dec 22, 2017

mattfarina Jan 9, 2018

timothyclarke Jan 12, 2018

mattfarina Dec 21, 2017

mattfarina Dec 21, 2017

timothyclarke Dec 22, 2017

mattfarina Dec 21, 2017

timothyclarke Dec 22, 2017 •

edited

mattfarina Jan 9, 2018

mattfarina Jan 9, 2018

pavelnikolov Dec 27, 2017

timothyclarke commented Dec 27, 2017

mattfarina commented Jan 9, 2018

mattfarina left a comment

mattfarina Jan 9, 2018

mattfarina Jan 9, 2018

mattfarina Jan 9, 2018

timothyclarke commented Jan 12, 2018

timothyclarke commented Jan 12, 2018

mattfarina commented Jan 12, 2018

k8s-ci-robot commented Jan 12, 2018

[incubator/webpagetest-server] Add WebPageTest to k8s #3103

[incubator/webpagetest-server] Add WebPageTest to k8s #3103

Conversation

timothyclarke commented Dec 19, 2017

timothyclarke commented Dec 20, 2017

timothyclarke commented Dec 21, 2017

timothyclarke commented Dec 21, 2017

mattfarina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyclarke Dec 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyclarke commented Dec 27, 2017

mattfarina commented Jan 9, 2018

mattfarina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyclarke commented Jan 12, 2018

timothyclarke commented Jan 12, 2018

mattfarina commented Jan 12, 2018

k8s-ci-robot commented Jan 12, 2018

timothyclarke Dec 22, 2017 •

edited