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

Don't stage on stagers without enough disk #202

Closed
wants to merge 2 commits into from

Conversation

julz
Copy link
Contributor

@julz julz commented Apr 7, 2014

Consider available_disk reported by stager before making staging request. This avoids errors which occur when cloud controller attempts to stage on a DEA that's out of disk. The same logic already exists for find_dea, this PR adds the same checks to find_stager.

Relies on cloudfoundry-attic/dea_ng#117 and cloudfoundry-attic/cf-release#346

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: http://www.pivotaltracker.com/story/show/68994902. This repo is managed by the 'Runtime' team.

app.disk_quota = 123
app.memory = 1024
config_hash[:staging][:minimum_staging_disk_mb] = 122
stager_pool.should_receive(:find_stager).with(app.stack.name, 1024, 123).and_return(stager_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since app.memory is not the focus of these "staging disk requirements" tests, can you repeat the pattern you used in the "staging memory requirements" test block and pass anything in as the memory input? For example:
stager_pool.should_receive(:find_stager).with(app.stack.name, anything, 123).and_return(stager_id)

Consider available_disk reported by stager before making staging
requests. This avoids errors which occur when cloud controller attempts
to stage on a DEA that's out of disk.
@julz
Copy link
Contributor Author

julz commented Apr 7, 2014

good point, done

@khwang1
Copy link
Contributor

khwang1 commented Apr 7, 2014

@MarkKropf, the changes look fine and there are good tests. Can we move it to the Runtime tracker project?

There are a few other PRs all related to this feature:
#201
cloudfoundry-attic/dea_ng#117
cloudfoundry-attic/cf-release#346

CF Community Pair (@khwang1 & @chou)

@MarkKropf
Copy link
Contributor

@khwang1 Yes, thank you

@julz
Copy link
Contributor Author

julz commented Apr 10, 2014

Hi @MarkKropf - while checking on the status of this PR I noticed https://www.pivotaltracker.com/story/show/69204488 relatively high up in the public tracker which seems to be asking for the same functionality implemented here (sorry about that, I wouldn't have started this if I'd noticed that there first); just flagging up to avoid duplicated effort in case someone picks that one up without realising this is here - thanks!

@MarkKropf
Copy link
Contributor

Thanks @julz I've commented in the story to check this PR to see if we can deliver that story by merging your contribution.

@jfmyers9
Copy link
Contributor

Hi @julz,

Thanks for submitting this. We merged this in today.

@jfmyers9 & @wsxiaozhang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants