GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — CLOUDSTACK-8605: KVM: Config Drive and getVmIp support by kishankavala · Pull Request #577 · apache/cloudstack

Conversation

kishankavala commented Jul 10, 2015

Copy link

Copy Markdown

Contributor

CLOUDSTACK-8324 added support for External DHCP/DNS support along with ConfigDrive for XenServer
FS link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=53740797
This PR adds KVM support for config drive and getVmIpAddress assigned by external DHCP
genisoimage package dependency is added for creating config drive ISO
libguestfs is used to get IPAddress from guest VM. Linux: From dhcp leases file. Windows: From registry

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asfbot commented Jul 10, 2015

Copy link

Copy Markdown

cloudstack-pull-rats #33 ABORTED

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Jul 10, 2015

Copy link

Copy Markdown

cloudstack-pull-requests #728 UNSTABLE
Looks like there's a problem with this pull request

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishankavala commented Jul 15, 2015

Copy link

Copy Markdown

Contributor Author

/var/lib/libvirt/images is used for local storage also.
com.cloud.hypervisor.kvm.resource.LibvirtComputingResource#configure
_localStoragePath = (String)params.get("local.storage.path");
if (_localStoragePath == null) {
_localStoragePath = "/var/lib/libvirt/images/";
}

I'll update the patch to use _localStoragePath instead of hard-coded /var/lib/libvirt/images

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yadvr reviewed

Jul 16, 2015

View reviewed changes

Comment thread

packaging/centos63/cloud.spec Outdated

yadvr Jul 16, 2015

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

you'll need to add this to other cloud.spec files in packages/{centos7,fedora20,fedora21}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asfbot commented Jul 17, 2015

Copy link

Copy Markdown

cloudstack-pull-rats #73 SUCCESS
This pull request looks good

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Jul 17, 2015

Copy link

Copy Markdown

cloudstack-pull-requests #771 UNSTABLE
Looks like there's a problem with this pull request

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Jul 17, 2015

Copy link

Copy Markdown

cloudstack-pull-analysis #6 UNSTABLE
Looks like there's a problem with this pull request

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishankavala commented Jul 17, 2015

Copy link

Copy Markdown

Contributor Author

@wido @bhaisaab Made the suggested changes

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wido commented Jul 17, 2015

Copy link

Copy Markdown

Contributor

Code-wise I'm not to happy. There are all kinds of assumptions about paths. mkisofs for example always being there in /usr/bin.

Using /tmp for temporary directories, who says that /tmp is big enough on every system? Always using /var/lib/libvirt to place the ISO? Why not fetch the local storage pool and figure out what the path is.

It's not guaranteerd that it will be /var/lib/libvirt/images on all systems.

Imho there are to many assumptions in the code which makes it fragile.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remibergsma commented Aug 14, 2015

Copy link

Copy Markdown

Contributor

@kishankavala Any update on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remibergsma added a commit to remibergsma/cloudstack that referenced this pull request

Aug 17, 2015


          Closing PRs older than a month without activity

8ade5dc

This closes apache#577
This closes apache#566
This closes apache#562
This closes apache#561
This closes apache#556
This closes apache#555
This closes apache#554
This closes apache#548
This closes apache#544
This closes apache#540
This closes apache#508
This closes apache#384
This closes apache#372

remibergsma mentioned this pull request

Aug 17, 2015

Closing PRs older than 1 month and without activity #706

Closed

remibergsma added a commit to remibergsma/cloudstack that referenced this pull request

Aug 17, 2015


          Closing PRs older than a month without activity

aa193a5

This closes apache#577
This closes apache#566
This closes apache#562
This closes apache#561
This closes apache#556
This closes apache#555
This closes apache#554
This closes apache#548
This closes apache#544
This closes apache#540
This closes apache#384
This closes apache#372

remibergsma added a commit to remibergsma/cloudstack that referenced this pull request

Aug 17, 2015


          Closing PRs older than a month without activity

7d28b3d

This closes apache#577
This closes apache#566
This closes apache#562
This closes apache#561
This closes apache#556
This closes apache#555
This closes apache#554
This closes apache#548
This closes apache#544
This closes apache#540
This closes apache#384
This closes apache#372

kishankavala commented Aug 18, 2015

Copy link

Copy Markdown

Contributor Author

Updated code to use local.storage.path config instead of hard-coded /var/lib/libvirt/images/. Local Storage pool is also created using same config. Removed /usr/bin path for mkisofs.
Other issue was regarding using /tmp for temporary directories. Should I change this also to use local storage path?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishankavala commented Aug 18, 2015

Copy link

Copy Markdown

Contributor Author

@remibergsma I've update the PR. There is still one issue open regarding the usage of /tmp

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Aug 18, 2015

Copy link

Copy Markdown

cloudstack-pull-rats #337 ABORTED

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Aug 18, 2015

Copy link

Copy Markdown

cloudstack-pull-requests #1032 ABORTED

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Aug 18, 2015

Copy link

Copy Markdown

cloudstack-pull-analysis #269 UNSTABLE
Looks like there's a problem with this pull request

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remibergsma commented Aug 19, 2015

Copy link

Copy Markdown

Contributor

@kishankavala Thanks for the update! When you're ready, also be sure to ping @wido so he can have another look.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yadvr commented Aug 26, 2015

Copy link

Copy Markdown

Member

LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yadvr commented Aug 26, 2015

Copy link

Copy Markdown

Member

maybe rebase and fix any issues, to get Travis green before merging this once @wido can review this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Sep 8, 2015

Copy link

Copy Markdown

cloudstack-pull-rats #540 SUCCESS
This pull request looks good

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Sep 8, 2015

Copy link

Copy Markdown

cloudstack-pull-analysis #474 SUCCESS
This pull request looks good

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remibergsma commented Sep 10, 2015

Copy link

Copy Markdown

Contributor

@kishankavala Please remove the 4 merge commits, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Sep 17, 2015

Copy link

Copy Markdown

cloudstack-pull-rats #640 SUCCESS
This pull request looks good

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Sep 17, 2015

Copy link

Copy Markdown

cloudstack-pull-analysis #579 ABORTED

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asfbot commented Sep 17, 2015

Copy link

Copy Markdown

cloudstack-pull-analysis #585 SUCCESS
This pull request looks good

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remibergsma commented Sep 21, 2015

Copy link

Copy Markdown

Contributor

@kishankavala There are 5 merge commits now... please remove them.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishankavala closed this

Dec 15, 2015

kishankavala force-pushed the master branch from 8cb5f90 to 94a1448 Compare

December 15, 2015 06:59

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.