DRAFT: #33446 with alternate test style #229
Conversation
This change implements the WORKER_TTL setting which lets Pulp decide how long to wait for a worker to respond before considering it gone and cleaning up it's database and re-assiging work.
wbclark
changed the title
|
NOTE: I updated the parameter documentation to be clearer for users, and also tried a new style for the tests which should make it easier for developers to interpret test output. |
wbclark
force-pushed
the
wclark-33446-updated
branch
from
e05de29 to
9c7c41e
Compare
| its(:stdout) { is_expected.to match(/^USE_NEW_WORKER_TYPE = True/) } | ||
| its(:exit_status) { is_expected.to eq 0 } | ||
| describe "Pulpcore Settings" do | ||
| let(:settings) { command("PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager diffsettings") } |
There was a problem hiding this comment.
As written, this works and should give a more descriptive output in the case of test failures (I will break a test shortly to confirm).
That said, I want to make sure really I understand correctly how this behaves -- would the block argument here get evaluated in the let block, or does it later get evaluated three times when calling settings.exit_status, settings.stdout, etc, causing inefficient code execution?
What I'm trying to do may or may not still be possible with describe command("PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager diffsettings") do instead. I am not sure if its(:stdout) "will a string work here?" do would function as I intend it do.
From looking at a lot of rspec examples, expect(...) seems to be preferred to it { is_expected.to ... } which is why I decided to try this. That said, maybe it is proper rspec style in general but not right for serverspec specifically.
There was a problem hiding this comment.
I like the new grouping of things, and I think let should be evaluated once.
As for expect vs is_expected.to -- we use the later everywhere, so I'd prefer to keep that for consistency (and if change, change globally).
There was a problem hiding this comment.
What you can do is:
describe 'Pulpcore settings' do
subject { command("PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager diffsettings") }
its(:exit_status) { is_expected.to eq 0 }
endThis is effectively the same as what was there before.
There was a problem hiding this comment.
I think I recall trying something like this in the past which failed:
describe "Pulpcore settings" do
subject { command("PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager diffsettings") }
its(:exit_status) "exits without error" { is_expected.to eq 0 }
endI will try again to confirm
There was a problem hiding this comment.
The issue was
Failure/Error: __send__(method, file)
SyntaxError:
/home/wclark/Projects/puppet-pulpcore/spec/acceptance/basic_spec.rb:80: syntax error, unexpected string literal, expecting `end'
its(:exit_status) "retrieves pulpcore settings" ...
So it accepts an optional string argument before the block argument, but its(:symbol) can receive only the block.
There was a problem hiding this comment.
Thinking about this some more, what I really want to do is deliver a version of this PR which delivers the feature as needed, adhering to the current test style though it may be flawed.
I thought the tests would work in this style, and indeed they do, but I'm simply not confident enough yet with it to rule out drawbacks.
Therefore I will rename this PR and mark it as a draft, and shortly open another version that delivers the same functionality with the current style tests.
wbclark
changed the title
|
OK, the real version of this is #230 |
| # The number of seconds before a pulpcore worker should be considered lost. If undefined, pulpcore will use its default setting which is 30 seconds as of | ||
| # pulpcore version 3.14. You should not need to modify this setting unless the application reports workers timing out while they are busy completing tasks. |
There was a problem hiding this comment.
Reading "You should not need to modify this setting unless the application reports workers timing out while they are busy completing tasks." one could think "Yeah, let's set it to 9000 then!", which is also not what we want the users to do.
For worker_count we say " Modifying this parameter should be done incrementally with benchmarking at each step to determine an optimal value for your deployment.", should something similar be added here?
There was a problem hiding this comment.
I still think this direction is wrong. We should not try to document what the value is but rather how to find out what the default is. Remember that this module is not exposed to the user directly so I don't see the value. In my experience it only brings work in trying to keep two values in sync.
If documentation is an issue, we should take it up with upstream. For example, we could investigate deploying a man page for the pulp settings, generated in packaging so it's guaranteed to be in sync.
|
Does this need a rebase just for the testing part or to be closed @wbclark ? |
|
Closing for now, I couldn't make the tests work quite how I wanted them to while adhering to the established style of the project. |
Continuing work from #227