You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Yaron Rosenbaum <no...@github.com> on 2015/04/12 09:11:53 UTC
[jclouds-labs-google] Add support for disks & ip forward during node
creation (#135)
Hi
This adds support for specifying disks during node creation, either by adding specific disks to templateOptions, or by using a method called GCETemplateOptions.autoCreateDisk(..) which is useful in case multiple nodes are created using a single templateOptions.
Also added support for the canIpForward flag (defaults to false).
Both have been manually tested and verified on GCE.
Both changes are backwards compatible.
I'm not sure whether unit tests are required for this change, but if so - I would need more guidance.
I would like to ask for this to be added to 1.9.1, since this is a minor change with very minimal risk, and using GCE without it is impossible in my case.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs-google/pull/135
-- Commit Summary --
* update gitignore
* Initial impl: Support adding attached disks during creation
* Merge branch 'master' of github.com:jclouds/jclouds-labs-google into Add-disk-template-options-back
* Trying to support canIpForward as well..
* Added support for ipForwarding
* update unit tests with new api.
-- File Changes --
A google-compute-engine/.gitignore (2)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (449)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/options/GoogleComputeEngineTemplateOptions.java (108)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/NewInstance.java (6)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiLiveTest.java (5)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiMockTest.java (1)
A googlecloud/.gitignore (2)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/135.patch
https://github.com/jclouds/jclouds-labs-google/pull/135.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by danbroudy <no...@github.com>.
> + }
> +
> + // Add metadata from template and for ssh key and image id
> + newInstance.metadata().putAll(options.getUserMetadata());
> +
> + LoginCredentials credentials = resolveNodeCredentials(template);
> + if (options.getPublicKey() != null) {
> + newInstance.metadata().put("sshKeys", format("%s:%s %s@localhost", credentials.getUser(), options.getPublicKey(), credentials.getUser()));
> + }
> +
> + String zone = template.getLocation().getId();
> + InstanceApi instanceApi = api.instancesInZone(zone);
> + Operation create = instanceApi.create(newInstance);
> +
> + // TODO: What does this AtomicReference do?
> + // TODO: What's the differenc between AttachDisk and Instance.AttachedDisk and can they be refactored together?
Refactoring Instance.AttachedDisk and AttachDisk to be one object is a good idea. I'm not sure what this AtomicReference does, the comment is a little cryptic.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28214376
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Yaron Rosenbaum <no...@github.com>.
@danbroudy I feel silly, but I need some help with this.
I have no idea why git decided to drop pom.xml, and what's worse - I don't know how to add it back..
Can you please help me out?
Thanks :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-112749790
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
@yaronr can you have a look and fix the PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-101964330
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by danbroudy <no...@github.com>.
> +
> + public AutoCreateDiskOptions getAutoCreateDiskOptions() {
> + return autoCreateDiskOptions;
> + }
> +
> + /**
> + * @return the AttachDisks for this instance.
> + */
> + public List<AttachDisk> getDisks() {
> + return disks;
> + }
> +
> + // /**
> + // * @see #shouldKeepBootDisk()
> + // */
> + // public GoogleComputeEngineTemplateOptions keepBootDisk(boolean
In general there isn't commented out code in the repo so these will have to go before check-in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28214395
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Yaron Rosenbaum <no...@github.com>.
> + this.diskMode = diskMode;
> + this.isBootDisk = isBootDisk;
> + this.diskSizeGb = diskSizeGb;
> + }
> +
> + public AutoCreateDiskOptions(final AttachDisk.Type diskType, final AttachDisk.Mode diskMode, final boolean isBootDisk, final int diskSizeGb, final String diskName) {
> + this(diskType, diskMode, isBootDisk, diskSizeGb);
> + // TODO: Validate against regex according to GCE spec.
> + this.diskName = diskName;
> + }
> +
> + public String getDiskName(final String nodeName) {
> + if (null != diskName)
> + return diskName;
> + else
> + return "jclouds-" + UUID.randomUUID().toString();
Hi Dan
It’s not necessary, but it would be nice.
Problem is, it’s impossible without changing stuff in the generic API.
Today, there’s an API to supply node names in the portable api’s templateOptions, and then create multiple nodes with a single call. Say for example, I set nodeNames to “node1,node2,node3” and then create 3 nodes using the same templateOptions, the nodes will be called node1..3.
To have the disk names include the node names (BTW, not always possible - in GCE disk name is governed by a regex) would require significant changes (I haven’t done a full feasibility research though).
(Y)
> On Apr 13, 2015, at 8:00 AM, danbroudy <no...@github.com> wrote:
>
> In google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/options/GoogleComputeEngineTemplateOptions.java <https://github.com/jclouds/jclouds-labs-google/pull/135#discussion_r28214469>:
>
> > + this.diskMode = diskMode;
> > + this.isBootDisk = isBootDisk;
> > + this.diskSizeGb = diskSizeGb;
> > + }
> > +
> > + public AutoCreateDiskOptions(final AttachDisk.Type diskType, final AttachDisk.Mode diskMode, final boolean isBootDisk, final int diskSizeGb, final String diskName) {
> > + this(diskType, diskMode, isBootDisk, diskSizeGb);
> > + // TODO: Validate against regex according to GCE spec.
> > + this.diskName = diskName;
> > + }
> > +
> > + public String getDiskName(final String nodeName) {
> > + if (null != diskName)
> > + return diskName;
> > + else
> > + return "jclouds-" + UUID.randomUUID().toString();
> Do you think it makes sense to include some aspect of the nodeName in the diskName? I am still figuring out the jclouds resource naming conventions/scheme.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28214469>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28250783
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by danbroudy <no...@github.com>.
Whats the status here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-101833908
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Yaron Rosenbaum <no...@github.com>.
@danbroudy Hi mate
Sorry, I am swamped with work. I'll take a look.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-112717402
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
It seems that your last commit has removed the pom.xml. Can you fix that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-92691151
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by danbroudy <no...@github.com>.
> + @Override public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String group, String name, Template template) {
> + GoogleComputeEngineTemplateOptions options = GoogleComputeEngineTemplateOptions.class.cast(template.getOptions());
> + checkNotNull(options.network(), "template options must specify a network");
> + checkNotNull(template.getHardware().getUri(), "hardware must have a URI");
> + checkNotNull(template.getImage().getUri(), "image URI is null");
> +
> + List<AttachDisk> disks = Lists.newArrayList();
> + disks.add(AttachDisk.newBootDisk(template.getImage().getUri()));
> +
> + disks.addAll(options.getDisks());
> + if (null != options.getAutoCreateDiskOptions()) {
> + AutoCreateDiskOptions diskOptions = options.getAutoCreateDiskOptions();
> + checkArgument(template.getLocation().getScope() == LocationScope.ZONE);
> + DiskApi diskApi = api.disksInZone(template.getLocation().getId());
> + Operation op = diskApi.create(diskOptions.getDiskName(name), new DiskCreationOptions.Builder().sizeGb(diskOptions.diskSizeGb).build());
> + // TODO: Is there a way to avoid the op, and just AttachDisk.create? where do we get the URI from? does it have to pre-exist?
I don't think there is a way to avoid the operation. You have to create the disk before attaching it.
https://cloud.google.com/compute/docs/instances#startinstanceapi
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28214283
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
The commit that removes the `pom.xml` is the last one, so the easiest path to redo it is to reset to the previous one, and discard the `pom.xml` change made in that commit. The following should work:
```bash
git reset HEAD~1 # Go back to the previous commit but keeping the changes
git checkout -- google-compute-engine/pom.xml # Discard the changes in this file
# Commit the remaining files again
git add .
git commit -m "trying to minimize formatting differences even more..."
```
Note that you will need to push-force to your fork because of the commit reset.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-112794403
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Yaron Rosenbaum <no...@github.com>.
@nacx @danbroudy hey guys. looks like i've fixed it. thanks
Please let me know if you could merge this, this is really critical stuff.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-113871575
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Yaron Rosenbaum <no...@github.com>.
Why do you need a pom?! :)
Sure.
Sorry for the delay, I didn’t see this email
(Y)
> On Apr 14, 2015, at 11:20 AM, Ignasi Barrera <no...@github.com> wrote:
>
> It seems that your last commit has removed the pom.xml. Can you fix that?
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-92691151>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-97377201
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
Closed #135.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#event-341126344
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for fixing @yaronr. The build is failing and the PR is not mergeable, as the branch has diverged from the master version and there are conflicts.
Could you squash all the commits in the PR into a single one, and then rebase it to the latest version of master? This way we'll be able to see the real diff and fixing any eventual build failure will be much easier.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-114257852
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
The build has been failing for some days now, and the provider has been promoted to the main repository. Please, address the comments and reopen the pull request on the main jclouds repository: https://github.com/jclouds/jclouds
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135#issuecomment-115671678
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by danbroudy <no...@github.com>.
> + this.diskMode = diskMode;
> + this.isBootDisk = isBootDisk;
> + this.diskSizeGb = diskSizeGb;
> + }
> +
> + public AutoCreateDiskOptions(final AttachDisk.Type diskType, final AttachDisk.Mode diskMode, final boolean isBootDisk, final int diskSizeGb, final String diskName) {
> + this(diskType, diskMode, isBootDisk, diskSizeGb);
> + // TODO: Validate against regex according to GCE spec.
> + this.diskName = diskName;
> + }
> +
> + public String getDiskName(final String nodeName) {
> + if (null != diskName)
> + return diskName;
> + else
> + return "jclouds-" + UUID.randomUUID().toString();
Do you think it makes sense to include some aspect of the nodeName in the diskName? I am still figuring out the jclouds resource naming conventions/scheme.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28214469
Re: [jclouds-labs-google] Add support for disks & ip forward during
node creation (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + this.diskMode = diskMode;
> + this.isBootDisk = isBootDisk;
> + this.diskSizeGb = diskSizeGb;
> + }
> +
> + public AutoCreateDiskOptions(final AttachDisk.Type diskType, final AttachDisk.Mode diskMode, final boolean isBootDisk, final int diskSizeGb, final String diskName) {
> + this(diskType, diskMode, isBootDisk, diskSizeGb);
> + // TODO: Validate against regex according to GCE spec.
> + this.diskName = diskName;
> + }
> +
> + public String getDiskName(final String nodeName) {
> + if (null != diskName)
> + return diskName;
> + else
> + return "jclouds-" + UUID.randomUUID().toString();
I think the right approach there, if the name needs to match a given GCE convention is to use the same approach than used with the firewalls. Let's create a naming convention [like the one used for firewalls](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/FirewallTagNamingConvention.java) and [inject it in the compute service adapter](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java#L91). Then, change the disk name option to be an `Optional`, and let the compute service adapter generate the name using the convention. The created convention class itself can get the optional as a parameter and generate an appropriate name.
Note that the firewall naming convention already delegates to the jclouds default GroupNamingConvention to generate the prefix. That will already take into account the properties that rule the naming of the resources jclouds creates.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/135/files#r28309276