Skip to content
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

initial work on sdc-cloudapi for PUBAPI-1420 #28

Closed
wants to merge 3 commits into from
Closed

initial work on sdc-cloudapi for PUBAPI-1420 #28

wants to merge 3 commits into from

Conversation

joshwilsdon
Copy link
Contributor

@joshwilsdon joshwilsdon commented Aug 30, 2017 •
edited

initial work

misterdjules
misterdjules approved these changes Sep 13, 2017
View reviewed changes
Copy link
Contributor

@misterdjules misterdjules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my comments are minor or nits, and I wouldn't mind merging this as is, as long as tickets for future work are filed.

lib/machines.js Outdated

function validVolumeName(volumeName) {
var VALID_VOLUME_NAME_REGEXP = /^[a-zA-Z0-9][a-zA-Z0-9_\.\-]+$/;
if (volumeName === undefined || volumeName === null ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: should we check that the value volumeName is of type 'string' instead with typeof (volumeName) !== 'string'? I'm not sure it makes a big difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this makes sense.

lib/machines.js Outdated

// stops at first error
function validateVolume(volume) {
var VALID_VOLUME_MODES = ['ro', 'rw'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert.object(volume, 'volume');?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep!

lib/machines.js Outdated
var VALID_VOLUME_MODES = ['ro', 'rw'];

if (volume.type !== undefined && volume.type !== 'tritonnfs') {
return (new InvalidArgumentError('unknown volume type "' + volume.type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return on the first error, then we lose the opportunity to report all relevant errors for a given request. Should we instead return an array of errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll change to do that.

lib/machines.js Outdated
for (idx = 0; idx < volumes.length; idx++) {
validationErr = validateVolume(volumes[idx]);
if (validationErr) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: we early out on the first validation error, should we instead validate all input and report all relevant errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

test/volumes-automount.test.js Outdated
var CONFIG = mod_config.configure();
var IMGAPI_SOURCE = 'https://images.joyent.com';
var KEY_FILENAME = '/tmp/cloudapi-test-key';
var TEST_IMAGE_LX = '7b5981c4-1889-11e7-b4c5-3f3bdfc9b88b'; // ubuntu-16.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to hardcode it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so for now, but I'll create a ticket to revisit this in the future.

test/volumes-automount.test.js Outdated
var KEY_FILENAME = '/tmp/cloudapi-test-key';
var TEST_IMAGE_LX = '7b5981c4-1889-11e7-b4c5-3f3bdfc9b88b'; // ubuntu-16.04
var TEST_IMAGE_SMARTOS =
'ede31770-e19c-11e5-bb6e-3b7de3cca9ce'; // minimal-multiarch-lts (15.4.1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same regarding hardcoding of the image UUID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created https://devhub.joyent.com/jira/browse/PUBAPI-1436 for this.

test/volumes-automount.test.js Outdated
'ede31770-e19c-11e5-bb6e-3b7de3cca9ce'; // minimal-multiarch-lts (15.4.1)
var UFDS_ADMIN_UUID = CONFIG.ufds_admin_uuid;

// XXX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this // XXX mean? I'd suggest replacing that with a clearer comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually left in by mistake. I'll remove it.

test/volumes-automount.test.js Outdated
});
});

// need a package we can use to provision our containers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that comment be about networks instead of packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it should!

test/volumes-automount.test.js
name: 'cloudapi-volume-lx-' + libuuid.create().split('-')[0],
firewall_enabled: false,
networks: [
{ipv4_uuid: networkUuidSsh, primary: true},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

lib/machines.js
if (req.params && req.params.volumes && req.params.volumes.length > 0) {
if (opts.brand === 'kvm') {
// At this time we do not support NFS volumes with KVM
return next(new InvalidArgumentError('volumes not yet supported ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test that checks that the relevant error is returned when trying to provision a KVM VM with volumes? That can be done in a subsequent PR, but we'd need to track that with a separate ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a test that does this. Sure.

@misterdjules
Copy link
Contributor

@joshwilsdon Latest updates look good to me!

@joshwilsdon
Copy link
Contributor Author

The "experimental_cloudapi_automount" flag stuff was pointed out to me by @misterdjules in 1:1 chat. At this point I'm considering this complete and just waiting on a final go-ahead.

@joshwilsdon
Copy link
Contributor Author

integrated via c70cbc5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misterdjules misterdjules misterdjules approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@joshwilsdon @misterdjules