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/06/28 18:39:10 UTC
[jclouds] Add support for disks & ip forward during node creation
(#790)
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 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/pull/790
-- Commit Summary --
* Add support for automatically adding attached storage to instances upon creation. Add support for ip forwarding
-- File Changes --
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (29)
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/options/GoogleComputeEngineTemplateOptions.java (633)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/790.patch
https://github.com/jclouds/jclouds/pull/790.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -43,9 +50,13 @@ public void copyTo(TemplateOptions to) {
> super.copyTo(to);
> if (to instanceof GoogleComputeEngineTemplateOptions) {
> GoogleComputeEngineTemplateOptions eTo = GoogleComputeEngineTemplateOptions.class.cast(to);
> + eTo.networks(to.getNetworks()); // YR: this is not relevant for my PR, but I think not having it is a bug
This is already copied in the parent class
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33454169
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + 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. What is the GCE spec?
> + this.diskName = diskName;
> + }
> +
> + public String getDiskName(final String nodeName) {
> + if (null != diskName)
> + return diskName;
> + else
> + return "jclouds-" + UUID.randomUUID().toString();
> + }
> + }
This class has been completely reformatted. Please, keep the formatting it had (basically a 3 space indent, but have a look at the [jclouds code style](https://cwiki.apache.org/confluence/display/JCLOUDS/Coding+Standards)) so the only changes in it are the lines relevant to your patch.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33426312
Re: [jclouds/jclouds] Add support for disks & ip forward during node
creation (#790)
Posted by Yaron Rosenbaum <no...@github.com>.
Hey @andrewgaul
The enhancement / fix was essential in my case, worked flawlessly in production for me, and I think many people would find it useful. How the unit tests work was beyond me though, and I just didn't have the capacity to invest in that. So I would encourage you to take a look if you can, but I will not be able to properly complete this to the jClouds standards (which I respect)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-331688842
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
> + public List<AttachDisk> getDisks() {
> + return disks;
> + }
> +
> + /**
> + * @see #getDisks()
> + * @see org.jclouds.googlecomputeengine.domain.templates.InstanceTemplate.AttachDisk
> + */
> + public GoogleComputeEngineTemplateOptions disks(List<AttachDisk> disks) {
> + this.disks = Lists.newArrayList(disks);
> + return this;
> + }
> +
> +
> +
> + public static class AutoCreateDiskOptions {
Change this class to use google auto.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33454235
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + /**
> + * @see #getDisks()
> + * @see org.jclouds.googlecomputeengine.domain.templates.InstanceTemplate.AttachDisk
> + */
> + public GoogleComputeEngineTemplateOptions addDisk(AttachDisk disk) {
> + this.disks.add(disk);
> + return this;
> + }
> +
> + public void autoCreateDisk(final AutoCreateDiskOptions diskOptions) {
> + this.autoCreateDisks.add(diskOptions);
> + }
> +
> + public void autoCreateDisks(final List<AutoCreateDiskOptions> autoCreateDisks) {
> + this.autoCreateDisks.addAll(autoCreateDisks);
What if the input parameter is `null`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33454186
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Yaron Rosenbaum <no...@github.com>.
@nacx I am still struggling with the tests.
Some of the mock tests fail which are not related to my changes.
The tests that I'm adding fail as well, but they don't provide enough information for me to go on.
Trying to follow the code and reverse engineer what http calls are made is very hard.
I'm afraid I'll need your help. Please take a look at GoogleComputeEngineServiceMockTest and see where I am.
Appreciate your help..
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-116739542
Re: [jclouds/jclouds] Add support for disks & ip forward during node
creation (#790)
Posted by Andrew Gaul <no...@github.com>.
Sorry code is incomplete without tests. Please reopen if you want to complete this.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-331724074
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @yaronr!
Please, have a look at the formatting thing. Apart from that, this PR allows to pass a list of disks to be attached, but the "auto create" option allows just to auto create one disk. Wouldn't it make sense to also allow to auto-create N disks, for consistency with the other option?
Also have a look at the [two test failures](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1863/org.apache.jclouds.provider$google-compute-engine/testReport/). Could you run the build locally with a `mvn clean install` and make sure all tests pass?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-116321745
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Yaron Rosenbaum <no...@github.com>.
@nacx Thanks.
FYI I have spent hours on it - this is not me being lazy. I just don't understand the tests and have a very hard time following the flow of the code. I do know that this test should be 99% what I've written and commented out, though.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-120289939
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Yaron Rosenbaum <no...@github.com>.
@danbroudy @nacx guys, can you PLEASE merge this? pretty please? :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-120266277
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
@yaronr Unfortunately the pull request is not complete and we can't merge it without tests.
I will try to have a look at it and give a hand with the test, but I can't promise I'll fix it as there are many things going on in the project and I'm not sure I'll have cycles available.
The easiest path to get this merged is that you spend some time finishing the work. A good approach would be to copy an existing test and modify it to exercise the new additions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-120270552
Re: [jclouds/jclouds] Add support for disks & ip forward during node
creation (#790)
Posted by Andrew Gaul <no...@github.com>.
@yaronr do we have a path forward on this pull request?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#issuecomment-331354974
Re: [jclouds/jclouds] Add support for disks & ip forward during node
creation (#790)
Posted by Andrew Gaul <no...@github.com>.
Closed #790.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790#event-1262728330
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + /**
> + * @see #getDisks()
> + * @see org.jclouds.googlecomputeengine.domain.templates.InstanceTemplate.AttachDisk
> + */
> + public GoogleComputeEngineTemplateOptions addDisk(AttachDisk disk) {
> + this.disks.add(disk);
> + return this;
> + }
> +
> + public void autoCreateDisk(final AutoCreateDiskOptions diskOptions) {
> + this.autoCreateDisks.add(diskOptions);
> + }
> +
> + public void autoCreateDisks(final List<AutoCreateDiskOptions> autoCreateDisks) {
> + this.autoCreateDisks.addAll(autoCreateDisks);
Also, rename it to `addAutoCreateDisks` or similar, to avoid the confusion that this method would just assign the passed disks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33454221
Re: [jclouds] Add support for disks & ip forward during node creation
(#790)
Posted by Yaron Rosenbaum <no...@github.com>.
> @@ -43,9 +50,13 @@ public void copyTo(TemplateOptions to) {
> super.copyTo(to);
> if (to instanceof GoogleComputeEngineTemplateOptions) {
> GoogleComputeEngineTemplateOptions eTo = GoogleComputeEngineTemplateOptions.class.cast(to);
> + eTo.networks(to.getNetworks()); // YR: this is not relevant for my PR, but I think not having it is a bug
Ok :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/790/files#r33456515