-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
docs: Add a user guide for using an Ansible Operator #559
Conversation
dymurray
commented
Oct 2, 2018
•
edited
Loading
edited
- Sdk-cli-reference updates
- Ansible Operator User Guide
- Ansible Operator Project Layout
- Include link to Ansible Operator user guide from top level user guide
Hi @dymurray. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment: please wrap text to 80 chars
doc/ansible/user-guide.md
Outdated
|
||
## Customize the operator logic | ||
|
||
For this example the memcached-operator will execute the following reconciliation logic for each `Memcached` CR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you call out CR fully? (custom resource?)
doc/ansible/user-guide.md
Outdated
|
||
For this example the memcached-operator will execute the following reconciliation logic for each `Memcached` CR: | ||
- Create a memcached Deployment if it doesn't exist | ||
- Ensure that the Deployment size is the same as specified by the `Memcached` CR spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/CR spec/CRD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually refers to the spec
field on the Custom Resource. I will remove spec
for simplicity.
doc/ansible/user-guide.md
Outdated
|
||
Once this is done, there are two ways to run the operator: | ||
|
||
- As pod inside Kubernetes cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a pod inside a Kubernetes cluster
doc/ansible/user-guide.md
Outdated
Once this is done, there are two ways to run the operator: | ||
|
||
- As pod inside Kubernetes cluster | ||
- As go program outside cluster using `operator-sdk` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a Go program outside the cluster...
doc/ansible/user-guide.md
Outdated
- As pod inside Kubernetes cluster | ||
- As go program outside cluster using `operator-sdk` | ||
|
||
#### 1. Run as pod inside a Kubernetes cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run as a pod...
doc/ansible/user-guide.md
Outdated
|
||
#### 1. Run as pod inside a Kubernetes cluster | ||
|
||
Run as pod inside a Kubernetes cluster is preferred for production use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running as a pod ...
doc/ansible/user-guide.md
Outdated
|
||
#### 2. Run outside the cluster | ||
|
||
This method is preferred during development cycle to deploy and test faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is preferred during the development cycle to speed up deployment and testing.
spec: | ||
size: 4 | ||
|
||
$ kubectl apply -f deploy/cr.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just use the kubectl patch
command here to make things simpler I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the user-guide that is at the top-level they seem to suggest using apply
. Any idea what the advantages using kubectl patch
provides?
Depends on #486 |
Also depends on: #565 |
doc/ansible/user-guide.md
Outdated
Build the memcached-operator image and push it to a registry: | ||
``` | ||
$ operator-sdk build quay.io/example/memcached-operator:v0.0.1 | ||
$ sed -i 's|REPLACE_IMAGE|quay.io/example/memcached-operator:v0.0.1|g' deploy/operator.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT @dymurray I was thinking it may be better if this step was done just before doing the kubectl create -f deploy/operator.yam
below, with a short description of what and why it's being done.
I have no idea why Travis is failing... it can't be related to my documentation changes so must be transient. I'm unsure how to kick off a rebuild. |
Due to the way that travis and github handle merging for tests, there's currently an issue where if your branch is behind the master, that could cause the tests to fail (depending on what has changed). I merged a change to the test framework yesterday, so that's out of sync now. To fix this, rebase your branch to the latest master, or merge the latest master into your branch. |
@dymurray It's not related to your changes. Currently the e2e tests have to vendor the SDK from the contributor's PR branch so that any transitive dependencies from changing the core SDK pkgs are resolved by dep as well. This is a workaround for dep's inability to source from local repos. Since it's just doc changes we can technically disregard the CI but I think you should be able to rebase from the master as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some minor suggestions, but this looks great!
One other suggestion, that I think someone else already made: It's maybe a matter of preference, but I find it much easier to edit markdown that has a reasonably-limited line length. 80 characters is fine, especially since it's a default for vim. Speaking of, here's how I do the wrapping super-quickly:
- highlight several lines by hitting
V
followed by appropriate up or down keys - hit
gq
to auto-wrap. Vim does a nice job of it, and it even handles inline code comments well.
doc/ansible/user-guide.md
Outdated
|
||
#### Options | ||
**Role** | ||
Specifying a `role` option in `watches.yaml` will configure the operator to use this specified path when launching `ansible-runner` with an Ansible Role. This is the default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: "This is the default", I think it would help to be more specific and say something like "By default, the new
command will fill in an absolute path to where your role should go." That helps clarify that "default" does not imply that there is a default value in case role
is left empty in this file.
doc/ansible/user-guide.md
Outdated
|
||
``` | ||
|
||
It is important to note that we used the `size` variable to control how many replicas of the Memcached deployment we want. We set the default to `1` but any user can create a Custom Resource which overwrites the default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comma before "but" since there are two independent clauses.
Also s/which/that/ since the ending clause is important to the meaning of the sentence.
doc/ansible/user-guide.md
Outdated
Once this is done, there are two ways to run the operator: | ||
|
||
- As a pod inside a Kubernetes cluster | ||
- As a go program outside cluster using `operator-sdk` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/cluster/the cluster/
doc/user-guide.md
Outdated
@@ -2,6 +2,8 @@ | |||
|
|||
This guide walks through an example of building a simple memcached-operator using tools and libraries provided by the Operator SDK. | |||
|
|||
This guide covers the workflow of creating and deploying an operator written in Go. To read about the Ansible Operator, see the [Ansible Operator User Guide][ansible_user_guide]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put the word "Ansible" as close as possible to the beginning of the paragraph for people who are visually scanning the document for this section and link. Also as we discussed earlier today, it might be better to avoid calling it the "Ansible Operator". Maybe:
"To learn how to use Ansible to create a memcached operator, see [Ansible Operator User Guide][ansible_user_guide]. The rest of this document will show how to program an operator in Go."
Thanks a lot for that information about Travis @hasbro17 and @AlexNPavel. And thank you for reviewing @mhrivnak that was great feedback. I had no idea about that autowrap feature in vim that is awesome! |
doc/ansible/user-guide.md
Outdated
- -o | ||
- modern | ||
- -v | ||
image: "memcached:1.4.36-alpine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more thing. This should be fully qualified so it will work with non-docker runtimes. I think just prefixing it with docker.io/
will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks.