FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

CLOUDSTACK-8605: KVM: Config Drive and getVmIp support by kishankavala · Pull Request #577 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

CLOUDSTACK-8605: KVM: Config Drive and getVmIp support - #577

Closed
kishankavala wants to merge 0 commit into
apache:masterfrom
kishankavala:master
ConversationCommits (0)ChecksFiles changed
Closed

CLOUDSTACK-8605: KVM: Config Drive and getVmIp support#577
kishankavala wants to merge 0 commit into
apache:masterfrom
kishankavala:master

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

asfbot commented Jul 10, 2015

Copy link
Copy Markdown

cloudstack-pull-rats #33 ABORTED

asfbot commented Jul 10, 2015

Copy link
Copy Markdown

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

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

yadvr reviewed Jul 16, 2015
View reviewed changes
Comment thread packaging/centos63/cloud.spec Outdated

yadvr Jul 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

asfbot commented Jul 17, 2015

Copy link
Copy Markdown

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

asfbot commented Jul 17, 2015

Copy link
Copy Markdown

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

asfbot commented Jul 17, 2015

Copy link
Copy Markdown

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

kishankavala commented Jul 17, 2015

Copy link
Copy Markdown
Contributor Author

@wido @bhaisaab Made the suggested changes

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.

remibergsma commented Aug 14, 2015

Copy link
Copy Markdown
Contributor

@kishankavala Any update on this?

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
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?

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

asfbot commented Aug 18, 2015

Copy link
Copy Markdown

cloudstack-pull-rats #337 ABORTED

asfbot commented Aug 18, 2015

Copy link
Copy Markdown

cloudstack-pull-requests #1032 ABORTED

asfbot commented Aug 18, 2015

Copy link
Copy Markdown

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

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.

yadvr commented Aug 26, 2015

Copy link
Copy Markdown
Member

LGTM.

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.

asfbot commented Sep 8, 2015

Copy link
Copy Markdown

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

asfbot commented Sep 8, 2015

Copy link
Copy Markdown

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

remibergsma commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

@kishankavala Please remove the 4 merge commits, thanks.

asfbot commented Sep 17, 2015

Copy link
Copy Markdown

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

asfbot commented Sep 17, 2015

Copy link
Copy Markdown

cloudstack-pull-analysis #579 ABORTED

asfbot commented Sep 17, 2015

Copy link
Copy Markdown

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

remibergsma commented Sep 21, 2015

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL