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

Follow upstream changes of libvirt-xml-go #376

Merged
merged 2 commits into from
Aug 8, 2017

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Aug 4, 2017

Virtlet uses a patch to adjust libvirt-xml-go domain definition. But due to changes in libvirt-xml-go upstream it's no longer possible to apply the patch. With this commit we update the patch so that it now follows the upstream. Also, some changes were conflicting therefore we needed to modify the Virtlet code that relies on those changes.

Namely, virtlet's patch suggested following struct for providing bootcmd:

DomainCMDLine
|-> Args: []QemuArg
|-> Envs: []QemuEnv

but the upstream code now provides following struct:

DomainQEMUCommandline
|-> Args: []DomainQEMUCommandlineArg
|-> Envs: []DomainQEMUCommandlineEnv

So we had to enforce such naming also in Virtlet code.

Fixes #375


This change is Reviewable

Virtlet uses a patch to adjust libvirt-xml-go domain definition.
But due to changes in libvirt-xml-go upstream it's no longer possible
to apply the patch. With this commit we update the patch so that it
now follows the upstream. Also, some changes were conflicting therefore
we needed to modify the Virtlet code that relies on those changes.

Namely, virtlet's patch suggested following struct for providing bootcmd:
```
DomainCMDLine
|-> Args: []QemuArg
|-> Envs: []QemuEnv
```

but the upstream code now provides following struct:
```
DomainQEMUCommandline
|-> Args: []DomainQEMUCommandlineArg
|-> Envs: []DomainQEMUCommandlineEnv
```

So we had to enforce such naming also in Virtlet code.

Fixes Mirantis#375
Signed-off-by: Miha Pleško <[email protected]>
@miha-plesko
Copy link
Contributor Author

I'm wondering how it's possible that this issue has occurred even if we're using glide. I mean, with Glide we have libvirt-go-xml locked at commit cc4025ea01eee7c73a7c75b4211b07125a8119ae where the patch should be possible to apply.

Anyway, I've prepared a PR where I both update the glide.yaml to use current latest commit and update the patch to fit on it. I'm looking forward to be hearing your opinion and possible explanation*!

* Note that the error occured on a clean development machine with all Docker images and containers purged and with latest Virtlet master checked out.

@ivan4th
Copy link
Contributor

ivan4th commented Aug 6, 2017

Hi, thanks for the PR. Concerning the explanation -- I still don't understand it myself, need to take a closer look. Concerning CI failure: we now have golden master tests, you need to run tests locally (in this case in pkg/libvirttools) and then include changed json files like pkg/libvirttools/TestContainerLifecycle.json in your commit.

Since we've synced the code with the latest commit on
libvirt-go-xml, all the progress from there must be reflected
in our golden tests.

Signed-off-by: Miha Pleško <[email protected]>
@miha-plesko
Copy link
Contributor Author

I've added a new commit with golden master tests updated which turned Travis green. But I'm not sure why "circleCI" is failing, the error seems to be unrelated to the changes.


Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented Aug 7, 2017

@miha-plesko that's some flake caused by (perhaps) CirrOS server glitch, restarting the build

@miha-plesko
Copy link
Contributor Author

May I ask, out of curiosity, what is the rationale to use both, TravisCI and CircleCI? I'm not familiar with CircleCI, but a quick googling shows me the post that claims:

Travis CI and CircleCI are almost the same.

https://hackernoon.com/continuous-integration-circleci-vs-travis-ci-vs-jenkins-41a1c2bd95f5

Just being curious here, not trying to persuade you anything.


Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@jellonek
Copy link
Contributor

jellonek commented Aug 7, 2017

The plan is to completely switch to CircleCI, because of Travis flackiness, especially on network bound operations.

@ivan4th
Copy link
Contributor

ivan4th commented Aug 7, 2017

@miha-plesko besides being generally less flaky for us, CircleCI provides secure ssh access to build VMs which is quite important at times. In case of Travis it's also possible because we requested debug mode from Travis, but it's insecure (anyone can connect to the VM after looking at the build log) and inconvenient (need to use curl to trigger it). We're going to revamp our image builds at some point so that images are built on our CI too -- docker cloud has it's own share of problems and we would like to stop using it, in which case we'll need to use some secrets, which makes Travis debug builds a non-option for us (secrets can be leaked).

@ivan4th
Copy link
Contributor

ivan4th commented Aug 7, 2017

@miha-plesko another thing we want to use in CircleCI is Workflows

@jellonek
Copy link
Contributor

jellonek commented Aug 8, 2017

:lgtm:


Reviewed 4 of 4 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented Aug 8, 2017

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@ivan4th ivan4th merged commit 8823244 into Mirantis:master Aug 8, 2017
@ivan4th
Copy link
Contributor

ivan4th commented Aug 8, 2017

@miha-plesko thanks for the PR, merged!

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