Skip to content
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

Fixes #12030 subscription data not getting updated after candlepin mo… #5536

Merged
merged 1 commit into from Nov 19, 2015

Conversation

Projects
None yet
6 participants
@johnpmitsch
Copy link
Member

commented Oct 15, 2015

…difies it

Left to do

  • refactor and update rabl templates to return content host subscriptions directly from cp
  • show guest/temporary subscriptions

This also fixes #12188 and #12029

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2015

In this PR, the quantity information for a content host or activation key subscription is recieved from candlepin and passed through the API to the UI. This is done because the subscription/pool in the AR database will not have information about the quantity attached to a particular content host or ack key.

This fixes some main issues with the recent subscription refactoring, but there is still work left to do, notably a refactoring of content hosts to show guest subscriptions.

What I am thinking will be best to solve the remaining issues and simplify things is create a new, duplicate pool in AR when one is attached to a content host or activation key. Then any information about the pool relating the the ack key or content host can be updated in the relation table or a new column in katello_pools, like quantity attached. This will also allow attaching multiple groups of the same subscription to a content host and have them be visible in different rows (satellite works this way now) and I believe will solve showing guest and temporary subscriptions.

Correct me if I am wrong, but I think we have a similar model for products?

This conversation can be taken elsewhere but I figured I would raise it here because this PR is a start to these changes.

@ehelms @jlsherrill @thomasmckay

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch Oct 19, 2015

@ehelms

View changes

app/controllers/katello/api/v2/subscriptions_controller.rb Outdated
@@ -232,48 +218,29 @@ def default_sort
%w(id desc)
end

def index_subscriptions

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

This is nitpicky, but is this importing or indexing? Indexing makes me think of our ES setup, import our newer way of handling backend data.

@ehelms

View changes

...pp/assets/javascripts/bastion_katello/subscriptions/subscriptionAttachAmountFilter.filter.js Outdated
return function (subscription) {
var amount = subscription.amount;
var amount;
var i;

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

You can do this in one line or one var statement on two lines.

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Actually, do these need to live outside the function using them in the return statement below?

@ehelms

View changes

app/controllers/katello/api/v2/subscriptions_controller.rb Outdated
params[:subscriptions].each do |sub_params|
subscription = Pool.find(sub_params[:id])
object.subscribe(subscription.cp_id, sub_params[:quantity])
subscription.import_data

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

If I read this correctly, now when we do an attach of a subscription to an object, we are doing an attach which calls out to Candlepin and then doing a data import from Candlepin to ensure out database is correct w.r.t. the Candlepin data?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

@ehelms yes basically.The import_data is mostly to update the quantities consumed.

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

Since we are adding complexity, with an additional call out to Candlepin, I am thinking this should be dynflowized to better deal with potential errors. @jlsherrill would you agree?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

@ehelms I think he suggested this originally, is there a good example I can follow?

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

You can always pick a resource that you know is dynflowed (e.g. product creation) and try and follow that. There are also a number of recordings that cover Dynflow and conversion: https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=UTF-8#q=dynflow&tbm=vid

I think those are two good places to start and then we can help fill in the gaps.

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

thanks for sharing those, I'll look into it and make the changes here.

@ehelms

View changes

app/models/katello/glue/candlepin/activation_key.rb Outdated
associations = Katello::PoolActivationKey.where(:activation_key_id => self.id)
associations.map { |assoc| assoc.destroy! if pools.map(&:id).exclude?(assoc.pool_id) }
pools.each do |pool|
Katello::PoolActivationKey.where(:pool_id => pool.id, :activation_key_id => self.id).first_or_create
pool_activation_key = Katello::PoolActivationKey.where(:pool_id => pool.id, :activation_key_id => self.id).first_or_create
quantity = key_pools.collect { |x| x["amount"] if x["id"] == pool.cp_id }.compact.first if key_pools.any?

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

There is a lot of calls and logic going into this statement -- could we break it down a bit to reduce the complexity when reading it?

@ehelms

View changes

app/models/katello/glue/candlepin/activation_key.rb Outdated
pool_activation_key = Katello::PoolActivationKey.where(:pool_id => pool.id, :activation_key_id => self.id).first_or_create
quantity = key_pools.collect { |x| x["amount"] if x["id"] == pool.cp_id }.compact.first if key_pools.any?
pool_activation_key.update_attributes(:quantity => quantity) if quantity
pool_activation_key.save

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Do we need to save this if the quantity isn't being updated?

@ehelms

View changes

app/models/katello/glue/candlepin/consumer.rb Outdated
@@ -88,6 +88,25 @@ def checkin(checkin_time)
raise e
end

def pool_quantities
quantity_table = {}

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Table is a confusing term to use here to me given it generally equates to a database table

@ehelms

View changes

app/models/katello/glue/candlepin/consumer.rb Outdated
pool_quantities = []
quantity_table.each do |key, value|
relation = {:pool_id => key.to_s.to_i, :quantity => value}
pool_quantities << relation

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

You could combine this statement and the above

@ehelms

View changes

app/models/katello/glue/candlepin/consumer.rb Outdated
quantity_table = {}
self.entitlements.each do |entitlement|
pool = self.get_pool(entitlement['pool']['id'])
pool_id = ::Katello::Pool.find_by_cp_id(pool["id"]).id.to_s.to_sym

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Is all of this casting from one thing to another required?

@ehelms

View changes

app/models/katello/glue/candlepin/pool.rb Outdated
@@ -27,8 +27,7 @@ def candlepin_data(cp_id)
Katello::Resources::Candlepin::Pool.find(cp_id)
end

def get_for_owner(organization)
Katello::Resources::Candlepin::Pool.get_for_owner(organization)
def get_for_owner(organization) Katello::Resources::Candlepin::Pool.get_for_owner(organization)

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Typo? This line on the same one as the def seems odd

id: subscription.host.uuid,
name: subscription.host.name
}
end

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Why do we need this information in the base rabl?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 27, 2015

Author Member

<div ng-if="subscription.virt_only === true && !subscription.host">
this uses it to show subscription type and guest host subscriptions in the index

@@ -13,3 +13,12 @@ attributes :sockets, :cores, :ram
attributes :instance_multiplier, :stacking_id, :multi_entitlement
attributes :type
attributes :name => :product_name
attributes :unmapped_guest
attributes :virt_only

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

Should we only provide these attributes if it is virtual?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

@ehelms I am not sure if that will work with the checks here, since virt_only is being checked for false? http://git.io/vW5mC

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

I think you mean the other way around? Since it is checking explicitly for true, then that piece of UI should be fine. However, the UI shouldn't inherently dictate the API.

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

@ehelms wrong line, my bad, I actually meant this one http://git.io/vW53Z Do you think we only return virt_only and unmapped_guest if virtual and update the UI there?

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

Way to turn my question on me :) I was more looking for your opinion -- my question being one of are these attributes that make sense in most use cases or a very select few of cases?

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

haha I wasn't sure if you had an opinion either way :) My opinion is to leave virt_only in because it is more explicit and can be used in logic easier when people use the api or in UI/hammer. Let me know if I am off base there.

This comment has been minimized.

Copy link
@ehelms
@jlsherrill

View changes

...pp/assets/javascripts/bastion_katello/subscriptions/subscriptionAttachAmountFilter.filter.js Outdated
if (attachedSubscriptions[i].pool_id === subscription.id) {
amount = attachedSubscriptions[i].quantity;
}
}

This comment has been minimized.

Copy link
@jlsherrill

jlsherrill Oct 20, 2015

Member

we typically prefer using underscore js's _.each function. Should be a number of examples in the code base

This comment has been minimized.

Copy link
@jlsherrill

jlsherrill Oct 20, 2015

Member

saves you from initializing 'i' as well :)

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 20, 2015

Member

or angular.forEach since its native to the angular library

This comment has been minimized.

Copy link
@jlsherrill

jlsherrill Oct 20, 2015

Member

err yes, or that :)

@ehelms

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

Is there an issue for the proposal in your comment? I not quite following what the issue is since you mentioned "remaining issues" but don't quantify them.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2015

@ehelms this is one http://projects.theforeman.org/issues/12031 and the other (which still needs an issue filed) is not showing multiple rows of the same subscription attached to the content host. So if you attach 4 of subscription foo and then 4 more of subscription foo (same pool as well) it should show in multiple rows, which is how candlepin returns them and satellite does this today.

@johnpmitsch johnpmitsch changed the title Fixes #12030 subscription data not getting updated after candlepin mo… [WIP] Fixes #12030 subscription data not getting updated after candlepin mo… Oct 23, 2015

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch Oct 23, 2015

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch 2 times, most recently Oct 28, 2015

@@ -13,3 +13,12 @@ attributes :sockets, :cores, :ram
attributes :instance_multiplier, :stacking_id, :multi_entitlement
attributes :type
attributes :name => :product_name
attributes :unmapped_guest

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

What does this attribute actually mean? I looked through the code, and it appears from Candlepin it comes in as unmapped_guests_only which has a different meaning than unmapped_guest to me.

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Oct 28, 2015

Author Member

I was just following the previous lazy accessor attribute here http://git.io/vW59b (from pre subscriptions scoped search) I can update the column to unmapped_guests_only and change where it is used. What about the API though?

This comment has been minimized.

Copy link
@ehelms

ehelms Oct 28, 2015

Member

Ahh right, sorry, missed that they were moving from show to base. We can leave them be since we've established this pattern of changing the attributes :)

@johnpmitsch johnpmitsch changed the title [WIP] Fixes #12030 subscription data not getting updated after candlepin mo… Fixes #12030 subscription data not getting updated after candlepin mo… Oct 28, 2015

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch 2 times, most recently Nov 10, 2015

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2015

@jlsherrill @cfouant updated with my proposed solution, I have changed https://github.com/Katello/katello/pull/5536/files#diff-d4ada19367f2e37f2adeafa9de778bccR530 to take an argument called include_temporary_guests which defaults to false. Its passed true for an argument when get_for_owner is called for ::Katello::Pool, which imports the temporary subscription when import_all is called. In the index of subscription controller, I changed it to only return when unmapped_guest column is false, which is now default in the migration (you'll have to reset or migrate) https://github.com/Katello/katello/pull/5536/files#diff-937e4184d63313d2975db7d87ebd1c90R37 This way a temporary subscription will not show up in an index call, but it is in the database when a content host is finding Pools by id.

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 11, 2015

@johnpmitsch - I'm still not seeing the subscription amounts updating upon registering a RHEL machine (either by activation key or with username/password and --auto-attach flag). Is the proposed solution in this yet, or are you just asking for feedback before implementing?

UPDATE: It does seem to update to the correct quantity as soon as subscriptions are manually attached on any content host in the org, leading me to believe there needs to be a subscription refresh as a final step on registration?

Also, the temporary subscriptions seem to be showing in the subscriptions table, which I think we should probably be hiding.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2015

@cfouant the proposed solution is in the code, I haven't tested it as thoroughly as I should but pushed it up for initial feedback in case I am on the wrong track and have to rewrite that part. I'll take a look tomorrow and get back to you on it, but if I remember correctly, the temporary subscription is a separate pool and was showing up in the list/remove part of the content host after auto-attach.
As far as subscriptions showing up in the subscriptions table (on an index call) did you rake katello:reset? The migrations have changed and that will affect what shows up on an index call.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2015

@cfouant I got a chance to look at this more,I am seeing the temporary subscription is not updating its quantity. There was '0 out of unlimited' on the temporary subscription and when I ran import_data on that subscription in the rails console it was updated to '1 out of unlimited' like here http://www.clipular.com/c/5697988432232448.png?k=FTT8Qz2b9rktG8neCysa5KukWYo, I'll fix and update. However, I am not seeing temporary subscriptions showing up on an index call (like in subscriptions main page), if you continue to see this after a reset, can you let me know if any of the Katello::Pool's have a :unmapped_guest => true value?

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

@johnpmitsch - I am going to check again with a reset database, but where I saw it was on the content host details Subscriptions tab, when you click 'add'. They have the temporary subscriptions listed in subscriptions to add to a content host. Possibly same on an activation key, but can't recall.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2015

@cfouant I thought temporary subscriptions should be shown to add to a content host? @jlsherrill @thomasmckay is this correct?

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

@jlsherrill @thomasmckay @johnpmitsch - Could be the case. I was under the impression that the temporary subscription is added automatically and behind the scenes, and that we wanted the functionality of an active subscription, but still hidden. Getting confused though.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2015

@cfouant If I understand @thomasmckay's explanation correctly, temporary subscriptions should be visible to a content host.

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

@thomasmckay - can you confirm? I would understand why we would want to see temporary subscriptions that exist on a content host, but would we want them to be available for adding manually to a content host, or via activation key?

@thomasmckay

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

@cfouant - Yes, they should be listed to manually attach to content host. Yes, the should be shown when attached to content host. No, they should not be listed on Subscriptions page. No, they should not be listed anywhere on Activation Keys.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2015

@cfouant what I am seeing locally -- temporary subscriptions are showing where they should, but the quantity on the temporary subscription is not updating after an auto-attach. Is this in line with what you are seeing?

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

@johnpmitsch - yes. Non temporary subscriptions quantities are also not updating on rhel machines.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2015

@cfouant @jlsherrill @thomasmckay Fixing updating temporary subscription quantities is currently being blocked by http://projects.theforeman.org/issues/12475 as the auto-attach happens in candlepin, and to update the subscription quantities for temporary subscriptions would require listening for a message from candlepin via qpid, which is not working correctly in a dev environment.

If my understanding of all this is correct, then it would appear we have two options:

  1. Fix any non-qpid-related issues in this PR and then merge after review, then open up issue for temporary subscriptions not being updated and assign to me.
  2. Wait for 12475 to be resolved before updating, reviewing, and merging this PR.

I am in favor of option 1, but want to hear your thoughts/opinions

@thomasmckay

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

I am in favor of merging this ASAP and dealing with any further issues. As it stands right now, subscriptions are all but unusable in master.

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

@johnpmitsch I'm with @thomasmckay on this one. I would link the new issue you open in this PR as well.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2015

@cfouant @thomasmckay sounds good, someone go ahead and ACK and I will merge

@cfouant

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

ACK from me, but please hold on merge until getting ACKs from the other commenters in this thread @jlsherrill @thomasmckay

@thomasmckay

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@johnpmitsch Mind rebasing this against master? I will test again today with the puppet-katello PRs that fix the qpid events from candlepin.

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch Nov 18, 2015

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2015

@thomasmckay rebased

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:subscription-fixes branch to 64ffadb Nov 18, 2015

@thomasmckay

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

systems_controller.rb:

  • GET /subscriptions - missing pagination apipie params
  • DELETE /subscriptions - no such method
@thomasmckay

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

ACK - There are remaining issues in the area of subscriptions but they can be addressed individually outside of this PR. I will defer to @jlsherrill to give final ACK to merge.

@jlsherrill

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

ACK

johnpmitsch added a commit that referenced this pull request Nov 19, 2015

Merge pull request #5536 from johnpmitsch/subscription-fixes
Fixes #12030 subscription data not getting updated after candlepin mo…

@johnpmitsch johnpmitsch merged commit 2ae50fe into Katello:master Nov 19, 2015

1 check passed

default Job result: SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.