Compute V2: implement server tags filtering support #1759

Fedosin · 2019-10-23T14:47:14Z

Server tags support was added in Nova API since 2.26, so we can implement it here as well.

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/nova/blob/3b54979ae02cc74bcf215713c16916bbf4131443/nova/api/openstack/compute/schemas/servers.py#L645-L651
https://docs.openstack.org/api-ref/compute/?expanded=list-servers-detail,show-server-details-detail#list-servers

coveralls · 2019-10-23T14:59:22Z

Coverage increased (+0.3%) to 77.2% when pulling 775d353 on Fedosin:server_tags into 96d83b9 on gophercloud:master.

theopenlab-ci · 2019-10-23T15:53:46Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 06s
gophercloud-acceptance-test : FAILURE in 1h 03m 35s
gophercloud-acceptance-test-ironic : SUCCESS in 38m 31s

theopenlab-ci · 2019-10-23T20:02:36Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 04s
gophercloud-acceptance-test : SUCCESS in 2h 33m 23s
gophercloud-acceptance-test-ironic : SUCCESS in 27m 38s

jtopjian · 2019-10-24T01:45:59Z

@Fedosin Thank you for submitting this. The referenced issue #1669 has been closed, but I'm OK with re-opening it for this PR. However, there are a few things that need worked out.

Per our contributor tutorial we need to see links to the actual API service implementation of listing tags, not just documentation. To save some time, here's the needed information.

Next, you are adding a field of Tags to the servers.Server result struct. Since tags was added in a microversion, it needs to be handled separately. We already have support for extracting tags here. Please see our documentation on microversions for more details. It would be best if this was reverted.

@ozerovandrei on that note, I believe this can be removed since we already have this functionality in the servers' microversions.go file. I can open a PR to fix this and then this PR can be rebased once that's done.

On the core topic of this PR, the additions to the ListOpts is correct and follows our microversion guidelines. We're not able to use omitempty with ListOpts, so the current implementation is fine.

I'll open a PR to fix the tags extension and then this PR can be rebased. Once that's done, this should be ready to go.

Please let me know if anyone has any questions.

jtopjian

@Fedosin Thank you again for your work on this.

I've merged #1760 which resolves the issues I previously mentioned.

Please see the above comments and discussion and let me know if you have any questions.

jtopjian · 2019-10-24T14:20:25Z

acceptance/openstack/compute/v2/servers_test.go

-	}
-
-	serverWithTags := serverWithTagsExt{}
+	serverWithTags := servers.Server{}


Per other comments, please revert this change and rebase. After the rebase, I believe this file can go unmodified. Let me know if you have any questions.

jtopjian · 2019-10-24T14:21:07Z

openstack/compute/v2/servers/requests.go

+	NotTags []string `q:"not-tags"`
+
+	// NotTagsAny filters on specific server tags. At least one of the tags must be absent for the server.
+	NotTagsAny []string `q:"not-tags-any"`


Just one nit with these in-line comments: can you add:

// This requires the client to be set to microversion 2.26 or later.

To each field?

jtopjian · 2019-10-24T14:21:38Z

openstack/compute/v2/servers/results.go

+
+	// Tags is a list of server tags. Tags are arbitrarily defined strings
+	// attached to a server.
+	Tags []string `json:"tags"`


Per the other comments/discussion, please revert this as there's already a way to achieve this. Let me know if you have any questions with the current implementation, though.

Fedosin · 2019-10-24T15:49:27Z

I made this as an extension to ListOpts, plus added a test.
We can always avoid making the extension and put these parameters directly in ListOpts, but as you said it doesn't comply with the rules.

jtopjian · 2019-10-24T16:41:42Z

We can always avoid making the extension and put these parameters directly in ListOpts

I'm so sorry for the confusion. Adding the parameters directly in ListOpts is the right solution. This is outlined under the "New Request Fields" section in the microversions doc, but it is terribly confusing.

There are three forms of an API amendment/extension that we try to deal with:

New API functionality that is added via an extension.

Nova used to have more well-defined/first-class extensions but collapsed them all into the core code, but other services (such as Neutron) still have a concept of extensions. So between that and the existing openstack/compute/v2/extensions packages, new API features that don't modify the existing request and response bodies are considered an extension.

New response fields that were amended via a microversion

These go under microversions.go in a way that doesn't modify the API response. The reason for this is because of Go's concept of a "zero-value".

We could add the field Tags to the servers.Server struct, but there is no way of determining if the server response doesn't support tags or if there are no tags in the response body. For "tags" specifically, this isn't a huge deal, but there might be other fields where this distinction is important.

So we use the concept of composing new structs and the developer can add their own conditional logic on when they want to do this, with the thinking that the developer will know when they need to support a microversion and when the response should have a microversion-amended response.

New request options that were amended via a microversion

Because we have control over how the request body is sent, we can omit fields that are sent in a request body. So it's possible for us to support microversion amendments in-line of the original structs and functions in the requests.go files.

It would be great if we could do this for responses, but as mentioned above, the zero-value makes it indeterminable, so that's why there's the extra work involved.

You can see some other examples of existing microversion amendments here:

https://github.com/gophercloud/gophercloud/blob/32225edcb95e34f926834ae229ccf2d88de83c68/openstack/compute/v2/extensions/extendedserverattributes/microversions.go

gophercloud/openstack/compute/v2/extensions/servergroups/requests.go

Lines 30 to 36 in 3b05c00

    
           // Policy specifies the name of a policy. 
        
           // Requires microversion 2.64 or later. 
        
           Policy string `json:"policy,omitempty"` 
        
           // Rules specifies the set of rules. 
        
           // Requires microversion 2.64 or later. 
        
           Rules *Rules `json:"rules,omitempty"`

Long story short, the way you originally had the ListOpts is correct. Just add an in-line comment that it requires the client to be set to a specific microversion. If no tag options are specified, their default value is an empty string and it won't be sent in the request body.

Adding the fields to ListOpts is the only change needed to add the functionality you want, so the PR can be trimmed to just that.

I'm really sorry about the confusion and the extra work you put in because of it.

theopenlab-ci · 2019-10-24T19:17:07Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 22s
gophercloud-acceptance-test : FAILURE in 2h 30m 31s
gophercloud-acceptance-test-ironic : SUCCESS in 19m 06s

Server tags filtering support was added in Nova API since 2.26, so we can implement it here as well. This patch adds new filters `tags`, `tags-any`, `not-tags`, `not-tags-any` to server list options.

theopenlab-ci · 2019-10-25T17:58:09Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 10s
gophercloud-acceptance-test : FAILURE in 11m 42s
gophercloud-acceptance-test-ironic : SUCCESS in 27m 33s

jtopjian · 2019-10-25T18:00:49Z

recheck

theopenlab-ci · 2019-10-25T18:44:09Z

Build failed.

gophercloud-unittest : SUCCESS in 2m 54s
gophercloud-acceptance-test : FAILURE in 17m 20s
gophercloud-acceptance-test-ironic : SUCCESS in 41m 25s

jtopjian · 2019-10-25T18:48:48Z

The test failures are not related to this PR.

jtopjian

LGTM - thank you so much for your patience.

Fedosin · 2019-10-25T21:00:21Z

My pleasure :) thanks for your support!

Now to delete servers we first get a list of all available servers from Nova, and then we iterate through them to find those with required metadata. In the case of a large number of servers, this can take a very long time. Fortunately gophercloud introduced filtering by tags, so we can start using this feature to get only servers with the required tag. gophercloud/gophercloud#1759

Fedosin force-pushed the server_tags branch from bfb4ae1 to a791794 Compare October 23, 2019 17:27

ozerovandrei mentioned this pull request Oct 24, 2019

Compute V2: Tags API #1669

Closed

8 tasks

jtopjian requested changes Oct 24, 2019

View reviewed changes

Fedosin force-pushed the server_tags branch 2 times, most recently from 3cd51a5 to 385867a Compare October 24, 2019 15:46

Fedosin force-pushed the server_tags branch from 385867a to 5b6ec39 Compare October 24, 2019 15:58

Fedosin force-pushed the server_tags branch from 5b6ec39 to 6aeb3c1 Compare October 24, 2019 16:45

Implement server tags filtering support

775d353

Server tags filtering support was added in Nova API since 2.26, so we can implement it here as well. This patch adds new filters `tags`, `tags-any`, `not-tags`, `not-tags-any` to server list options.

Fedosin force-pushed the server_tags branch from 6aeb3c1 to 775d353 Compare October 25, 2019 17:29

jtopjian approved these changes Oct 25, 2019

View reviewed changes

jtopjian merged commit ac31aca into gophercloud:master Oct 25, 2019

Fedosin mentioned this pull request Jun 29, 2020

OpenStack: Deleting servers using tag-based filtering openshift/installer#3818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute V2: implement server tags filtering support #1759

Compute V2: implement server tags filtering support #1759

Fedosin commented Oct 23, 2019 •

edited

coveralls commented Oct 23, 2019 •

edited

theopenlab-ci bot commented Oct 23, 2019

theopenlab-ci bot commented Oct 23, 2019

jtopjian commented Oct 24, 2019

jtopjian left a comment

jtopjian Oct 24, 2019

jtopjian Oct 24, 2019

jtopjian Oct 24, 2019

Fedosin commented Oct 24, 2019

jtopjian commented Oct 24, 2019

theopenlab-ci bot commented Oct 24, 2019

theopenlab-ci bot commented Oct 25, 2019

jtopjian commented Oct 25, 2019

theopenlab-ci bot commented Oct 25, 2019

jtopjian commented Oct 25, 2019

jtopjian left a comment

Fedosin commented Oct 25, 2019

Compute V2: implement server tags filtering support #1759

Compute V2: implement server tags filtering support #1759

Conversation

Fedosin commented Oct 23, 2019 • edited

coveralls commented Oct 23, 2019 • edited

theopenlab-ci bot commented Oct 23, 2019

theopenlab-ci bot commented Oct 23, 2019

jtopjian commented Oct 24, 2019

jtopjian left a comment

Choose a reason for hiding this comment

jtopjian Oct 24, 2019

Choose a reason for hiding this comment

jtopjian Oct 24, 2019

Choose a reason for hiding this comment

jtopjian Oct 24, 2019

Choose a reason for hiding this comment

Fedosin commented Oct 24, 2019

jtopjian commented Oct 24, 2019

theopenlab-ci bot commented Oct 24, 2019

theopenlab-ci bot commented Oct 25, 2019

jtopjian commented Oct 25, 2019

theopenlab-ci bot commented Oct 25, 2019

jtopjian commented Oct 25, 2019

jtopjian left a comment

Choose a reason for hiding this comment

Fedosin commented Oct 25, 2019

Fedosin commented Oct 23, 2019 •

edited

coveralls commented Oct 23, 2019 •

edited