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

forgot to include os library #5

Merged
merged 6 commits into from
Mar 13, 2019

Conversation

StephaneGerardVUB
Copy link
Contributor

No description provided.

@@ -76,7 +76,7 @@ resources:
flavor: { get_param: user_vm_flavor }
user_data: |
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

this is not ok
code should be kept in it's own files/scripts (so we can unittest or parse the), and incided here using the {get_file: some_file_name} directive
so move the script to a script, and use the get_file to include it (it also allows reuse in other templates)

Copy link
Contributor Author

@StephaneGerardVUB StephaneGerardVUB Mar 1, 2019

Choose a reason for hiding this comment

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

I agree with you on the idea to separate the code of the Python script from the heat template. In principle, when you create a stack with Horizon using a template, you can always include an environment file in a second field, and this how - I think - you have to pass on the Python script, but when you do so, you've got the following error message: "The value of get_file is mount_share.py inside the template. When launching a stack from this interface, the value must start with "http://" or "https://". Having googled around a bit, I have found that: "This is a limitation of Horizon - it can't access local files on your machine the way the CLI client can. The recommended way to launch stacks with nested templates using Horizon is to first upload them to Swift, and then reference by URL."

Copy link
Member

Choose a reason for hiding this comment

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

that's a pity. but we need to find other ways to achieve this. this is too error prone
(and the script better adds the export to fstab)

@alvarosimon what options do we have to get eg some of our own python code in all the VMs? cloudinit?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm.. it works for me.

@StephaneGerardVUB have you tried this? I did a very stupid example with EB and also with some github public script files and it seems that is working, something like:

          user_data:
              get_file: https://raw.githubusercontent.com/alvarosimon/vsc_user_docs/master/deploy.sh

The only issue is the size of the file, it should be a few kilobytes because the file is injected into the metadata (but we can increase this limit if we want).

So I think that even for the tiny script @stdweird is right and we should include those scripts into a different directory in this repo and use them directly from our heat templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing some tests but for a reason I don't understand, I can't find the export location path in the metadata anymore...

os.mkdir(mountpoint)
os.chown(mountpoint,0,0)
os.system("mount %s %s" % (sharepath,mountpoint))
user_data: { get_file: 'https://raw.githubusercontent.com/StephaneGerardVUB/openstack-templates/HeatTemplateWithShare/heat/examples/mount_share.py' }
Copy link
Member

Choose a reason for hiding this comment

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

don't reference your own fork, use the main repo (it will only work once this PR is merged)

Copy link
Member

Choose a reason for hiding this comment

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

also do not use *.py extension, cloud-init is a bit stupid and only executes *.sh scripts

Copy link
Member

@alvarosimon alvarosimon Mar 13, 2019

Choose a reason for hiding this comment

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

@StephaneGerardVUB I have tested this PR directly from our openstack dashboard and it works! the only thing that we should change as @stdweird said is your personal URL by the final URL, do not use your personal github repo.
Also move the script to a new scripts directory. This will help us to keep yaml files and scripts in different places, it is cleaner and we can reuse these scripts quite easy.

Copy link
Member

Choose a reason for hiding this comment

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

and I was mistaken *.py also works :-) cloud-init is quite agnostic about that

os.mkdir(mountpoint)
os.chown(mountpoint,0,0)
os.system("mount %s %s" % (sharepath,mountpoint))
user_data: { get_file: 'https://raw.githubusercontent.com/hpcugent/openstack-templates/HeatTemplateWithShare/heat/examples/mount_share.py' }
Copy link
Member

Choose a reason for hiding this comment

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

This URL is still not correct, HeatTemplateWithShare branch is not available from upstream, it should be changed by the master branch. Also move the script to a different directory like heat/scripts//mount_share.py.
So the URL should look like this:

user_data: { get_file: 'https://raw.githubusercontent.com/hpcugent/openstack-templates/master/heat/scripts/mount_share.py' }

If you move the scripts into ../heat/scripts folder.

@@ -0,0 +1,11 @@
#!/usr/bin/env python
import os,json,re,requests
Copy link
Member

Choose a reason for hiding this comment

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

could you please import all the packages in different lines to follow PEP style guide?
https://www.python.org/dev/peps/pep-0008/#imports

@alvarosimon alvarosimon merged commit 9a347cf into hpcugent:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants