-
Notifications
You must be signed in to change notification settings - Fork 713
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
Initial Foundations as a Service PR #194
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
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 looks like a good start overall. Various comments inline.
For the networking template, I also hoped to have a single overall template which could be called to create the networks and subnets together - similar to how the networking Terraform module works.
community/faas/README.md
Outdated
@@ -0,0 +1,127 @@ | |||
# Creating Projects Through Deployment Manager |
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 seems to be related to the project factory template, which is not in this folder or PR. Can we remove unrelated information?
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.
fixed
community/faas/docs/development.md
Outdated
make clean-all | ||
``` | ||
|
||
To **Only** cleanup the tmp/cache files (keeping the *virtualenv* and the |
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.
Only -> only
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 catch
@@ -0,0 +1,40 @@ | |||
# Networks and subnets |
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.
Network template(s) should go into a nested directory - templates/networks.
This file should be the README in that directory.
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 directory structure be addressed later on @morgante ?
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.
Yes, please do in a follow-up.
|
||
### Deployment | ||
|
||
#### Create |
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'd like to see a full usage example (how you write the yaml config as 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.
Yep. Adding links to example configs, and looking further in the existing examples folder for a good full usage example. The one I've used as model are quite skimpy in details (much less than this file). If you have a specific model you want us to follow, please point us to it and we'll use that as a template for these docs.
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 think the example itself is fine, would just like explanation of how to actually use it. ie. download the template and example, modify which values, etc.
|
||
See `properties` section in the schema files | ||
|
||
- [network](../../templates/network.py.schema) |
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.
Is there any way to auto-generate a nice property explanation in the markdown? Like terradoc.
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's the idea, but I don't think we're going have time get this done on phase 1. Can we log this task in a ticket and address it later for all templates
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.
Yes, please track for later.
Paths are important. Always run the test from the root of the `faas` project: | ||
|
||
``` | ||
./tests/integration/network.bats |
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 think we should compartmentalize tests. Ex. ./network/tests/integration/network.bats. @ocsig 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.
Agree.
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.
For now, I'd suggest is not to compartmentalize them just yet until this project gets a little bit more mature. If we want to reuse testcases/fixtures, the implementation will be quite a bit more complex, pressuring our timelines. Please let us know if this is a must do for this PR.
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.
Fine.
community/faas/examples/network.yaml
Outdated
# Example on how to use the network and subnetwork templates | ||
# | ||
# Variables: | ||
# RAND: Just a random string used mostly by the testing suite. |
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.
Do we actually need to use RAND for tests? i think we can reuse the same network name each time. Examples would be cleaner without it.
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 reason for this is when two or more people need to fire the same tests against the same project or org. This not only helps firing tests in parallel, but later the reuse of testcases and fixtures for when building more complex/stacked tests.
This also helps quite a bit on the speed of development, since we don't need to wait for a deployment to be deleted in order to run the same test again.
My 2c is to keep this only for the great flexibility/speed it gives us, but think of a way to make this prettier later on. What do you think?
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 see those reasons, but ultimately think the downside of having a more confusing example outweighs the benefits.
Regarding firing tests in parallel, different people should be doing it on different projects. I'd assume you'd delete the existing deployment before rerunning anyways.
I would like to see this fixed for this PR.
properties: | ||
- networkUrl: | ||
type: string | ||
description: The selfLink URL for the network resource |
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.
Should we also have a name output?
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.
Yep. Will add that.
'network': context.properties['network'], | ||
'ipCidrRange': context.properties['range'], | ||
'region': context.properties['region'], | ||
'privateIpGoogleAccess': True |
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.
privateIpGoogleAccess should be configurable.
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.
Adding it...
{ | ||
'type': 'compute.v1.subnetwork', | ||
'name': context.properties['name'], | ||
'properties': |
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.
Need to add support for secondary ranges as 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.
Will do.
686bacb
to
ab828bf
Compare
I signed it! |
CLAs look good, thanks! |
26b46cd
to
06d69a0
Compare
community/faas/examples/network.yaml
Outdated
ipCidrRange: 10.0.1.0/24 | ||
- rangeName: my-secondary-range-2 | ||
ipCidrRange: 10.0.2.0/24 | ||
- name: test-subnetwork-2 |
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.
Example shouldn't include both formats. Drop the outside subnets block and just embed them in the network resource.
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.
fixed
Paths are important. Always run the test from the root of the `faas` project: | ||
|
||
``` | ||
./tests/integration/network.bats |
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.
Agree.
# ${PYTHON} -m pip install -e . | ||
${PYTHON} -m pip install -r requirements/development.txt | ||
${PYTHON} -m pip install -r requirements/install.txt | ||
git clone https://github.com/sstephenson/bats.git && cd bats && ./install.sh ../virtualenv |
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.
Tested it on Cloud console: "/usr/bin/env: ‘bats’: No such file or directory"
Something is not correct with the installaton.
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 catch! I didn't use the cloud console. Lemme try that and report 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.
I tested this using the Cloud Console and it worked. Though I noticed the documentation needed to be improved. Could you check it out again please?
|
||
### Deployment | ||
|
||
#### Usage |
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 add a code version of the steps below as 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.
Not sure I understand what you mean by "code version" in this context... mind giving me an example?
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.
git clone https://github.com/GoogleCloudPlatform/deploymentmanager-sample
cd community/faas
cp examples/network.yaml project-network.yaml
##Changing network.yaml
gcloud deployment-manager deployments create <YOUR_DEPLOYMENT_NAME> --config network.yaml
Something like this
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.
fixed
|
||
|
||
@test "Creating deployment ${DEPLOYMENT_NAME} from ${CONFIG}" { | ||
run gcloud deployment-manager deployments create ${DEPLOYMENT_NAME} --config ${CONFIG} --project ${FAAS_PROJECT_NAME} |
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 creation step can fail. It should be checked if there is any DM error returned or the creation was successful.
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.
Removed the run
so, if gcloud deployment-manager deployments create
fails, bats will spit out an error. Same for gcloud deployment-manager deployments delete
later on this fine
05943db
to
435aaa1
Compare
* FAAS initial commit * fixes for PR-194
* Update Contributing read me Making it clear where community examples should be published. * Add redis and filestore resource snippets * Load the module by full name (#198) * Load the module fully * Fix spacing * updating master config fixing typo in config property. * Template to create a cloudsql instance, list of databases and users (#189) * Template to create a cloudsql instance, list of databases and users and import sqldump file from public shared file, for more info README.md * Change my email address * change google-container-manifest to gce-container-declaration and added serviceAccounts (#187) * Change the dependencies to fix the operationInProgress Error (#186) * change the dependencies of the replicas to avoid operationInProgress Error * add failover * fix the problem with the replica names (-replica-0-1 instead of -replica-1) * change the dependencies in replicas and failover to avoid operationInProgress Error * fixes: - dependencies with a variable weren't working - failover was always deployed * fix failover section * Add snippets for monitoring v3 * Add folder example for crm-v2 (#215) * upgrade cloud function sample to use cloud functions v1 provider * Support for centos in HTCondor deployment solution (#216) * Added support for centos, different versions of the OS, and different (stable) versions of htcondor * Added support for centos, different versions of the OS, and different (stable) versions of htcondor * Using centos 7 (instead of 6) as the default in the yaml file * Change subnetwork sample to have secondaryIpRanges * Hierarchical configuration v2 (#221) * Hierarchical configuration example * fixing comments * fixing comments * Update diagram * Moving example under community folder * Separating 'basic' and 'Organization with departments' practice. Implementing both of them. * Deletion of extra files. * Removing hidden files * System with external project creation * Helper function example version 1 * Update Actions release to 2018Q2 Actions feature release is currently 2018 Q2. * debian 8>9, import fixes, naming fixes * Applying glob import style * Style check * Adding makefile, stylecheck and integration tests * gitignore to ignore bats and virtualenv installations * Entry point readme * typo * Added tests, removing actual project creation * Sanitize config * Fixing tests * typo * Initial Foundations as a Service PR (#194) * FAAS initial commit * fixes for PR-194 * Add logsink template (#199) * Add cloud router template (#202) * Add cloud router template * Add org policy (#203) * Add Org Policy * Add firewall template (#201) * add firewall template * Add iam custom role template (#204) * Initial check in for iam custom role template * Add route template (#207) * Add route template * Initial check in for vpn template (#208) * Add iam member template (#210) * Add iam member template * Add shared vpc subnet iam template (#211) * Add shared vpc subnet iam template * Added role input. Added outputs. * FAAS Rename and restructure directories by templates (#213) * Renamed FASS directory to cloud-foundation * All FAAS references to cloud-foundation * Restructure all templates into individual directories * Restructure directories by templates * Correct link in testing documentation to reflect new directory structure * Renamed all files that use dashes to underscores for consistency. Documentation and testing updates to reflect file name changes. * Add folder template (#214) * Add project template (#206) Project Factory template * Create Pub/Sub template (#224) * Add Pub/Sub template * Add VM instance template (#225) * Add template for VM instance * Create service accounts and grant project/subnet IAM permissions (#222) creating service accounts with roles and creating subnet. * fixing schema file (#233) corrected schema file. * Added DNS managed zone template (#229) Added DNS managed zone template * Added DNS Records Template. (#230) DNS Records Template * Missing name tag for import resource. * Update requirements.txt (#240) * Template to create a storage bucket, copy files and add an acl. (#190) * Added region to sql instance (#228) * use service account instead of arbitrary email for patch IAM sample * fix prefix for service account * Corrected typo on README file * Clarifying warning (#280) Plus, adding a caution that the DM service account gets elevated privileges. * adding support to set pre-emptible instances on instance templates (#304) * HTCondor autoscaler for GCE (#296) * Removing fluentd config to gather condor jobs stats: condor-jobs.conf is not working * First import into github * autoscaler instructions * Refined argument passing and instructions * Fixed bug where make all was trying to build an unexistent file * Adding option to disable the autoscaler for the MIG, in case we are using the expernal autoscaler.py to control the scale * Fixing autoscaler * Adding a note to use setup_autoscaler=false in the condor-cluster properties. * Fixed spelling * Fixed a bug with scaling up Bug: When jobs are in the queue but not assigned to compute instances yet, autoscaling script should not shut down instances that are not handling jobs * Added option to specify max number of compute instances Added "--computeinstancelimit" option * Added argument to specify compute instance limit to the documentation * Fixed text formating problem * Improved script argument handling Added short form to represent arguments Removed --debug to --verbosity * Fixed argument definition * Updated example of calling the script * Added adjustment for the on-hold jobs On-hold jobs will not be counted when calculated job queue length and resources will not be allocated for jobs that are on-hold * generilized example invocation * Grammar improvements - Grammar improvements - Table with the list of arguments updated * Fixed argument name * Integrated feedback on the documentation after peer review and test by Jamie Kinney * Updating Cloud Functions type from v1beta2 to v1. (#322) * Updating Cloud Functions type from v1beta2 to v1. * Adding `parent` property * Added `parent` property * Add cloudfunctions v1beta2 configs back in temporarily * Remove unnecessary location field from cloudfunctions-v1 resource snippet template. (#325) * Add folder example for crm-v2 * Remove unnecessary location field from cloudfunctions resource template. * Updating Readme with Cloud Foundation Toolkit (#311) * Updating Readme with Cloud Foundation Toolkit * Readme extension with CFT (#327) * Updating Readme with Cloud Foundation Toolkit * add access context manager samples (#340) * Cloud foundation - phase 3 merge to master (#343) * Added `VCP Peering` template * Updated `Project` template documentation * Added `Cloud Build and Build Trigger` template * Added `Managed Instance Group` template * Updated `Health check` template documentation * Add Internal Load Balancer template (#298) * Add Internal Load Balancer template * Added `Dataproc` template * Updated `BigQuery` template documentation * Updated `GCS Bucket` template documentation * Add Cloud Key Management Service (KMS) Template (#266) * Add Cloud KMS Template * Normalizing output variables across templates * CFT deployment tool (#289) CFT utility * Add bastion host template (#308) * Add Cloud SQL template (#317) * Add logsink support for org/billing account/project/folder (#315) * Add logsink support for org/billing account/project/folder * GCS_bucket merge issue fix * Fix PR#333 Logsink destination is storage, not bucket (#336) * Interconnect Attachment Template (#305) * Interconnect Template (#312) * Add external load balancer template (#307) * Add external load balancer template * Addressed feedback * Added `Spanner` template * Add Forseti template (#328) * Add Forseti template * Added `Cloud Runtime Configurator` template * hotfix incorrect installation directory for bats * Added `Stackdriver Metric Descriptor` template * Added `Cloud Tasks` template * CFT test - GKE version hotfix * GKE version update to latest for tests * Hotfix networkURL > selfLink test * CFT - GKE - cluster version assert remove Test was failing because of latest cluster version can't be defined. * NAT Gateway Template (#310) * NAT Gateway Template * Add feedback * Remove setting of project metadata for bats testing * Update ip_reservation.bats * CFT - Test - gcloud beta install removal gcloud beta is a pre-requironment and makes the test fail. * Fix PR#334/330/329 (#342) * Doc update - merged to master Reference to Cloud Foundation repo removed because we merged to master. * Shared VPC projects can be created in a folder (#352) * Shared VPC projects can be created in a folder Fix #351
This PR contains the introduction of the "Foundations as a Service" (FAAS) project to the repository, and the review by the Google staff working on this project is requested.
In order to make the initial review of the structure, coding style, testing suite, and documentation easier, this PR is only including one piece of functionality (network template). Once issues with this PR are addressed and the team is comfortable with the standards, other templates will be promptly presented in a one-PR-per-template fashion.
Files related to FAAS should be found only in the community/faas directory. Nothing outside of this directory is expected in the near future.