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.

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

OpenStack support #114

Merged
merged 87 commits into from
Jun 1, 2018
Merged

OpenStack support #114

merged 87 commits into from
Jun 1, 2018

Conversation

JHaydock-Pro
Copy link
Contributor

@JHaydock-Pro JHaydock-Pro commented Feb 12, 2018

This pull request intends to finalize OpenStack support for the cloud interface.

This pull also resolves #117, resolves #116 and resolves #73

MRichards99 and others added 26 commits February 5, 2018 17:09
Copy link
Member

@tofu-rocketry tofu-rocketry left a comment

Choose a reason for hiding this comment

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

See comments.

Indents need standardising to spaces (some use tabs).

README.md Outdated
@@ -7,6 +7,9 @@ python-cherrypy | 3.2.2
python-ldap | 2.3.10
python-jinja2 | 2.2.1
python-websockify | 0.5.1
python-keystoneauth |
Copy link
Member

Choose a reason for hiding this comment

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

Any particular version for these?

README.md Outdated
@@ -48,3 +51,5 @@ And then navigate to:
When running the webfrontend without Apache, you might have to manually launch websockify.

`/usr/bin/websockify -v PORT --cert=CERT --key=KEY --target-config=TOKENDIR`

*NOTE: You may also need to create a `sessions/` directory*
Copy link
Member

Choose a reason for hiding this comment

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

Where? Why?

loadingCount++;
console.log(loadingCount);
incrementLoadingCount();
// console.log("loadingCount: " + loadingCount)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed if it's not being used?

$('#memory-input').val(flavorList['data'][i]['ram']);
$('#disk-output').val(flavorList['data'][i]['disk']);
$('#disk-input').val(flavorList['data'][i]['disk']);
// $('#cpu-output').val(flavorList['data'][i]['cpu']);
Copy link
Member

Choose a reason for hiding this comment

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

Commented out lines not removed?

loadingCount++;
console.log(loadingCount);
incrementLoadingCount();
// console.log("loadingCount: " + loadingCount)
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

loadingCount++;
console.log(loadingCount);
incrementLoadingCount();
// console.log("loadingCount: " + loadingCount)
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

@@ -15,6 +15,10 @@ def GET(self):
username = cherrypy.request.cookie.get('fedid').value
auth_string = cherrypy.request.cookie.get('session').value

# osDistro = ""
Copy link
Member

Choose a reason for hiding this comment

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

What's point of these comments?

min_count = json['count']
min_count = json['count'],
aq_archetype = json['archetype'],
aq_personality = json['personality'],
Copy link
Member

Choose a reason for hiding this comment

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

Mixed tabs and spaces!!!!! 👿

Copy link
Member

Choose a reason for hiding this comment

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

Arg! There's more of them further on!

@@ -29,7 +29,8 @@ def PUT(self):
try:
keyname = novaClient.keypairs.list()[0].name
except IndexError:
raise cherrypy.HTTPError('400 You haven\'t got a keypair, you must have a keypair to create a VM.')
# raise cherrypy.HTTPError('400 You haven\'t got a keypair, you must have a keypair to create a VM.')
Copy link
Member

Choose a reason for hiding this comment

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

If this is commented out (not removed?) what stops bad things happening?

<span id="SearchBlock" style="float:right; height:auto;">
<!--
Search:
Ideally the search box should go here but i can't seem find it anywhere in this repo
Copy link
Member

Choose a reason for hiding this comment

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

Was there a search box before?

Copy link
Member

Choose a reason for hiding this comment

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

So, what's happening here?

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 believe the search bar element is created by jquery in one of the min.js files (this one i think). This is near impossible to read, even after de-minifying it.

The element also does not have any IDs or unique classes which makes it very difficult to modify with JQuery and CSS.

The search bar is functional but is placed in a different location and under different parents to the other elements next to it (ie.the filter button and projects drop down). I'm not sure why this is and it is awkward for CSS.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds like you should record that as an issue.

}

// Limit numbers of VMs created at once in unlimited quotas
if (data['count'] > count_limit && groupquotavm === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be checked whether your group has an unlimited quota or not? You want to stop people from flooding the system accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number of VMs is still limited by quota. I asked Alex about it, he was fine with people filling up their quotas in one go.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

if quotaData['groupquotacpu'] > -1:
quotaData['availablequotacpu'] = quotaData['groupquotacpu'] - quotaData['groupusedcpu']
else:
quotaDataKeys.append('biggestCPUAmount')

if quotaData['groupquotavm'] > -1:
quotaData['availablequotavm'] = quotaData['groupquotavm'] - quotaData['groupusedvm']

# Always appended as there's no disk quota - biggest disk size will always be required
Copy link
Member

Choose a reason for hiding this comment

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

This comment has drifted away from the line it applies to.


for flavor in novaClient.flavors.list(detailed = True):
flavorInfo[flavor.name] = [flavor.vcpus, flavor.ram]
flavorList[flavor.id] = {'name':str(flavor.name), 'vcpus':flavor.vcpus, 'ram':flavor.ram}
print flavorList
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a sprinkling of print statements in this file rather than proper logging?

cherrypy.log(server.name + ' - NotFound Error for name matching FlavorID: ' + server.flavor[u'id'])
flavorName = flavorList[server.flavor['id']]['name']
except Exception as ex:
cherrypy.log('- ' + str(type(ex)) + ' when getting flavorName for VM: ' + str(server.name), username)
Copy link
Member

Choose a reason for hiding this comment

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

When logging, string substitution should be used rather than concatenation and should be done the way that is recommended for the Python logging module.

from getFunctions import getNovaInstance

class VNC(object):
'''
Copy link
Member

Choose a reason for hiding this comment

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

Triple double quotes should be used around doctrings rather than single.

'id': 'pakiti',
'q': 'Why are Pakiti and rsyslog running on my VM?',
'a': 'These services are used for monitoring and security.
Please refrain from stopping or changing the behaviour of either of these service.
Copy link
Member

Choose a reason for hiding this comment

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

Should probably use a phrase that is stronger than just "refrain".

Maybe: "You must not stop or change the behaviour of..."

<span id="projectChoiceBlock" class="pull-right">
Current Project:
<select style="height:34px" id="projectChoice" class="btn btn-default btn-xs" onchange="makeAjaxCalls()"></select>
<!-- <div id="loading-project" class="loader"></div> -->
Copy link
Member

Choose a reason for hiding this comment

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

Why's this commented out?

<button type="button" class="btn btn-default" id="filter-button" title="Table Filters" onclick="filterTableDialog()"><span class="glyphicon glyphicon-cog"></span></button>
<div style="height: 34px">
<button id="newMachine" type="button" class="btn btn-green" onclick="createVMdialog()" title="Create New Machine"
{% if cloudPlatform == "openstack" %}disabled=""{% endif %}>
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be indented and the closing angle bracket dropped onto a newline to make it clear this is all one HTML tag.

<span>

<span id="loading-quota" style="display:none;">
&nbsp; Loading Quota: <div class="loader"></div>
Copy link
Member

Choose a reason for hiding this comment

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

The contents of all these span tags should be indented.

<div class="modal-body" style="text-align:center; margin-bottom:18px;">

<div class="modal-body">
<form id="renamevm" class="form-horizontal" role="form" action"/">
Copy link
Member

Choose a reason for hiding this comment

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

Can you sort the indentation out please?

# Aquilon
meta = {}

if (json['archetype'] != None) and (json['archetype'] != ''):

Choose a reason for hiding this comment

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

According to PEP8 (the style guide for Python code)

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

meta['AQ_ARCHETYPE'] = json['archetype']
meta['AQ_PERSONALITY'] = json['personality']

if (json['sandbox'] != None) and (json['sandbox'] != ''):

Choose a reason for hiding this comment

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

as above


for server in novaClient.servers.list(detailed = True):
print server.name + " - " + server.status
cherrypy.log('- ' + server.name + ' - ' + server.status, username)
Copy link

@gregcorbett gregcorbett May 31, 2018

Choose a reason for hiding this comment

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

is there a mix of tabs and spaces around this line? The indentation looks 1 space to many to me in a side by side comparison but not when viewing the file as a whole.

@@ -158,6 +208,7 @@ def GET(self, action):
'type' : imageName,
'candelete': True,
'keypair' : server.key_name,
'aquilon' : aquilon,

Choose a reason for hiding this comment

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

again, this indentation looks 'wonky' in a side by side comparison view but not when viewing the file as a whole...

@gregcorbett
Copy link

I echo Adrian's comments about logging and string substitution.

@jrha jrha merged commit 295a8dd into stfc:master Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants