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

[PROPOSAL] APB self testing proposal #92

Merged
merged 7 commits into from Aug 17, 2017

Conversation

shawn-hurley
Copy link
Contributor

This document should describe the self-testing for an APB

@shawn-hurley shawn-hurley changed the title [PROPOSAL] APB self testing proposal [WIP][PROPOSAL] APB self testing proposal Aug 14, 2017
@shawn-hurley
Copy link
Contributor Author

docs/testing.md Outdated
To the run the test, you should only need to run ```oc run <name you want> --image <image> -- test```

### Writing a ```test.yaml``` action
To orchastrate the testing of an APB it is suggested to use the include_vars and include_roles.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/orchastrate/orchestrate

docs/testing.md Outdated
@@ -0,0 +1,64 @@
## Testing APB Image

### Why would we want to test the image?
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps an alternative like below

Motivation

As the ecosystem of APBs grows we want to facilitate a means for performing a basic sanity check to ensure that an APB is working as the author intended. The basic concept is to package an integration test with the APB code which will contain all of the needed parameters for a basic provision of service and check that it is up as intended.

@aweiteka
Copy link

This is good to see. I think we can probably simplify the main message: include a playbook named "test.yaml". How the author wants to organize it is up to them. Another example we may want to include is using a Makefile. The test entrypoint is still the test.yaml playbook: https://github.com/aweiteka/apb-examples/tree/mongodb/mongodb-apb

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

very good, minor formatting nit, otherwise. LGTM

docs/testing.md Outdated
*Note: we are focusing on a basic provision to start other actions should be added in the future.*

### Design
The base APB entry point will learn the test action. The test action will be a user defined playbook.
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean by "APB entry point will learn"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest creating a parallel directory for proposals here as we did in ansible-service-broker repo.

docs/proposals

docs/testing.md Outdated

### Design
The base APB entry point will learn the test action. The test action will be a user defined playbook.
* To include the testing of an APB just add the playbook ```test.yml```
Copy link
Contributor

Choose a reason for hiding this comment

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

When inline simply use a single back tick test.yml when doing blocks use the triple.

docs/testing.md Outdated
### Design
The base APB entry point will learn the test action. The test action will be a user defined playbook.
* To include the testing of an APB just add the playbook ```test.yml```
* The defaults for the test will be in the ```vars/```` directory of the playbooks.
Copy link
Contributor

Choose a reason for hiding this comment

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

single backtick around vars/. Looks like triples still work you just have 4 at the end. But still a shortcut is single for inline and triple for block.

docs/testing.md Outdated
```

### Writing a ```test.yaml``` action
To orchestrate the testing of an APB it is suggested to use the [include_vars](http://docs.ansible.com/ansible/latest/include_vars_module.html) and [include_role](http://docs.ansible.com/ansible/latest/include_role_module.html) modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

good to use those features. +1

@jmrodri
Copy link
Contributor

jmrodri commented Aug 15, 2017

Also, if this is ready for general review remove the [WIP] from the title.

@shawn-hurley shawn-hurley changed the title [WIP][PROPOSAL] APB self testing proposal [PROPOSAL] APB self testing proposal Aug 15, 2017
@djzager
Copy link
Contributor

djzager commented Aug 15, 2017

I haven't done a great deal with ansible-plabook-bundles as of yet so this question may point to a lack of experience, but here it goes. Why exactly do we need another playbook simply for providing integration testing of APBs? Should the playbooks themselves not be self-verifying?

As an example, consider https://github.com/fusor/apb-examples/blob/master/hello-world-apb/playbooks/provision.yml. I see that it has a couple of roles that are associated with it and, in particular, https://github.com/fusor/apb-examples/blob/master/hello-world-apb/roles/provision-hello-world-apb/tasks/main.yml has create hello-world service. Would it not be sufficient to have a test_vars.yaml that can be used to run, in this case, provision and verify it?

There may be work that is required to support integration testing, like having test_vars.yaml and reporting results of an integration test, but I wonder if having another playbook is necessary to achieve the end goal of APB self testing.

@tchughesiv
Copy link
Contributor

@djzager makes a good point here... but rather than recreate the wheel, we should probably be encouraging the use of built in openshift application health capabilities.
https://docs.openshift.org/latest/dev_guide/application_health.html

Liveness and readiness probes will self-test out-of-the-box and you are assured all is well when a dc rollout is successful and a route/svc is accessible.

Template references:
https://github.com/openshift/library/tree/master/official
e.g. https://github.com/openshift/library/blob/9fadaa4752b1b9a9618a5a4241cbfdbefab6f261/community/mariadb/templates/mariadb-ephemeral.json#L125-L149

@tchughesiv
Copy link
Contributor

That said, i like offering the ability to add additional/optional functional tests via this proposal...

@shawn-hurley
Copy link
Contributor Author

shawn-hurley commented Aug 15, 2017

@djzager I think that if an APB author could write their test in that way if they wanted. I still think that we need the test action to pull out test results. In this case, the author could just call the role and then continue because the role will verify itself. I think that giving an orchestration playbook to determine how and what to call will be helpful when we want to test other actions.

I think you bring up a good edge case that should be handled, which is the author does not output a test file, what should the apb test return in that case?

@aweiteka
Copy link

aweiteka commented Aug 15, 2017

Should the playbooks themselves not be self-verifying?

Most applications have a liveness probe that provides minimal self verification. Adding the same api check in the provision playbook is redundant. Adding a test playbook that does the same call is, of course, again redundant.

However, to validate an application beyond "does it have a heartbeat?" is the scope of this proposal. For example...

  • what happens when the application tries to write a file? does it have proper permissions?
  • what happens when it tries to bind to an external service and simulate load? Does it fall over?

@djzager
Copy link
Contributor

djzager commented Aug 15, 2017

However, to validate an application beyond "does it have a heartbeat?" is the scope of this proposal. For example...

what happens when the application tries to write a file? does it have proper permissions?
what happens when it tries to bind to an external service and simulate load? Does it fall over?

Fair point. I guess from my vantage point I am trying to understand:

  1. Who is running this test?
    • Is the broker going to, at some time, consume and exercise this test playbook?
  2. What are you going to do with the results?
    • Are we wanting the broker to validate APBs by running the integration test?

From afar it looks like we want to help APB developers have a measure of certainty that user's attempt to provision, bind, or update the service will be successful by providing them a means of testing their APB. Could we not encourage APB developers to write continuous integration tests against OpenShift/k8s clusters by giving examples of how that would be done? For example, in the apb-examples project having a .travis-ci.yml that brings up a cluster with the service-catalog and broker and runs a script (bash, ansible playbook, etc.) that does a provision/bind/update and verifies that it works as expected.

edit: formatting

@shawn-hurley
Copy link
Contributor Author

shawn-hurley commented Aug 16, 2017

@djzager @aweiteka @tchughesiv My initial thinking regarding this proposal is that this test playbook should be run by either APB authors or APB consumers before the APB is ever deployed to production. I was not planning on this test action to be used to test an already provisioned service.

I was envisioning a CI process that would run the APB against an ephemeral cluster that the CI process would start up.

Is there some other way that you guys were thinking this should work?

If not I think that I should add a specific limitation section to point this out:

Limitation Of Proposal

...
The intention of this design is to check that an APB passes a basic sanity check before publishing to the service catalog. The initial proposal is to be used by CI or another process to check the APB before it is published. This proposal is not meant to be testing a live service. OpenShift provides the ability to test a live service using liveness and readiness probes, which you can add when provisioning.

Any thoughts on that wording?

name: rhscl-postgresql-apb-openshift
- name: Verify the provision.
include_role:
name: verify_rhscl-postgresql-apb-openshift
Copy link
Contributor

Choose a reason for hiding this comment

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

probably with

- name: Verify the provision
  include_role:
    name: verify
    tasks_from: provision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will add to the layout and to the example

<message>
```
- Update [entrypoint.sh](https://github.com/fusor/apb-examples/blob/master/apb-base/files/usr/bin/entrypoint.sh) to wait with test-results were created.
- Create `test-retrieval-init` to follow the same pattern as [bind-init](https://github.com/fusor/apb-examples/blob/master/apb-base/files/usr/bin/bind-init).
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way I think you can do this is to put test.yaml as a local playbook alongside main.yaml. That way, we can run test.yaml at the end of the main.yaml, but before the broker bind credentials are gathered. If a test fails, the bind credentials are never gathered and the provision fails.

 - include: test.yaml

 - name: Save bind creds
   ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this approach for a couple of reasons

  1. I don't think that will work because of the case where we are not creating credentials on the provision action. (example MediaWiki).
  2. I think that having 1 solution for gathering test results and waiting on them will make it easier to understand.
  3. I would also prefer to not require an APB author to run the test during provision just to add testing. I would like them to be separate actions, where testing calls the provision action.

We do need to make sure that the bind credentials are removed when test-results are gathered so that the pod can exit.

EDIT: typo

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that will work because of the case where we are not creating credentials on the
provision action. (example MediaWiki).

From the Broker side, we still run monitorOutput even when bindable=false. So we still will capture a failure from the tests because the APB will fail. But to improve the on this, we could return a specific error code like exit 3 and pick it up on the broker side.

I think that having 1 solution for gathering test results and waiting on them will make it easier to
understand.

I think the above would meet this criteria.

I would also prefer to not require an APB author to run the test during provision just to add testing.

I disagree here. What other time would the tests run? I thought the scope was not to monitor an running app, but to test what we just provisioned works.

Plus, always running the tests on a provision are a positive. I think this lays out the groundwork for APB validations.

I would like them to be separate actions, where testing calls the provision action.

We could still accomplish this by adding a layer of logic to main.yml.

 - include: {{ action }}.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The broker should never care about the test. The broker will not be interacting with the test action.

I disagree here. What other time would the tests run? I thought the scope was not to monitor an running app, but to test what we just provisioned works.

Testing that the provision just worked should be done by liveness and readiness probes. We will be running the test action as a way to verify APB's before they are ever handled by the broker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm not seeing why this has to be inside each APB. To me, it seems like each APB will go through the same test suite: Provision, if bindable; then bind; done. If that's the case, why not make a playbook/script/framework that runs externally to an APB and calls these actions? Why do we have to ship it with each APB? And even if there are app specific validations/scripts in there, I think it would follow the same skeleton.

I think using an external framework is more pluggable. It provides an imperative top down approach to testing, which makes it easier to create more complex tests and prevents us from having to carry test playbooks across all the example APBs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I spoke with John, the goal is to get portability. Creating an external framework will come later and we can always revisit if we want to change anything.

@rthallisey
Copy link
Contributor

I see what you mean by a separate action. I guess you have to have it as an entirely different playbook, but I think you can plug into the existing monitorOutput for verification.

@fabianvf
Copy link
Contributor

@rthallisey I got the impression this was something the apb author could run for testing without involving the broker

@rthallisey
Copy link
Contributor

@fabianvf Ya I didn't quite understand what was meant by CI when I read it through initially. Now I understand this as a smoke test that an apb is functional.

### Design
The base APB entry point will be able to find and run the test action. The test action will be a user defined playbook.

* To include the testing of an APB just add the playbook `test.yml`
Copy link
Contributor

Choose a reason for hiding this comment

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

I see value in splitting out the requirement we are imposing as well as the suggestions for best practices.

Requirement:

  • test.yml exists and is intended to run provision logic at a minimal with some known values for a basic configuration. Requirement is that "oc run imagename test" is sufficient to execute the APB on the users cluster (perhaps we need to specify a namespace and syntax a little so it is accurate)

If test.yml succeeds, assume test was successful.
If test.yml fails, assume failure of test.

APB author is free to structure the test logic as they deem fit, we will provide suggestions of what we recommend yet they have freedom in adapting as long as they conform to the above.

Recommendations/Best Practices:

  • In addition to the minimal requirements of a test.yml, we recommend a verify role be used to contain logic for checking if the service provisioned is working as expected.
  • Then continue with any other recommendations

I would personally not cover much in best practices/suggestions at this phase as I assume we will learn what patterns work best in the test as we flesh out a few examples. I think it's good info to capture, just wonder if it might be premature for now.

* I will create another PR for implementation proposal
@shawn-hurley shawn-hurley merged commit 1d423ce into ansibleplaybookbundle:master Aug 17, 2017
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

8 participants