Fixes #13013 - Adds API to display pulp dir status #3

shlomizadok · 2016-01-06T09:43:27Z

No description provided.

ohadlevy · 2016-01-06T11:23:10Z

Gemfile

@@ -2,5 +2,5 @@ source 'https://rubygems.org'
 gemspec

 group :development do
-  gem 'smart_proxy', :git => "https://github.com/theforeman/smart-proxy"
+  gem 'smart_proxy', :git => 'https://github.com/theforeman/smart-proxy', :branch => 'develop'


Actually, not. Since smart-proxy now have only develop branch (master was deleted), bundle will fail when trying to run. So need to point it to develop branch (which also makes sense in development).

@witlessbird your thoughts?

This seems to be the standard for group development.
see:
https://github.com/theforeman/smart_proxy_remote_execution_ssh/blob/master/Gemfile#L12
https://github.com/theforeman/smart_proxy_discovery/blob/develop/Gemfile#L8
https://github.com/theforeman/smart_proxy_openscap/blob/master/Gemfile#L7
https://github.com/theforeman/smart_proxy_dns_plugin_template/blob/master/Gemfile#L5
https://github.com/theforeman/smart_proxy_chef/blob/master/Gemfile#L7
to name a few

ohadlevy · 2016-01-06T12:51:55Z

lib/smart_proxy_pulp_plugin/pulp_disk.rb

+module PulpProxy
+  class PulpDisk
+    def initialize(path = PulpProxy::Plugin.settings.pulp_dir)
+      @path = path


can you add the properties to the class? e.g.

attr_accessor :free, :total, :block_device, :path

ohadlevy · 2016-01-06T14:31:32Z

lib/smart_proxy_pulp_plugin/disk_usage.rb

@@ -0,0 +1,25 @@
+module PulpProxy
+  class DiskUsage
+    attr_accessor :free, :total, :block_device, :path


maybe consider accepting a unit argument, so you can decide if its bytes, kilo, mb, gb or human

shlomizadok · 2016-01-06T15:39:27Z

Changed a little bit. let me know what u think

ohadlevy · 2016-01-06T16:03:38Z

lib/smart_proxy_pulp_plugin/disk_usage.rb

+    attr_reader :path, :stat, :size
+
+    def initialize(path = PulpProxy::Plugin.settings.pulp_dir, size = SIZE[:megabyte])
+      @path = path


I would pass opts = {} instead of hard coding the size as the second argument, e.g.

DiskUsage.new(:path => '..', :size_format ..)

lzap · 2016-01-06T17:31:34Z

smart_proxy_pulp.gemspec

@@ -25,6 +25,7 @@ Gem::Specification.new do |gem|
  gem.add_development_dependency('webmock', '~> 1')
  gem.add_development_dependency('rack-test', '~> 0')
  gem.add_development_dependency('rake', '~> 10')
+  gem.add_dependency('sys-filesystem', '~> 1.1')


Uh are you sure? :-) You need to create packages for Red Hat and Debian... I was not insisting on this at all.

👍 df is simple enough instead of calling out to an external ffi gem.
granted it will not work on windows, but since the scope of pulp is only redhat family I think I'm fine with it.

lzap · 2016-01-12T08:41:06Z

settings.d/pulpnode.yml.example

+#:pulp_dir: /var/lib/pulp
+#:pulp_content: /var/lib/pulp/content
+#:mongodb: /var/lib/mongodb


The values are incorrect, you have renamed them.

lzap · 2016-01-12T08:52:18Z

Ok it works but convert to numbers what should be a number...

[lzap@lzapx smart-proxy-pulp]$ curl -sk http://localhost:8448/pulp/status/disk_usage?size=megabyte | json_reformat 
{
    "pulp_dir": {
        "filesystem": "tmpfs",
        "1m-blocks": "7875",
        "used": "1",
        "available": "7875",
        "percent": "1%",
        "mounted": "/tmp",
        "path": "/tmp",
        "size": "megabyte"
    }
}

shlomizadok · 2016-01-12T09:08:19Z

Converted numbers to what should be a number.

lzap · 2016-01-12T09:14:18Z

There are test failures.

lzap · 2016-01-12T09:16:50Z

I have no push rights, @witlessbird RTM:

13 tests, 19 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

ohadlevy · 2016-01-12T10:05:37Z

lib/smart_proxy_pulp_plugin/disk_usage.rb

+        stdout.read.split("\n")
+      end
+      raw.each do |line|
+        logger.debug "[#{command_path}] #{line}"


i think you can just dump raw instead of loop over it

It looks better on a loop, but I can switch to a raw dump

dumping raw

dmitri-d · 2016-01-12T12:01:08Z

lib/smart_proxy_pulp_plugin/pulp_api.rb

+
+    get '/status/disk_usage' do
+      size = (params[:size] && DiskUsage::SIZE.keys.include?(params[:size].to_sym)) ? params[:size].to_sym : :kilobyte
+      monitor_dirs = ::PulpProxy::Plugin.settings.to_h.delete_if { |key, _| key == :pulp_url || key == :enabled }


Not great, as this will have to change the moment an additional setting is added; Why not explicitly list settings you are interested in?

Changed to select, with the keys I want

dmitri-d · 2016-01-12T12:10:08Z

[test]

shlomizadok · 2016-01-12T12:49:02Z

Changed according to your comments.
Don't think [test] plays here

dmitri-d · 2016-01-12T14:47:12Z

test/pulp_api_test.rb

+
+  def test_returns_pulp_disk_on_200
+    PulpProxy::Plugin.load_test_settings(:pulp_dir => ::Sinatra::Application.settings.root)


You'll need to define 'pulp_content_dir' and 'mongodb_dir' too, as otherwise default values are going to be used (and test will fail on my machine, as I don't have those dirs).

dmitri-d · 2016-01-12T14:58:22Z

lib/smart_proxy_pulp_plugin/disk_usage.rb

+    def get_stat
+      raw = Open3::popen3({"LC_ALL" => "C"}, *command) do |stdin, stdout, stderr, thread|
+        unless stderr.read.empty?
+          error_line = stderr.read.join(" ")


There is an issue here: I'm getting "undefined method `join' for "":String": #read returns a single string, readlines returns an array.

dmitri-d · 2016-01-12T15:08:01Z

merged at 7b4d074. Thanks, @shlomizadok!

Add spool dir instructions for git users

ohadlevy reviewed Jan 6, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from a54d5f2 to 397a691 Compare January 6, 2016 12:45

ohadlevy reviewed Jan 6, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from 397a691 to 9c62328 Compare January 6, 2016 14:06

ohadlevy reviewed Jan 6, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from 9c62328 to 023b99c Compare January 6, 2016 15:38

ohadlevy reviewed Jan 6, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from 023b99c to 23da45d Compare January 6, 2016 17:06

lzap reviewed Jan 6, 2016
View reviewed changes

lzap reviewed Jan 12, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch 2 times, most recently from b46deb2 to 1a519eb Compare January 12, 2016 08:50

shlomizadok force-pushed the fix_13013 branch from 1a519eb to 83716f8 Compare January 12, 2016 09:07

shlomizadok force-pushed the fix_13013 branch from 83716f8 to 6fa37ef Compare January 12, 2016 09:16

ohadlevy reviewed Jan 12, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from 6fa37ef to 0204f1c Compare January 12, 2016 10:18

dmitri-d reviewed Jan 12, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from 0204f1c to c29abe3 Compare January 12, 2016 12:47

dmitri-d reviewed Jan 12, 2016
View reviewed changes

shlomizadok force-pushed the fix_13013 branch from c29abe3 to fc1fe6e Compare January 12, 2016 14:54

dmitri-d reviewed Jan 12, 2016
View reviewed changes

Fixes #13013 - Adds API to display pulp dir status

69eab7b

shlomizadok force-pushed the fix_13013 branch from fc1fe6e to 69eab7b Compare January 12, 2016 15:01

dmitri-d closed this Jan 12, 2016

shlomizadok referenced this pull request in shlomizadok/smart-proxy-pulp May 11, 2016

Merge pull request #3 from ares/master

b16868a

Add spool dir instructions for git users

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #13013 - Adds API to display pulp dir status #3

Fixes #13013 - Adds API to display pulp dir status #3

shlomizadok commented Jan 6, 2016

ohadlevy Jan 6, 2016

shlomizadok Jan 6, 2016

ohadlevy Jan 6, 2016

shlomizadok Jan 6, 2016

ohadlevy Jan 6, 2016

ohadlevy Jan 6, 2016

shlomizadok commented Jan 6, 2016

ohadlevy Jan 6, 2016

lzap Jan 6, 2016

ohadlevy Jan 7, 2016

lzap Jan 12, 2016

lzap commented Jan 12, 2016

shlomizadok commented Jan 12, 2016

lzap commented Jan 12, 2016

lzap commented Jan 12, 2016

ohadlevy Jan 12, 2016

shlomizadok Jan 12, 2016

shlomizadok Jan 12, 2016

dmitri-d Jan 12, 2016

shlomizadok Jan 12, 2016

dmitri-d commented Jan 12, 2016

shlomizadok commented Jan 12, 2016

dmitri-d Jan 12, 2016

shlomizadok Jan 12, 2016

dmitri-d Jan 12, 2016

dmitri-d commented Jan 12, 2016


		def test_returns_pulp_disk_on_200
		PulpProxy::Plugin.load_test_settings(:pulp_dir => ::Sinatra::Application.settings.root)

Fixes #13013 - Adds API to display pulp dir status #3

Fixes #13013 - Adds API to display pulp dir status #3

Conversation

shlomizadok commented Jan 6, 2016

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

shlomizadok commented Jan 6, 2016

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

lzap commented Jan 12, 2016

shlomizadok commented Jan 12, 2016

lzap commented Jan 12, 2016

lzap commented Jan 12, 2016

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

dmitri-d commented Jan 12, 2016

shlomizadok commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jan 12, 2016