-
Notifications
You must be signed in to change notification settings - Fork 1.8k
ansible/runner: Adding ability to rotate artifacts via runner #889
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
ansible/runner: Adding ability to rotate artifacts via runner #889
Conversation
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 like that we are adding this flag to runner but I think we should expose this as a configurable option.
06be9d7
to
3ed0a85
Compare
284749e
to
ccc0515
Compare
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.
Minor nits, but overall docs are good 👍
The only other question I had was if we have guidance for when/whether to capitalize Ansible
and Ansible Operator
in our documentation.
ccc0515
to
8d43e6e
Compare
doc/ansible/dev/advanced_options.md
Outdated
| Reconcile Period | `reconcilePeriod` | time between reconcile runs for a particular CR | ansbile.operator-sdk/reconcile-period | 1m | | ||
| Manage Status | `manageStatus` | Allows the ansible operator to manage the conditions section of the resources status section. | | true | | ||
| Watching Dependent Resources | `watchDependentResources` | Allows the ansible operator to dynamically watch resources that are created by ansible | | true | | ||
| Max Runner Artifacts | `maxRunnerArtifacts` | Manages the number of [artifact directories](https://ansible-runner.readthedocs.io/en/latest/intro.html#runner-artifacts-directory-hierarchy) that ansible runner will keep in the operator container. | ansible.operator-sdk/max-runner-artifacts | 20 | |
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'd add the words "for each individual resource" or similar at the end of the sentence.
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 can't come up with a use case for needing to set this on an individual CR with an annotation. Maybe you have one in mind? Even if you're just debugging, it seems sufficient to adjust on a broader level. It may even be undesirable from a safety/security standpoint, because allowing an individual to affect this would enable an ill-meaning user to fill up the operator's filesystem with asset directories about their own CR.
What would you think about just making this a command-line flag that gets set at runtime and applies to all GVKs in the watches file?
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 think the debugging use case is important. We still really don't know what a user can change for a running operator via OLM. I think that a command line will be inaccessible. As well as the watches file.
The malicious user is interesting to me, but at the end of the day all they could do is say keep 10000000 artifacts but that means that the operator will have to run that many times. I think that this might be something worth keeping a metric for, that way cluster admins can monitor.
The same could be said for the reconcile period If I set it to .5s
and that could monopolize the worker. I think let's deal with these situations a little later but we do need this for admins to debug IMO. Thoughts?
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 assume if you're adjusting this value for troubleshooting, the next thing you need to do is exec into the pod to look at the artifact directories. Otherwise I'm not sure how this would be useful. And I assume if you can do that, you have the ability to adjust the command line args for the operator.
Do you see a different scenario?
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.
Yeah, if you use OLM it will force the deployment to look a certain way from my understanding, therefore you would have to change and set up a new CSV.
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.
Just adding my 2 cents here...
I can see a use-case for wanting to set this value on an individual CR if my operator is watching 2 applications, one that has a long start-up window (24 hours) and one that has a short start-up window (5 minutes). I prefer to maintain all artifacts files when creating an operator for a long start-up application since it is useful to find the last running state of the app if I have walked away for a few hours and it failed 5 hours ago.
I also see how this may be undesired from a security aspect because it's essentially altering the resource requirements of the application. Since changing this would require a brand new CSV from OLM I am leaning towards the annotation approach as I have used it for reconcilePeriod
in the past when debugging.
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.
Perhaps we need to find out then what the OLM user story is for affecting runtime settings of an operator. We have the same issue I assume with setting how many workers the operator should use. Other operators must have similar needs.
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 agree but let's not block this while that story is figured out.
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 can see a use-case for wanting to set this value on an individual CR if my operator is watching 2 applications, one that has a long start-up window (24 hours) and one that has a short start-up window (5 minutes). I prefer to maintain all artifacts files when creating an operator for a long start-up application since it is useful to find the last running state of the app if I have walked away for a few hours and it failed 5 hours ago.
That's a good use case. The user wants to keep all the artifacts until the thing is up and running, then start pruning them. The user still presumably has elevated permission if they're intending to look at those artifacts.
Sold. :)
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 will help a lot. Maybe a link in the user-guide to the advanced options?
|
||
This document shows the advanced options available to a developer of an ansible operator. | ||
|
||
### Watches File Options |
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.
Would like to see information about setting a finalizer for a particular GVK. Specifically I would like to see:
- An example, maybe something like:
- version: v1alpha1
group: osb.openshift.io
kind: AnsibleServiceBroker
playbook: /opt/ansible/playbook.yml
finalizer:
name: finalizer.osb.openshift.io
vars:
state: absent
- The extra-vars passed when the playbook is called via finalizer.
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.
Are you ok with a follow on for that?
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.
yup
Allow runner to manage the artifacts it creates. Setting to 20 at the moment, as a random guess on the first time. Could see the eventual need to make this configurable. Open to doing it in this PR if others feel it is necessary.
/cc @mhrivnak