Fixes #22909 - Remove subscriptions from upstream allocation #7241

jturel · 2018-03-15T14:48:41Z

API may be invoked as follows:

DELETE /katello/api/v2/organizations/:id/upstream_subscriptions

With a body:

{
  "pool_ids: ["a", "b", "c"]
}

The pool ids given must have an upstream_entitlement_id

theforeman-bot · 2018-03-15T14:48:48Z

Issues: #22909

jturel · 2018-03-15T14:49:17Z

@akofink this is how things are shaping up from my side, FYI

akofink

Looking good!

akofink · 2018-03-15T17:07:08Z

app/lib/actions/katello/upstream_subscriptions/delete.rb

+        def plan(organization, entitlement_ids)
+          sequence do
+            entitlement_ids.each do |id|
+              plan_self(organization_id: organization.id, entitlement_id: id)


I think this line could be concurrent, but I don't think we're going to have a bunch of tasks here anyway.

does this actually work? Can you plan_self multiple times? I've not tried it. Normally we would plan some other action like this, rather than plan_self in a loop.

akofink · 2018-03-15T17:10:21Z

app/lib/actions/katello/upstream_subscriptions/delete.rb

@@ -0,0 +1,30 @@
+module Actions
+  module Katello
+    module UpstreamSubscriptions


I placed mine in Actions::Candlepin::Consumer, since that's the relevant API endpoint in the Candlepin resource.

akofink · 2018-03-15T17:45:31Z

app/lib/katello/resources/candlepin/consumer.rb

-          def path(id = upstream_consumer_id)
-            super(id)
+          def path
+            join_path(prefix, 'consumers', upstream_consumer_id)


I wouldn't override this. You can use the following to achieve the same result:

self["entitlements/#{entitlement_id}"]

akofink · 2018-03-15T17:54:33Z

app/models/katello/upstream_consumer.rb

+module Katello
+  class UpstreamConsumer < OpenStruct
+    def self.remove_entitlement(organization, entitlement_id)
+      resource_class.organization = organization


I moved this org setting stuff to the Resource (for now).

jlsherrill · 2018-03-16T03:15:57Z

app/controllers/katello/api/v2/upstream_subscriptions_controller.rb

@@ -27,6 +25,15 @@ def index
      respond(collection: collection)
    end

+    api :PUT, "/organizations/:organization_id/upstream_subscriptions",


hrmmm, using PUT here seems weird, what is the update quantity going to look like? We really should have planned out all these apis from the start :)

Yeah, should be a DELETE, huh :)

waldenraines · 2018-03-16T11:57:16Z

@jturel can you please provide examples of the requests including the payload? Want to see how it's supposed to be structured so we can go ahead and start on our end.

jturel · 2018-03-16T15:38:56Z

@waldenraines added example w/ payload

jturel · 2018-03-16T15:39:47Z

app/lib/katello/resources/candlepin.rb

@@ -130,6 +130,13 @@ def resource(url = self.site + self.path, client_cert = self.client_cert, client
                                    )
          end

+          def json_resource(url = self.site + self.path, client_cert = self.client_cert, client_key = self.client_key, ca_file = nil, options = {})


Nabbed this from PR7235 - will be rebased out of here once it's merged

waldenraines

@waldenraines added example w/ payload

Thanks. I approve this from a consumer's perspective.

jturel · 2018-03-16T17:16:15Z

app/lib/actions/katello/upstream_subscriptions/remove_entitlement.rb

+  module Katello
+    module UpstreamSubscriptions
+      class RemoveEntitlement < Actions::Base
+        middleware.use ::Actions::Middleware::PropagateCandlepinErrors


This action locks the org if we raise an error when, for example, the entitlement was already removed from the allocation

The user doesn't really have a way to work around that, so the locking doesn't make sense. Should the action be cancellable or something similar?

I think this is taken care of with the rescue_strategy: https://github.com/Katello/katello/pull/7235/files#diff-562f4c0f3f476d7dd66af859c597ce58R24

That should keep the task from locking on failure.

cool, I think I'll use that

@jturel Still need to follow up here.

I added it in the 'parent' action ::RemoveEntitlements

Should this middleware.use be in the parent action, or this one, and why?

This lower-level action is the one that's hitting candlepin, so I want it to propagate the error - and it appears that it bubbles up through the parent action too.

jturel · 2018-03-19T15:03:15Z

@jlsherrill @akofink I think this is complete and is not dependent on the "add subs" PR. please take a look & let me know if you'd like any more changes

akofink · 2018-03-19T15:24:06Z

app/lib/katello/resources/candlepin/consumer.rb

@@ -233,6 +240,9 @@ def update(url, client_cert, client_key, ca_file, attributes)
          ensure
            RestClient.proxy = ""
          end
+
+          delegate :[], to: :json_resource
+          delegate :put, to: :json_resource


Unnecessary line.

akofink · 2018-03-19T15:24:38Z

config/routes/api/v2.rb

-          api_resources :upstream_subscriptions, only: :index
+          api_resources :upstream_subscriptions, only: [:index] do
+            collection do
+              delete :destroy


why not use the shorthand? DELETE is the default method for a destroy action.

I'm not destroying a single resource - but a collection of them (potentially) - so there won't be an "upstream subscription ID" in the URL. I think this is the correct way...

Ah yeah, right. :)

akofink · 2018-03-19T15:26:39Z

It looks good. There's still some vestigial pieces of old implementation hanging around that I commented on.

jlsherrill · 2018-03-20T13:01:18Z

[test katello]

houndci-bot · 2018-03-26T14:57:23Z

app/lib/katello/resources/candlepin/consumer.rb

@@ -214,6 +214,12 @@ def path(id = upstream_consumer_id)
            super(id)
          end

+          def remove_entitlement(entitlement_id)
+            raise ArgumentError.new("No entitlement ID given to remove.") if entitlement_id.blank?


Provide an exception class and message as arguments to raise.
Use fail instead of raise to signal exceptions.

jturel · 2018-03-26T15:01:47Z

@akofink @jlsherrill updated to use the new KeepCurrentTaxonomies action middleware

jlsherrill · 2018-03-26T16:59:50Z

app/lib/actions/katello/upstream_subscriptions/remove_entitlement.rb

+        middleware.use Actions::Middleware::PropagateCandlepinErrors
+
+        input_format do
+          param :org_id


org_id is no longer needed

jturel · 2018-03-26T18:13:40Z

@jlsherrill removed the org_id input param

akofink · 2018-03-26T18:38:27Z

Unrelated bug when refreshing manifest, which is caused by the audit work:

Validation failed: Locations expecting locations used by hosts or inherited (check mismatches report).

Same thing happens for the add API, of course. :)

akofink

Code looks good, and my entitlements are successfully removed from my allocation (or whatever ;). One thing that's a bit weird: if you remove a bunch (~20) entitlements, the progress bar quickly hits ~95% and then sits there forever while the manifest refreshes. I wonder if there's a way to weight the progress, since we know the manifest refresh is going to take way longer than the actual remove. I think @johnpmitsch did something like this a long time ago...

akofink · 2018-03-26T18:53:56Z

test/katello_test_helper.rb

@@ -191,6 +191,10 @@ def set_user(user = nil)
    User.current = user
  end

+  def set_organization(org)
+    Organization.current = org


Hmm... I guess this is fine. I just stubbed it because I was scared that the current org would be kept for other tests, but it's clearly copying the set_user pattern above, so I guess it's fine? :)

Cool, I think it's good to abstract the impl details from the testing as much as we can!

That should depend on what kind of test you're writing. :)

johnpmitsch · 2018-03-26T18:59:09Z

Here is the PR that I used to weight the capsule sync progress bar

jturel · 2018-03-26T21:14:27Z

Thanks @johnpmitsch

@akofink - added some weighting, give it a try - it does seem better with it.

johnpmitsch · 2018-03-27T13:18:01Z

app/lib/actions/katello/upstream_subscriptions/remove_entitlement.rb

+        end
+
+        def run_progress
+          done? ? 1 : 0.1


I should have clarified, the example I showed you isn't the typical way to weight a task. For that task, it would show as half done very quickly, as the first "half" of the task took a short amount of time, but the second "half" could take hours. This meant the progress bar would just jump to 50% and stay there a long time, which made me use this override.

I'm not sure if that fix is what you need (it very well could be), but there is also the run_progress_weight method, which we us other places which gives a task weight among all the other tasks.

Thanks for clarifying @johnpmitsch - I think the way I've done it works fine (I did try run_progress too but it actually didn't seem to have an affect).

must be a similar situation then! As a long term goal we should look at adding this functionality to dynflow since it seems to be common and we could possibly use in other places

jturel · 2018-03-27T15:00:28Z

[test]

jturel · 2018-03-27T15:01:07Z

[test katello]

jturel · 2018-03-27T15:07:31Z

@akofink @jlsherrill anything missing which prevents an ACK (aside from the tests still running!)?

akofink · 2018-03-27T18:23:34Z

I still need to retest. The code looks good, though!

jturel · 2018-03-27T18:26:09Z

@akofink don't test just yet, this API is going to be changing per recent IRC discussion to accept local pool id rather than upstream entitlement ID so that we can do the lookup on the server side

jturel · 2018-03-28T13:40:38Z

@akofink @jlsherrill API changed to accept downstream pool ids where we will now lookup the upstream entitlement ID on the backend. Let me know what you think.

Reminder that the purpose of the change is from the UI/API perspective where the upstream entitlement id is not readily available.

akofink

Do you think we should still also accept entitlement_ids? This supports a future refresh_manifest param so users could add/remove subs without refreshing their manifest until the end of their actions. We've spoken a bit about this potential outcome for CLI users.

akofink · 2018-03-28T13:44:22Z

app/controllers/katello/api/v2/upstream_subscriptions_controller.rb

@@ -26,6 +24,14 @@ def index
      respond(collection: collection)
    end

+    api :DELETE, "/upstream_subscriptions",
+      N_("Remove one or more subscriptions from an upstream subscription allocation")
+    param :pool_ids, Array, desc: N_("Array of pool IDs. This will only work with upstream (non-custom) pools"), required: true


I think we should make it clear that this ID does not come from upstream. I'm not really sure how, maybe:

"Array of local pool IDs. ...", or "Katello pool IDs"?

jturel · 2018-03-28T15:04:15Z

I think I'd rather support entitlement_ids when we have the need. Updating the API language..

akofink · 2018-03-29T15:45:57Z

app/lib/katello/resources/candlepin/consumer.rb

+          def remove_entitlement(entitlement_id)
+            fail ArgumentError, "No entitlement ID given to remove." if entitlement_id.blank?
+
+            self["entitlements/#{entitlement_id}"].delete


Working well, other than the individual RemoveEntitlement output in Dynflow. You need to return a string to the action. Wrapping this in a JSON.parse would do the trick.

Hmm... nevermind. Candlepin doesn't return anything here on success! Bummer!

akofink

Works as expected. Could we have a remove_entitlement_test.rb please?

jturel · 2018-03-29T16:09:44Z

test/actions/katello/upstream_subscriptions/remove_entitlements_test.rb

@@ -0,0 +1,63 @@
+require 'katello_test_helper'
+
+describe ::Actions::Katello::UpstreamSubscriptions::RemoveEntitlement do


@akofink I'm testing that action here - do you want me to put this in a separate file?

Oh! lol Yeah, that'd be good. :) Just for the reason that people looking for these tests will look there (including myself just now).

akofink

Very nice @jturel! Works as expected. 🥇

jturel · 2018-03-29T16:17:22Z

@akofink done!

akofink · 2018-03-29T19:49:31Z

@jturel One more thing I just realized: perhaps we should handle the case of nil Organization.current. Right now, the manifest refresh is called with nil.

jturel · 2018-04-02T20:35:35Z

@akofink this is updated with the latest stuff and I added the nil check for current org. can I get (hopefully!) one last run-through?

akofink

Still works great. ACK from me! Thanks @jturel!

jturel · 2018-04-03T13:49:34Z

[test katello]

theforeman-bot added Needs testing Not yet reviewed labels Mar 15, 2018

akofink reviewed Mar 15, 2018

View reviewed changes

jlsherrill reviewed Mar 16, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from e6be9a9 to 2d494d5 Compare March 16, 2018 15:35

jturel changed the title ~~[WIP] Fixes #22909 - Remove subscriptions from upstream allocation~~ Fixes #22909 - Remove subscriptions from upstream allocation Mar 16, 2018

jturel commented Mar 16, 2018

View reviewed changes

waldenraines reviewed Mar 16, 2018

View reviewed changes

jturel commented Mar 16, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from 2d494d5 to bf04aa5 Compare March 16, 2018 17:44

akofink reviewed Mar 19, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from bf04aa5 to 5968916 Compare March 19, 2018 15:32

jturel force-pushed the remove_upstream_subs branch from 5968916 to a010df5 Compare March 26, 2018 14:56

houndci-bot reviewed Mar 26, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from a010df5 to e4b9c6f Compare March 26, 2018 15:01

jlsherrill reviewed Mar 26, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from e4b9c6f to 0244713 Compare March 26, 2018 18:12

akofink reviewed Mar 26, 2018

View reviewed changes

johnpmitsch reviewed Mar 27, 2018

View reviewed changes

jturel changed the title ~~Fixes #22909 - Remove subscriptions from upstream allocation~~ [WIP] Fixes #22909 - Remove subscriptions from upstream allocation Mar 27, 2018

jturel force-pushed the remove_upstream_subs branch from 1ae0aae to 17e2920 Compare March 28, 2018 13:21

jturel changed the title ~~[WIP] Fixes #22909 - Remove subscriptions from upstream allocation~~ Fixes #22909 - Remove subscriptions from upstream allocation Mar 28, 2018

akofink reviewed Mar 28, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from 17e2920 to 0e92de8 Compare March 28, 2018 15:07

akofink reviewed Mar 29, 2018

View reviewed changes

jturel commented Mar 29, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch from 0e92de8 to 6ebefb0 Compare March 29, 2018 16:16

akofink approved these changes Mar 29, 2018

View reviewed changes

jturel force-pushed the remove_upstream_subs branch 2 times, most recently from bfdc666 to 9a12f49 Compare April 2, 2018 20:26

Fixes #22909 - Remove subscriptions from upstream allocation

05ac7ff

jturel force-pushed the remove_upstream_subs branch from 9a12f49 to 05ac7ff Compare April 2, 2018 20:34

akofink approved these changes Apr 3, 2018

View reviewed changes

jturel merged commit 59e3a1a into Katello:master Apr 3, 2018

jturel deleted the remove_upstream_subs branch April 3, 2018 15:41

		@@ -0,0 +1,63 @@
		require 'katello_test_helper'

		describe ::Actions::Katello::UpstreamSubscriptions::RemoveEntitlement do

Fixes #22909 - Remove subscriptions from upstream allocation #7241

Fixes #22909 - Remove subscriptions from upstream allocation #7241

Conversation

jturel commented Mar 15, 2018 • edited

theforeman-bot commented Mar 15, 2018

jturel commented Mar 15, 2018

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines commented Mar 16, 2018 • edited

jturel commented Mar 16, 2018

Choose a reason for hiding this comment

waldenraines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink commented Mar 19, 2018

jlsherrill commented Mar 20, 2018

Choose a reason for hiding this comment

jturel commented Mar 26, 2018

Choose a reason for hiding this comment

jturel commented Mar 26, 2018

akofink commented Mar 26, 2018

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Mar 26, 2018

jturel commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel commented Mar 27, 2018

jturel commented Mar 27, 2018

jturel commented Mar 27, 2018

akofink commented Mar 27, 2018

jturel commented Mar 27, 2018

jturel commented Mar 28, 2018 • edited

akofink left a comment

Choose a reason for hiding this comment

akofink Mar 28, 2018 • edited

Choose a reason for hiding this comment

jturel commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink left a comment

Choose a reason for hiding this comment

jturel commented Mar 29, 2018

akofink commented Mar 29, 2018

jturel commented Apr 2, 2018

akofink left a comment

Choose a reason for hiding this comment

jturel commented Apr 3, 2018

jturel commented Mar 15, 2018 •

edited

waldenraines commented Mar 16, 2018 •

edited

jturel Mar 19, 2018 •

edited

jturel Mar 19, 2018 •

edited

jturel commented Mar 28, 2018 •

edited

akofink Mar 28, 2018 •

edited