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

Ports Cyris to Ubuntu 20.04 and Python 3, fixes several/potential bugs #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tdgunes
Copy link

@tdgunes tdgunes commented Jul 10, 2020

As title says, I would like to share the changes made to make Cyris work with Ubuntu 20.04 with Python 3 (tested with Python 3.8).

  • Iptables related changes: Opens both given TCP and UDP ports, main/entities.py:551
  • Waiting for host shutting down: The fix for a bug in the logic, main/cyris.py:1391.
  • Making Cyris work under new version of QEMU in instantiation/vm_clone/vm_clone_xml.sh:31
  • PEP8 compliance and encoding related changes
  • I also confirm that this version worked for me in a nested virtualization setting (i.e. Ubuntu 20.04 Host -> Ubuntu 20.40 Cyris Guest -> VMs created by Cyris).
  • TODO: I had issues with some multiple host related commands, I disabled one in main/cyris.py:1457.

Hopefully, this will be useful for somebody! :)

@crond-jaist
Copy link
Collaborator

Dear Taha Doğan Güneş,

Thank you for your contribution, we really appreciate your wish to help. I have checked your pull request, and unfortunately we cannot accept it as it is. The main reasons are the following:

  1. The changes you made to support Ubuntu 20.04 in HOST-PREPARE.sh break compatibility with older OSes which we would still like to support, such as Ubuntu 18.04 and even 16.04. One idea could be to prepare a new script just for Ubuntu 20.04, and commit it in addition to the current HOST-PREPARE.sh.
  2. Your commits are somewhat difficult to review because several changes are not related to the purpose of the commit, as they seem to be simple reformatting of the code, done perhaps by your IDE. I suggest you disable such automatic reformatting in the future to minimize the differences between your commit and the original code.

The contribution that would be most welcome is related to adding compatibility with Python 3, as I believe these changes do not introduce any incompatibility with Python 2, which we still use on several servers. If you are willing to put some more effort, I would suggest you make another commit that contains only the Python 2 to 3 compatibility modifications and resubmit.

I understand that your main goal is to make the code running for you, but as main developers and maintainers we have to consider compatibility issues very carefully, and we also must review the code carefully for security reasons. We are looking forward to any future contributions that you have the time to make. As a side comment, we'd also be interested in knowing more about how you are using our system, as your feedback could be useful for future development.

Best wishes,
Razvan

@tdgunes
Copy link
Author

tdgunes commented Jul 13, 2020

Hi Razvan,

You are welcome and thanks for checking the pull request. Regarding the issues that you mentioned:

  1. Having separate bash script HOST-PREPARE.sh for each version of Ubuntu is a good idea. However, the changes in instantiation/vm_clone/vm_clone_xml.sh needs to be reverted when an older version of Ubuntu is used.
  2. Regarding commits being difficult to review, the original code was not PEP8 compliant. This made porting to Python 3 harder, when I used 2to3. Therefore, I run autopep8 which fixed issues that are described here before porting semi-manually with 2to3. I think if you use autopep8 to fix original codebase and get a diff between this pull request, it should make reviewing the changes easier. The main changes are the ones mentioned in my first comment.
  3. Regarding Python 2 compatibility, I think this requires looking into using futures module to make Cyris work in both versions of Python.

About making additional changes to this PR, unfortunately I am very occupied nowadays. But if the time allows, I may be able to share some feedback, and hopefully may be able to get back to this PR.

Thanks again for Cyris, it has been quite useful!

Best,
Taha.

@crond-jaist
Copy link
Collaborator

Hi Taha,

I understand about issue 1, it seems to be more complicated than I thought. As for 2 and 3, we'll take into account your advice when we'll consider this aspect. Sharing some feedback when you have time is perfectly fine. Good luck with your research!

Best wishes,
Razvan

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.

2 participants