Skip to content

Shrink unused subnets #180

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2016
Merged

Shrink unused subnets #180

merged 1 commit into from
Aug 18, 2016

Conversation

@axelaris
Copy link
Contributor

@axelaris axelaris commented Aug 2, 2016

Hi there!
In many cases we want to have a quite simple deployment in one single AZ. However, we have to keep descriptions for all resource pools and all 3 subnets in the manifest stub. This makes the stub file overloaded and looks dirty. Otherwise manifest generation script does not work.

This PR adds a few fixes into diego.yml, which makes it possible to keep stub file as clean as possible.

@cfdreddbot
Copy link

cfdreddbot commented Aug 2, 2016

Hey axelaris!

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.

1 similar comment
@cfdreddbot
Copy link

cfdreddbot commented Aug 2, 2016

Hey axelaris!

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.

@cf-gitbot
Copy link

cf-gitbot commented Aug 2, 2016

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/127546135

The labels on this github issue will be updated when the story is started.

@emalm
Copy link
Contributor

emalm commented Aug 3, 2016

Thanks, @axelaris! I'm concerned about the potentially surprising behavior of copying the z1 configurations over to the other zones. I think it would work better to have a default empty-list ([]) value for the subnets, and a default empty-hash value for the cloud properties on each job. I tried this out with the default single-AZ BOSH-Lite deployment, and it does allow that iaas-settings.yml stub file to be simplified somewhat by removing all the cloud properties and the diego2 and diego3 network sections. Would those empty defaults also work to simplify your single-AZ OpenStack deployment?

If that does work as a change, please note that because of a spiff idiosyncrasy, you can't use a literal {} as an empty hash. We have run into that problem elsewhere in manifest-generation, though, and so there's already an empty_hash helper available in the diego.yml template.

Thanks,
Eric, CF Runtime Diego PM

@axelaris
Copy link
Contributor Author

axelaris commented Aug 10, 2016

Hi @ematpl,
thank you for suggestion.
It seems to be working fine for Openstack!

@emalm
Copy link
Contributor

emalm commented Aug 10, 2016

Hey, @axelaris, glad to hear that works! I've prioritized your PR for the team to review and pull in. In the meantime, since the final set of changes is small in scope, could you please squash those changes down to a single commit? You should be able to force-push over your branch and the PR will update automatically.

Thanks again,
Eric

@axelaris
Copy link
Contributor Author

axelaris commented Aug 10, 2016

@ematpl squashed last 2 commits.
Have no idea how to squash first two, I even can't see some of them in git log

@jenspinney
Copy link
Contributor

jenspinney commented Aug 11, 2016

Hi @axelaris, the additions use empty defaults for the non-Z1 AZs looks fine, but we would rather not remove the z2 and z3 definitions from the example openstack stub.

Would you be able to remove that file from this PR?

Also, instead of merging, can you do a rebase and force push to your PR branch? Right now, there are additional merge commits that we wouldn't want to pull in.

Thanks!
@jenspinney and @jfmyers9, CF Diego team members

@axelaris
Copy link
Contributor Author

axelaris commented Aug 12, 2016

Hi @jenspinney,
rebased and restored z2 and z3 in example openstack file.
Please take a look now.

@jenspinney
Copy link
Contributor

jenspinney commented Aug 12, 2016

Hi @axelaris, thanks for the rebase! It looks like the removal of z2 and z3 in the example file is still there though?

@axelaris
Copy link
Contributor Author

axelaris commented Aug 12, 2016

I've restored z2 and z3 subnets.
Would you like to restore z2-z3 resource pools also? This makes stub file quite overloaded :-(

@jenspinney
Copy link
Contributor

jenspinney commented Aug 12, 2016

It would match our examples for AWS closer if we kept the resource pools for other AZs (see here for example: https://github.com/cloudfoundry/diego-release/blob/develop/examples/aws/templates/diego/iaas-settings-internal.yml). I think for an example document, it's better to nudge people towards using a multi-AZ deployment. I agree with your change that we should support single-AZ, but by default I would prefer pushing people in the direction of deploying a Diego in a way that promotes high availability and provides the safety of a multi-AZ deployment.

@axelaris
Copy link
Contributor Author

axelaris commented Aug 17, 2016

Hi @jenspinney,
I understand your position, but when you studying, it's often you do not have a much resources (just like in my case) to run a high-available deployment.
Would do you like a new stub design? I retained both z2 & z3 descriptions, but formatted them in a way to ease cut them off.

@bdshroyer
Copy link

bdshroyer commented Aug 18, 2016

Looks good. Thanks, @axelaris !

@bdshroyer bdshroyer merged commit 9a9ee6f into cloudfoundry:develop Aug 18, 2016
jvshahid added a commit that referenced this pull request Jun 18, 2018
@jvshahid
bump diego-ssh uaa-go-client jwt-go
4081347 
[finishes #155485548](https://www.pivotaltracker.com/story/show/155485548)

Submodule src/code.cloudfoundry.org/diego-ssh 9b3f460..1174556:
  > Upgrade jwt-go library.
Submodule src/code.cloudfoundry.org/uaa-go-client 0c176509..26b271e3:
  > Ignore validation error when token used before issued
  > go back to using submodules from routing-release
  > Provide correct url
  > Update README
  > all tests passing with newer JWT
  > fixing vet errors
  > Merge pull request #10 from cloudfoundry-incubator/vendor-deps
  > Revert "remove incubator"
  > remove incubator
Submodule src/github.com/dgrijalva/jwt-go f62f64ea..06ea1031:
  > documentation around expected key types
  > Merge branch 'master' of github.com:dgrijalva/jwt-go
  > add options to ParseFromRequest
  > fixed a formatting error in a test
  > documenting changes for upcoming 3.2.0 release
  > Merge pull request #152 from pusher/parse-unverified
  > Merge pull request #219 from geertjanvdk/feat/parse
  > Merge pull request #205 from zamicol/icon_godoc
  > Merge pull request #209 from zhyuri/patch-1
  > Merge pull request #220 from polarina/readme-alt-include
  > Notice about upcoming 4.0.0 release
  > 3.1.0 changelog
  > Merge pull request #218 from zoofood/patch-1
  > updated note on alg type vulnerability
  > Merge pull request #183 from hnakamur/support_rs256_in_jwt_command
  > Merge pull request #196 from dgrijalva/dg/cmd_args
  > Merge pull request #190 from jamesrwhite/patch-1
  > Merge pull request #180 from kevinburke/fix-unreachable
  > Merge pull request #166 from johnlockwood-wf/issue-165-missing-arg
  > Merge pull request #151 from zaichang/FixMigrationGuide
  > Merge pull request #146 from pkieltyka/master
  > Merge pull request #140 from kazhuravlev/patch-1
  > Merge pull request #77 from dgrijalva/release_3_0_0
  > v2.7.0
  > notice about imminent 3.0.0
  > Merge pull request #136 from bruston/keyfunc-typo
  > fixes #135 copy/paste error in rsa decoding tools
  > Merge pull request #132 from abourget/master
  > Merge pull request #133 from johnlockwood-wf/expire-delta
  > release notes
  > expose inner error within ValidationError
  > Merge branch 'master' of https://github.com/emanoelxavier/jwt-go-contr into dg/merge_112
  > cleaned up style and added tests
  > Merge branch 'master' of https://github.com/dakom/jwt-go into dg/pr_121
  > version history update
  > Merge pull request #79 from dgrijalva/dg/none
  > Merge pull request #122 from appleboy/patch-1
  > add 1.6 to travis.yml
  > Merge pull request #107 from Snorlock/bearer-verification
  > Merge pull request #111 from matm/master
  > added supported signing methods
  > Added some clarification and (hopefully) helpful documentation
  > version history
  > signature should be populated after parsing a valid token
  > Merge pull request #98 from dgrijalva/dg/parser
  > use cleaner version of prefix checking (thanks shurcooL)
  > fix array OOB panic (#100)
  > Merge pull request #93 from EnerfisTeam/master
  > Merge branch 'master' of github.com:dgrijalva/jwt-go
  > minor refactor of HMAC verify for legibility.  no functional changes
  > updated documenation of SigningMethod interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@axelaris @cfdreddbot @cf-gitbot @emalm @jenspinney @bdshroyer