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

Add the date of when inventory was collected #340

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 25, 2019

We have a column tracking when the EMS was last successfully saved
(#last_refresh_date). This tells us when save_inventory last finished,
but a lot of code wants to know when the inventory which was saved was
collected.

An example of this is "I just added a disk to a VM at time T1, I want to
know when this change is represented in VMDB"

You can wait until last_refresh_date >= T1, but it is possible that
save_inventory took a long time and the inventory was actually
collected at T0 and just saved at T2.

If we had a column that stored when the inventory was collected then no
matter how long the refresh sat in the queue we would know for a fact
that the EMS includes our changes.

Currently this is done by getting a task_id back with the refresh queue
item, but e.g. VMware streaming refresh doesn't use the normal queue for
refreshes so this method won't work.

@agrare agrare requested a review from Fryguy February 25, 2019 18:40
@agrare
Copy link
Member Author

agrare commented Feb 25, 2019

This fixes the ReconfigVM_Task_Complete event handler which uses the Refresh state machine to wait for the refresh task to complete when using vmware streaming refresh.

@@ -0,0 +1,5 @@
class AddLastCollectedDateToEms < ActiveRecord::Migration[5.0]
def change
add_column :ext_management_systems, :last_collected_date, :datetime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion note, _date was used here for consistency with other date columns...would have like this to be called _at otherwise.

@Fryguy
Copy link
Member

Fryguy commented Feb 25, 2019

Naming wise last_collected_date still feels like "after" the fact. Perhaps last_refresh_start_date? Then at least that pair are tied together as bookends? @jrafanie Thoughts?

@agrare
Copy link
Member Author

agrare commented Feb 25, 2019

Yeah I like tying this to the last_refresh_date attribute, only issue I have with last_refresh_start_date is that looking at that I would assume that is when the refresh was started, not when the collect_inventory_for_targets stage was completed.

High level I'd like this to represent the timestamp for how old the inventory data is that we saved rather than when the save finished.

@jrafanie
Copy link
Member

Isn't naming fun?

I think the process of collecting or refreshing the inventory "clouds" the meaning of the current suggestions... what a user really wants to know is what inventory we have:

inventory_captured_at
inventory_captured_date
date_of_inventory
time_of_inventory

Throw in _last if you want, such as:
date_of_last_inventory

@agrare
Copy link
Member Author

agrare commented Feb 26, 2019

I do like inventory_captured_at/date, maybe last_refresh_inventory_date to keep it closely related to last_refresh_date ?

@jrafanie
Copy link
Member

do we care about the word capture or refresh?

last_inventory_date tells me the inventory is from that date. I don't care about the mechanics of capture/refresh, only that the inventory is relevant to that specific date, right?

@jrafanie
Copy link
Member

naming is hard

@agrare agrare force-pushed the add_last_collected_on_to_ems branch from 5302668 to e9746f0 Compare February 26, 2019 19:18
@agrare
Copy link
Member Author

agrare commented Feb 26, 2019

Okay last_inventory_date sounds pretty good, updated.

@jrafanie
Copy link
Member

note, the old filename and class reference the original idea. I'm ok with it... but in case it bothered you...

class AddLastCollectedDateToEms

We have a column tracking when the EMS was last successfully saved
(#last_refresh_date).  This tells us when save_inventory last finished,
but a lot of code wants to know when the inventory which was saved was
collected.

An example of this is "I just added a disk to a VM at time T0, I want to
know when this change is represented in VMDB"

You can wait until last_refresh_date >= T0, but it is possible that this
was in the queue for a long time and the inventory was actually
collected at T-1 and just saved at T1.

If we had a column that stored when the inventory was collected then no
matter how long the refresh sat in the queue we would know for a fact
that the EMS includes our changes.

Currently this is done by getting a task_id back with the refresh queue
item, but e.g. VMware streaming refresh doesn't use the normal queue for
refreshes so this method won't work.
@agrare agrare force-pushed the add_last_collected_on_to_ems branch from e9746f0 to 5dda780 Compare February 26, 2019 19:29
@agrare
Copy link
Member Author

agrare commented Feb 26, 2019

Good catch updated

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, what say you @Fryguy... we did a thing with naming

@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2019

Checked commit agrare@5dda780 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@agrare
Copy link
Member Author

agrare commented Mar 1, 2019

@Fryguy what do you think of the name?

@jrafanie
Copy link
Member

jrafanie commented Mar 1, 2019

He'll have an opinion after I merge it 😉 😉 😉 ... will give a few more hours and then pull the trigger

@jrafanie jrafanie merged commit e609d70 into ManageIQ:master Mar 1, 2019
@jrafanie jrafanie assigned jrafanie and unassigned Fryguy Mar 1, 2019
@jrafanie jrafanie added this to the Sprint 106 Ending Mar 4, 2019 milestone Mar 1, 2019
@agrare agrare deleted the add_last_collected_on_to_ems branch July 29, 2019 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants