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 stack association to buildpack model [#153256959] #1064
Conversation
Hey idoru! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/154527662 The labels on this github issue will be updated when the story is started. |
4c8190d
to
da7220b
Compare
In this commit we had to add a new presenter class and it seems like there's already a bunch in here that have this |
da7220b
to
49b0a66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some initial reactions to some of the decisions made. You all did a great job.
## NOTE: if a requested system buildpack is not on the requested stack, | ||
## the BuildpackInfo object will have a name and not a record (even | ||
## though the buildpack exists). At this point the error returned | ||
## to the user will probably be VERY confusing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a test be written to demonstrate the behavior described here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect a better plan would be to improve the message. We could not work out how to do that however. If we can organise some cross team pairing I would love to work on that. @idoru who has the best context is out until Feb 26 so maybe if we could organise cross team pairing for that week?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have cross teams planned with Routing and CLI, so it will be difficult to start another one in the near term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message was clear to me. I find tests to be valuable in terms of documenting desired/known behavior, in this case documenting what error users will see. Leaving comments in the code that describe behavior is a last resort. Main motivation being that tests go red when things change, whereas many a comment has become obsolete silently. Tests do a good job of showing rather than telling. Lastly, this particular case feels straightforward to set up, but correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we just gave this a test, and the error we see is:
Staging app and tracing logs...
Unexpected Response
Response code: 500
CC code: 0
CC error code:
Request ID: 24dddc03-c91c-4fb8-4783-ac3b8568e49c::4ab4e09a-c73d-4466-a3e9-e5daffccf25e
Description: {
"description": "Stager error: no compiler defined for requested stack",
"error_code": "CF-StagerError",
"code": 170011
}
Coming back to comments made earlier by @dgodd : this error doesn't really inform the user well when they have specified a buildpack name, but there is no buildpack with that name for the relevant stack (either specified, or defaulted).
As outsiders to this code base it appears to us that this error handling is interwoven with the lifecycle, and as such we would really want a cross team to improve this. I suppose this could be acceptable in a first cut, and we could attack this as an improvement later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gerg ^^
|
||
buildpacks = Buildpack.where(name: name) | ||
if buildpacks.count > 1 | ||
raise AmbiguousBuildpackException.new("Buildpack #{name} has #{buildpacks.count} stacks, not updated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does 'not updated' mean in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially the update-buildpack
command failed. The context is that if the operator does not specify which version (stack) of the buildpack to update then the update fails. The cli will need to allow an operator to specify a stack before this works well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception is
- Raised from a private method
- Caught by the same class that raises it
- Not re-raised
This exception is only used for control flow in this class. I propose we either re-raise it (wrapped in an APIError) if it is an exceptional case or find another way of modeling the control flow without using exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A rewording to "update failed" is more clear to me, as long as it is not inconsistent with other buildpack errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gerg sure, we can remove the PBE. We don't want to re-raise the error since we don't want errors in buildpack installation to fail the deployment. We'd rather the operator see that a particular buildpack update failed and make fixes manually as needed.
buildpacks.first | ||
end | ||
|
||
class AmbiguousBuildpackException < RuntimeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to use 'Error' instead of 'Exception'. There are good reasons to distinguish between the two, such as people wanting to handle exceptions and execute a different code path, but in this case, it's ambiguous and sounds to me like an error. Is there a reason to keep the name the way it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we just thought we were following the current pattern in this case (very possibly we were incorrect). @idoru will have more context when he gets back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This problem should go away since we're going to avoid processing by exception now.
def buildpack_stack | ||
stacks = Buildpack.where(name: buildpacks_to_use).select(:stack).uniq | ||
if stacks.length == 1 | ||
stacks.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a buildpack is associated with more than one stack, what should this function return?
Additionally, is there a test for this behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual buildpacks can no longer be associated with more than one stack. Thus for each buildpack/stack combination a different zip file would need to be uploaded.
In this case the method is returning a possible answer for the stack if there is only one currently uploaded version of the buildpack. You can see the fallback strategy inside staging_stack
and I believe that is tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find the associated tests. I would expect them to be in ./spec/unit/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_spec.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's just 1 missing test, we'll add.
vendor/errors/v2.yml
Outdated
http_code: 400 | ||
message: "The buildpack name is already in use: %s" | ||
name: BuildpackNameStackTaken | ||
http_code: 409 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the http_code change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seemed to us to be a better plan since the meaning changed. We are happy for you to leave it the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did the meaning change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400 seemed wrong, it's meant for when the request could not be understood by the server.
409 seemed more appropriate, which indicates that there is a conflict with the resource.
Will changing this cause problems? If so we are happy to revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that 409 is more appropriate. Concern is that we break clients that rely on this behavior. @zrob thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not quite clear on what's happening from this snippet, but roughly, we should not change names/codes of existing errors. if a different error is appropriate we should add a new entry and use that. as far as 409 -- we have moved towards a convention of using 422 for this type of error and relying more on the text to describe the case than the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@selzoc and I took a second look today and had some more thoughts.
@@ -47,8 +50,31 @@ def upload_buildpack(buildpack, bits_file_path, new_filename) | |||
true | |||
end | |||
|
|||
def extract_stack_from_buildpack(bits_file_path) | |||
bits_file_path = bits_file_path.path if bits_file_path.respond_to?(:path) | |||
output, _, status = Open3.capture3('unzip', '-p', bits_file_path, 'manifest.yml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the second output is ignored, you could use capture2
instead
- @selzoc && maryam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certainly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, we would prefer to use the rubyzip
gem instead of shelling out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, we will refactor to use rubyzip instead.
create_request = { | ||
name: 'my_app', | ||
environment_variables: { open: 'source' }, | ||
lifecycle: { | ||
type: 'buildpack', | ||
data: { | ||
stack: nil, | ||
stack: buildpack.stack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test for the case where nil
gets passed into the request as the stack name?
- @selzoc && maryam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would test these permutations at the controller level. Request specs and generally happy path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think that buildpacks_controller_spec has a #create defaults stack to unknown when nil
at 49b0a66#diff-77441f6f04beceda9b6bb6c88e5a0643R64
@dgodd @jfeeny @TisVictress @idoru Just a ping in case y'all haven't seen our comments. |
Thanks for the ping, I hadn't seen the comments. Assuming you would like more changes, can we schedule a meeting for after @idoru is back? (Feb 26) |
Also another request. How do you make code climate pass? The complaint that it has is based off too many arguments to a method which we are subclassing? |
@@ -0,0 +1,30 @@ | |||
require 'yaml' | |||
|
|||
def default_stack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is some logic here, it would be nice to write a migration test.
See examples here: https://github.com/cloudfoundry/cloud_controller_ng/tree/master/spec/migrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Will do.
add_index [:name, :stack], unique: true, name: :unique_name_and_stack | ||
end | ||
|
||
self['UPDATE buildpacks SET stack = ?', default_stack || 'unknown'].update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would prefer to not use raw SQL when possible. Is there a way to write this using the Sequel migration DSL?
(Using raw SQL makes it difficult to expand to new databases, which is something we occasionally attempt to do.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll give it a shot and reach out if we can't.
if buildpack.nil? | ||
buildpacks_lock = Locking[name: 'buildpacks'] | ||
buildpacks_lock.db.transaction do | ||
buildpacks_lock.lock! | ||
buildpack = Buildpack.create(name: name) | ||
buildpack = Buildpack.create(name: name, stack: 'unknown') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like stack already defaults to 'unknown'
. If you want to be explicit here, please pull out a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that the default is defined on the controller rather than the model. Either way, we shouldn't have the same string in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gerg yeah having the defaults on the controller did seem odd, but position
, enabled
and locked
are all defaulted here as well and it doesn't appear any sort of defaulting is done in the model.
We're not very familiar with Sequel, so if you can recommend a better practice, we would be happy to incorporate.
c267a1b
to
5e83e58
Compare
Looking through the context both here and on slack, it seem like this PR is blocked on the merging of the fix to rubyzip - I don't think we want to use a fork. Another option that @Gerg noted would be for the stack to be passed as a parameter to the api instead of relying on unzipping stuff server-side. Thanks, |
I don't know if this might be relevant but you could use the ZIP reader we have in https://github.com/WeTransfer/zip_tricks as it opens these entries correctly |
We would prefer to validate the stack on the server side. A buildpack that is associated with the wrong stack can have confusing and unexpected failure modes during app staging or launch, especially if the stacks are similar (ex. cflinuxfs2 vs. cflinuxfs3). We're also concerned about the behavior of the cf Java client. We understand not wanting to merge this PR until the rubyzip PR is merged. We will wait on that PR for a few more days before considering other options. |
@sclevine Sounds good. We're good to wait on your PR to rubyzip/other potential options. |
Rubyzip PR is merged. We will update this PR shortly. |
@sclevine Are you ready for us to look at this again? |
end | ||
|
||
def default_stack | ||
stacks_yml['default'] if stacks_yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of stacks_yml
should be cached, even by doing something like:
@stacks_yml ||= begin
stacks_yml_path = ENV.fetch('STACKS_YML', nil)
YAML.safe_load(File.read(stacks_yml_path)) if stacks_yml_path && File.exist?(stacks_yml_path)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with @ericpromislow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand what is the concern with reading a fairly small yaml file to get a single key a handful of times during a single migration?
Implementing your suggestion appears to introduce global state, breaking the tests. This is the diff of what I applied:
--- a/db/migrations/20180404165800_assign_stacks_to_buildpacks.rb
+++ b/db/migrations/20180404165800_assign_stacks_to_buildpacks.rb
@@ -1,8 +1,10 @@
require 'yaml'
def stacks_yml
- stacks_yml_path = ENV.fetch('STACKS_YML', nil)
- YAML.safe_load(File.read(stacks_yml_path)) if stacks_yml_path && File.exist?(stacks_yml_path)
+ @stacks_yml ||= begin
+ stacks_yml_path = ENV.fetch('STACKS_YML', nil)
+ YAML.safe_load(File.read(stacks_yml_path)) if stacks_yml_path && File.exist?(stacks_yml_path)
+ end
end
def default_stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a dealbreaker. We only use that pattern when you new up a instance of a class so that the yaml isn't cached forever.
extracted_stack = Buildpacks::StackNameExtractor.extract_from_file(bits_file_path) | ||
new_stack = [extracted_stack, buildpack.stack, Stack.default.name].find(&:present?) | ||
|
||
new_stack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to store the result of the .find
in new_stack
. Just return it.
With @ericpromislow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Good spot. Updating code now...
- Add "stack" to buildpack model - At migration time, buildpacks will be assigned default stack from stacks.yml file if its path is set in the STACKS_YML env var - hwc_buildpack is assigned the newer of windows2016,windows2012R2 stacks if present in stacks.yml - if STACKS_YML is not set, buildpacks' stacks will be set to nil - Buildpacks are now unique over name AND stack - Sets buildpack stack from manifest.yml in buildpack zip on creation - Validate buildpack model stack against stack in buildpack zip manifest.yml - Validate stack exists upon buildpack bits upload - Include stack name in serialized buildpack filename - Only provide buildpacks for the relevant stack to the staging container - Handle buildpack stacks appropriately in the buildpack installer - Use fork of rubyzip/rubyzip to work around rubyzip/rubyzip#236 NOTE: The API checkshum has changed due to adding stack as an input Signed-off-by: Dave Goddard <dave@goddard.id.au> Signed-off-by: Victoria Henry <vhenry@pivotal.io> Signed-off-by: Jackson Feeny <jacksonfeeny@gmail.com> Signed-off-by: Tyler Phelan <tphelan@pivotal.io> Signed-off-by: Andrew Meyer <ameyer@pivotal.io> Signed-off-by: Leah Hanson <lhanson@pivotal.io>
What happens if user upload hwc_buildpack as 'my-hwc-buildpack' |
It isn't possible to associate already-uploaded buildpacks with their intended stack, so this feature will require manual intervention by operators on foundations with more than one stack. We believe the HWC buildpack (uploaded as @zrob reminder that this should be called out loudly in the release notes |
Y'all should probably have been pulled into this earlier. This is a feature being introduced in cc that will need to work with bit service also. |
to determine what stack should be assigned to existing buildpacks
NOTE: The API checkshum has changed due to adding stack as an input
Signed-off-by: Dave Goddard dave@goddard.id.au
Signed-off-by: Victoria Henry vhenry@pivotal.io
Signed-off-by: Jackson Feeny jacksonfeeny@gmail.com
Signed-off-by: Sam Coward scoward@pivotal.io
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
(see above)
An explanation of the use cases your change solves
Creating multiple buildpacks with the same name (as long as they have unique stacks).
Links to any other associated PRs
Migration
20180102183100_add_stack_to_buildpack_table.rb
requires theSTACKS_YML
environment variable to be set to the path of the CC'sstacks.yml
so it can identify the default stack to be assigned to existing buildpacks during the migration. See this capi-release PRI have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
master
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests