-
Notifications
You must be signed in to change notification settings - Fork 238
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
contrib/pkg/awstagdeprovision: Allow for OR filters #47
Conversation
4b2e92c
to
7bdaa6a
Compare
6e7bacc doesn't look like necessary and can be merged separate to the PR, so just drop it maybe? 7bdaa6a But i'm confused.. why force only one use the first filter? 7bdaa6a#diff-6df0d52da59598889c66f03dd6a7bc67R35 has |
7bdaa6a
to
f59dd98
Compare
I've pushed 7bdaa6a -> f59dd98, rebasing onto master so GitHub only shows the new-to-this-PR commit in the diff.
The |
@wking i'm seeing a panic when i tried to run this
|
f59dd98
to
d846d34
Compare
d846d34
to
971d238
Compare
/approve |
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.
We should allow passing of multiple key=value tags that we can OR against.
Currently you can pass multiple to AND. Did you have a command-line syntax in mind for OR? |
I was thinking that each command line argument is what we OR against. No fancy combinations of ORs with ANDs...you just get OR. Do we have a need for more complex matching than ORing against a list of key=value pairs at this time? |
The installer doesn't, but the installer doesn't use |
Yes, I'm suggesting to switch from ANDing the list of key=value pairs to ORing. @dgoodwin what do you think about changing the default away from ANDing the list to ORing (and taking away the ability to do any kind of ANDing until we find a use-case for it)? |
I suspect pretty much everyone using it would be specifying single tags, but it's a little hard to know for sure and a tiny bit risky. It's probably fine to do so but we might want to communicate it a little bit. Would a --or option seem ok? |
Are you suggesting AND-by-default, and --or will switch to 'OR all of thes key=value pairs passed as parameters'? |
Yes, but I am up for either, mild risk in changing behavior from AND to OR on folks who might be using it, but I don't think there are a lot. |
Being that the number of people using this at this time is still probably quite low, I'd argue that we should move to OR-by-default since it solves our usecase more cleanly (we can put the OR-by-default loud and clear in the '--help' output). I think we make the change now and if needed it should be easier to add AND after the fact than retrofit OR into a previously AND-by-default world. |
@wking i'm more than willing to make the proposed changes if you don't want to. you want to incorporate the OR-by-default changes? |
971d238
to
7522689
Compare
AWS docs for filters are a bit sparse, but [1] has: Combining search filters In general, multiple filters with the same key field (for example, tag:Name, search, Instance State) are automatically joined with OR. This is intentional, as the vast majority of filters would not be logical if they were joined with AND. For example, you would get zero results for a search on Instance State=running AND Instance State=stopped. In many cases, you can granulate the results by using complementary search terms on different key fields, where the AND rule is automatically applied instead. If you search for tag: Name:=All values and tag:Instance State=running, you get search results that contain both those criteria. To fine-tune your results, simply remove one filter in the string until the results fit your requirements. Unfortunately, there seems to be no way to represent OR filters in ec2's Filter structure [2]. With the approach I have here, resources that match multiple filters will be fetched multiple times, and may have parallel, racing delete attempts. But we should be robust in the face of racing delete attempt, and hopefully one of the deletes will go through before too long ;). It would be possible to adjust our locally-filtered types (which use filterObjects) to avoid this issue for those types. I may do that in follow-up work, but for now I'm treating all of our types the same way. I've added the filter information to some of the logs to help distinguish between multiple goroutines handling different filters for the same resource. I've also switched hiveutil over to use OR for multiple arguments (it used to use AND) at Joel's suggestion [3]. [1]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Filtering.html#advancedsearch [2]: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#Filter [3]: openshift#47 (comment)
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 looks good. Just one change to raise the number of goroutines to wait on.
AWS docs for filters are a bit sparse, but [1] has: Combining search filters In general, multiple filters with the same key field (for example, tag:Name, search, Instance State) are automatically joined with OR. This is intentional, as the vast majority of filters would not be logical if they were joined with AND. For example, you would get zero results for a search on Instance State=running AND Instance State=stopped. In many cases, you can granulate the results by using complementary search terms on different key fields, where the AND rule is automatically applied instead. If you search for tag: Name:=All values and tag:Instance State=running, you get search results that contain both those criteria. To fine-tune your results, simply remove one filter in the string until the results fit your requirements. Unfortunately, there seems to be no way to represent OR filters in ec2's Filter structure [2]. With the approach I have here, resources that match multiple filters will be fetched multiple times, and may have parallel, racing delete attempts. But we should be robust in the face of racing delete attempt, and hopefully one of the deletes will go through before too long ;). It would be possible to adjust our locally-filtered types (which use filterObjects) to avoid this issue for those types. I may do that in follow-up work, but for now I'm treating all of our types the same way. I've added the filter information to some of the logs to help distinguish between multiple goroutines handling different filters for the same resource. I've also switched hiveutil over to use OR for multiple arguments (it used to use AND) at Joel's suggestion [3]. [1]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Filtering.html#advancedsearch [2]: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#Filter [3]: openshift#47 (comment)
7522689
to
b1cad98
Compare
/lgtm |
Generated with: $ rm -rf "$(go env GOPATH)/pkg/dep/sources" # to avoid errors like "Unable to update checked out version: fatal: reference is not a tree" # possibly [1] $ dep ensure -update using: $ dep version dep: version : v0.5.0 build date : git hash : 22125cf go version : go1.10.3 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false I haven't reviewed all of these changes, but I want this to pull in openshift/hive@b1cad987 (contrib/pkg/awstagdeprovision: Allow for OR filters, 2018-10-18, openshift/hive#47). [1]: golang/dep#928 (comment)
Generated with: $ rm -rf "$(go env GOPATH)/pkg/dep/sources" # to avoid errors like "Unable to update checked out version: fatal: reference is not a tree" # possibly [1] $ dep ensure -update using: $ dep version dep: version : v0.5.0 build date : git hash : 22125cf go version : go1.10.3 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false I haven't reviewed all of these changes, but I want this to pull in openshift/hive@b1cad987 (contrib/pkg/awstagdeprovision: Allow for OR filters, 2018-10-18, openshift/hive#47). [1]: golang/dep#928 (comment)
Builds on #46; review that first.
AWS docs for filters are a bit sparse, but the AWS docs have:
Unfortunately, there seems to be no way to represent OR filters in ec2's
Filter
structure. With the approach I have here, resources that match multiple filters will be fetched multiple times, and may have parallel, racing delete attempts. But we should be robust in the face of racing delete attempt, and hopefully one of the deletes will go through before too long ;).It would be possible to adjust our locally-filtered types (which use
filterObjects
) to avoid this issue for those types. I may do that in follow-up work, but for now I'm treating all of our types the same way.