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

Schedule a daily count of managed VMs and print result to audit log. #19830

Merged
merged 4 commits into from
Feb 14, 2020

Conversation

jaredm-ibm
Copy link
Contributor

@jaredm-ibm jaredm-ibm commented Feb 12, 2020

To implement license management, we need to get a daily count of managed VMs. This work adds a scheduled task controlled. The frequency is controlled by :audit_vm_total key. A new method was added to miq_server which performs the count and logging.

The more specific license management is not part of this PR. We decided to add this feature here to avoid requiring credentials in the client script.

https://bugzilla.redhat.com/show_bug.cgi?id=1805252

Schedule the collection of VMs managed and print to the audit log.
app/models/miq_server.rb Outdated Show resolved Hide resolved
total_vms += vms
total_hosts += hosts
end
$audit_log.info("Under Management: VMs: [#{total_vms}], Hosts: [#{total_hosts}]")
Copy link
Member

Choose a reason for hiding this comment

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

For parsing purposes, let's change the log message to be a JSON output. Something like

totals = {"vms" => total_vms, "hosts" => total_hosts}
$audit_log.info("Under Management: #{totals.to_json}")

This way, your parser can just parse simple JSON, and if we ever need to add stuff in the future we can and it won't break your parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've changed it. Output is:
[----] I, [2020-02-14T11:06:26.844294 #84536:3fde43432e3c] INFO -- : Q-task_id([audit_managed_resources]) Under Management: {"vms":0,"hosts":0}

@Fryguy
Copy link
Member

Fryguy commented Feb 14, 2020

Overall LGTM... Just a few comments above.

@@ -16,6 +16,10 @@ def miq_server_worker_log_status
queue_work(:class_name => "MiqWorker", :method_name => "log_status_all", :task_id => "log_status", :server_guid => MiqServer.my_guid, :priority => MiqQueue::HIGH_PRIORITY)
end

def miq_audit_vm_total
Copy link
Member

Choose a reason for hiding this comment

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

why the miq prefix on this method name? Is that a standard in this file? /cc @Fryguy

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest a simpler name, such as audit_managed_resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using the same convention (although missed the _server) as the rest of the file "miq_server_worker_log_status" and "miq_server_status_update" which are the two methods above. My guess is that it is a way to discern where the job is coming from. So the method should be "miq_server_audit_managed_resources"

Copy link
Member

Choose a reason for hiding this comment

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

If you look through the file, you will see many methods without miq_server. miq_server refers to the MiqServer object (which is associated with the appliance). I advise we use the name audit_managed_resources here.

miq_server.rb - Changed method name and log entry is JSON encoded.
jobs.rb - Method name is now miq_server_audit_managed_resources.
settings.yaml - Setting name is now :audit_managed_resources
runner.rb - Update for method name changes
@@ -1211,6 +1211,7 @@
:vim_performance_states_purge_interval: 1.day
:vm_retired_interval: 10.minutes
:yum_update_check: 12.hours
:audit_managed_resources: 1.days
Copy link
Member

Choose a reason for hiding this comment

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

this list is intended to be alphabetized, so if you can move this line into the proper place - that would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved my setting to the top of the list. Although the list isn't sorted properly. Should I move the other settings?

Copy link
Member

Choose a reason for hiding this comment

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

let's change the other out of order ones in a separate PR

Copy link

Choose a reason for hiding this comment

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

I didn't find a way other then restarting evmserverd for this value (when changed) to pick up. Is this expected?

@miq-bot
Copy link
Member

miq-bot commented Feb 14, 2020

Checked commits jaredm-ibm/manageiq@589add9~...5f6ea19 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@chessbyte chessbyte merged commit c223c1c into ManageIQ:master Feb 14, 2020
@chessbyte chessbyte added this to the Sprint 130 Ending Feb 17, 2020 milestone Feb 14, 2020
@chessbyte
Copy link
Member

Thank you @jaredm-ibm for your first PR to ManageIQ!

simaishi pushed a commit that referenced this pull request Feb 21, 2020
Schedule a daily count of managed VMs and print result to audit log.

(cherry picked from commit c223c1c)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805915
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 6fc121e6dc5306046814ba216c879a833c39f1e3
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Fri Feb 14 11:51:24 2020 -0500

    Merge pull request #19830 from jaredm-ibm/master

    Schedule a daily count of managed VMs and print result to audit log.

    (cherry picked from commit c223c1c6aa0183a0df6211e46bb900de11d59904)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805915

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.

6 participants