You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Edison Su <Ed...@citrix.com> on 2014/07/11 23:48:37 UTC
[DISCUSS]CLOUDSTACK-6191
Move the discussion to the list about CloudStack-6191:
Automation test is blocked by this bug, we need to find a solution. My suggestion is sort-of-revert-the-patch, but still give admin the opportunity to specify the way to optimize KVM volume creation. The reasons:
1. End user shouldn't care about how the volume is created, is it sparse,flat/thin-provisoned or whatever technology used by hypervisor. So we'd better not expose this option in disk offering.
2. It's true that admin does care about how the volume is created, so we can add a global configuration just for the kvm volume creation. For vmware, the option is already there(vmware.create.full.clone to control whether link clone or full clone is used to create volume). We can add an option, something like kvm.qcow2.volume.create.options.
Comments?
RE: [DISCUSS]CLOUDSTACK-6191
Posted by Santhosh Edukulla <sa...@citrix.com>.
Its not a false positive though, the complain is about possible null de-reference, the calling method declaration and definition provides that scope, i now made a change to accommodate null checks for vnetid and protocol values, only inside few cases where it is locally applicable, submitted a patch.
I believe all other changes are still available in branch, only for this file, it was reverted earlier.
Santhosh
________________________________________
From: Daan Hoogland [daan.hoogland@gmail.com]
Sent: Thursday, July 17, 2014 3:33 AM
To: Edison Su
Cc: dev; Santhosh Edukulla
Subject: Re: [DISCUSS]CLOUDSTACK-6191
Seems like it could have been fixed with a smaller code replace but I
guess you are right.
@Santosh, Can you split your commit and reapply. At least chipping off
the BridgeVifDriver bit, but preferably all chopped up as other issues
will probably come up.
thanks,
On Wed, Jul 16, 2014 at 6:57 PM, Edison Su <Ed...@citrix.com> wrote:
> The commit: a600d8408ea86782318139c17cf346c8497943d0, only fixes the "issues" reported by coverity. Sometimes, coverity may report false alarm, that's what happened in the code I reverted.
> If we want to make coverity happy, we'd better refactor the code in BridgeVifDriver->Plug
>
>> -----Original Message-----
>> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
>> Sent: Tuesday, July 15, 2014 4:43 AM
>> To: dev; Edison Su; Santhosh Edukulla
>> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>>
>> Edison,
>>
>> You reverted a600d8408ea86782318139c17cf346c8497943d0 in the end. The
>> code in there is solving a lot of resource problems. Did you pinpoint the exact
>> problem? I can not imagine that there is not a side effect that Santhosh didn't
>> know about and is undesirable that is really the really the root case. We
>> should find that and not just revert because an existing bug was uncovered.
>>
>>
>> On Mon, Jul 14, 2014 at 11:18 PM, Edison Su <Ed...@citrix.com> wrote:
>> > Yoshikazu fixed the issue related to qemu-img which introduced by his
>> patch in cloudstack-6191.
>> > But there is another issue introduced by commit:
>> a600d8408ea86782318139c17cf346c8497943d0, which causes starting vm
>> failure.
>> > So I checked in a commit: e1095b0110f08fb0c7965f9cf293a6073bbce511, to
>> fix CLOUDSTACK-7051.
>> > So I think, both CLOUDSTACK-6191 and CLOUDSTACK-7051 should be fixed
>> now, no need to revert or change CLOUDSTACK-6191.
>> >
>> >> -----Original Message-----
>> >> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
>> >> Sent: Saturday, July 12, 2014 11:02 AM
>> >> To: dev
>> >> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>> >>
>> >> -0 What does it fix and is the solution bonifide. We should fix the
>> >> test suite if it is. The test suite not working is not enough reason
>> >> to revert a commit, it should block the test-suite because the system
>> >> is broken, not because of the way the test suite works.
>> >>
>> >> Disclaimer: I do not know enough of KVM to make a judgement call. I
>> >> just got triggered by the motivation in the mail thread.
>> >>
>> >> On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
>> >> <ra...@citrix.com> wrote:
>> >> > +1 Revert now and enable after complete full test in KVM
>> >> >
>> >> > KVM automation blocked more than 7 days due to this defect
>> >> >
>> >> > https://issues.apache.org/jira/browse/CLOUDSTACK-7051
>> >> >
>> >> > Regards,
>> >> > Rayees
>> >> >
>> >> > -----Original Message-----
>> >> > From: Edison Su [mailto:Edison.su@citrix.com]
>> >> > Sent: Friday, July 11, 2014 2:49 PM
>> >> > To: dev@cloudstack.apache.org
>> >> > Subject: [DISCUSS]CLOUDSTACK-6191
>> >> >
>> >> > Move the discussion to the list about CloudStack-6191:
>> >> > Automation test is blocked by this bug, we need to find a solution.
>> >> > My
>> >> suggestion is sort-of-revert-the-patch, but still give admin the
>> >> opportunity to specify the way to optimize KVM volume creation. The
>> reasons:
>> >> >
>> >> > 1. End user shouldn't care about how the volume is created, is it
>> >> sparse,flat/thin-provisoned or whatever technology used by
>> >> hypervisor. So we'd better not expose this option in disk offering.
>> >> >
>> >> > 2. It's true that admin does care about how the volume is created, so
>> >> we can add a global configuration just for the kvm volume creation.
>> >> For vmware, the option is already there(vmware.create.full.clone to
>> >> control whether link clone or full clone is used to create volume).
>> >> We can add an option, something like kvm.qcow2.volume.create.options.
>> >> > Comments?
>> >>
>> >>
>> >>
>> >> --
>> >> Daan
>>
>>
>>
>> --
>> Daan
--
Daan
Re: [DISCUSS]CLOUDSTACK-6191
Posted by Daan Hoogland <da...@gmail.com>.
Seems like it could have been fixed with a smaller code replace but I
guess you are right.
@Santosh, Can you split your commit and reapply. At least chipping off
the BridgeVifDriver bit, but preferably all chopped up as other issues
will probably come up.
thanks,
On Wed, Jul 16, 2014 at 6:57 PM, Edison Su <Ed...@citrix.com> wrote:
> The commit: a600d8408ea86782318139c17cf346c8497943d0, only fixes the "issues" reported by coverity. Sometimes, coverity may report false alarm, that's what happened in the code I reverted.
> If we want to make coverity happy, we'd better refactor the code in BridgeVifDriver->Plug
>
>> -----Original Message-----
>> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
>> Sent: Tuesday, July 15, 2014 4:43 AM
>> To: dev; Edison Su; Santhosh Edukulla
>> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>>
>> Edison,
>>
>> You reverted a600d8408ea86782318139c17cf346c8497943d0 in the end. The
>> code in there is solving a lot of resource problems. Did you pinpoint the exact
>> problem? I can not imagine that there is not a side effect that Santhosh didn't
>> know about and is undesirable that is really the really the root case. We
>> should find that and not just revert because an existing bug was uncovered.
>>
>>
>> On Mon, Jul 14, 2014 at 11:18 PM, Edison Su <Ed...@citrix.com> wrote:
>> > Yoshikazu fixed the issue related to qemu-img which introduced by his
>> patch in cloudstack-6191.
>> > But there is another issue introduced by commit:
>> a600d8408ea86782318139c17cf346c8497943d0, which causes starting vm
>> failure.
>> > So I checked in a commit: e1095b0110f08fb0c7965f9cf293a6073bbce511, to
>> fix CLOUDSTACK-7051.
>> > So I think, both CLOUDSTACK-6191 and CLOUDSTACK-7051 should be fixed
>> now, no need to revert or change CLOUDSTACK-6191.
>> >
>> >> -----Original Message-----
>> >> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
>> >> Sent: Saturday, July 12, 2014 11:02 AM
>> >> To: dev
>> >> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>> >>
>> >> -0 What does it fix and is the solution bonifide. We should fix the
>> >> test suite if it is. The test suite not working is not enough reason
>> >> to revert a commit, it should block the test-suite because the system
>> >> is broken, not because of the way the test suite works.
>> >>
>> >> Disclaimer: I do not know enough of KVM to make a judgement call. I
>> >> just got triggered by the motivation in the mail thread.
>> >>
>> >> On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
>> >> <ra...@citrix.com> wrote:
>> >> > +1 Revert now and enable after complete full test in KVM
>> >> >
>> >> > KVM automation blocked more than 7 days due to this defect
>> >> >
>> >> > https://issues.apache.org/jira/browse/CLOUDSTACK-7051
>> >> >
>> >> > Regards,
>> >> > Rayees
>> >> >
>> >> > -----Original Message-----
>> >> > From: Edison Su [mailto:Edison.su@citrix.com]
>> >> > Sent: Friday, July 11, 2014 2:49 PM
>> >> > To: dev@cloudstack.apache.org
>> >> > Subject: [DISCUSS]CLOUDSTACK-6191
>> >> >
>> >> > Move the discussion to the list about CloudStack-6191:
>> >> > Automation test is blocked by this bug, we need to find a solution.
>> >> > My
>> >> suggestion is sort-of-revert-the-patch, but still give admin the
>> >> opportunity to specify the way to optimize KVM volume creation. The
>> reasons:
>> >> >
>> >> > 1. End user shouldn't care about how the volume is created, is it
>> >> sparse,flat/thin-provisoned or whatever technology used by
>> >> hypervisor. So we'd better not expose this option in disk offering.
>> >> >
>> >> > 2. It's true that admin does care about how the volume is created, so
>> >> we can add a global configuration just for the kvm volume creation.
>> >> For vmware, the option is already there(vmware.create.full.clone to
>> >> control whether link clone or full clone is used to create volume).
>> >> We can add an option, something like kvm.qcow2.volume.create.options.
>> >> > Comments?
>> >>
>> >>
>> >>
>> >> --
>> >> Daan
>>
>>
>>
>> --
>> Daan
--
Daan
RE: [DISCUSS]CLOUDSTACK-6191
Posted by Edison Su <Ed...@citrix.com>.
The commit: a600d8408ea86782318139c17cf346c8497943d0, only fixes the "issues" reported by coverity. Sometimes, coverity may report false alarm, that's what happened in the code I reverted.
If we want to make coverity happy, we'd better refactor the code in BridgeVifDriver->Plug
> -----Original Message-----
> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> Sent: Tuesday, July 15, 2014 4:43 AM
> To: dev; Edison Su; Santhosh Edukulla
> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>
> Edison,
>
> You reverted a600d8408ea86782318139c17cf346c8497943d0 in the end. The
> code in there is solving a lot of resource problems. Did you pinpoint the exact
> problem? I can not imagine that there is not a side effect that Santhosh didn't
> know about and is undesirable that is really the really the root case. We
> should find that and not just revert because an existing bug was uncovered.
>
>
> On Mon, Jul 14, 2014 at 11:18 PM, Edison Su <Ed...@citrix.com> wrote:
> > Yoshikazu fixed the issue related to qemu-img which introduced by his
> patch in cloudstack-6191.
> > But there is another issue introduced by commit:
> a600d8408ea86782318139c17cf346c8497943d0, which causes starting vm
> failure.
> > So I checked in a commit: e1095b0110f08fb0c7965f9cf293a6073bbce511, to
> fix CLOUDSTACK-7051.
> > So I think, both CLOUDSTACK-6191 and CLOUDSTACK-7051 should be fixed
> now, no need to revert or change CLOUDSTACK-6191.
> >
> >> -----Original Message-----
> >> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> >> Sent: Saturday, July 12, 2014 11:02 AM
> >> To: dev
> >> Subject: Re: [DISCUSS]CLOUDSTACK-6191
> >>
> >> -0 What does it fix and is the solution bonifide. We should fix the
> >> test suite if it is. The test suite not working is not enough reason
> >> to revert a commit, it should block the test-suite because the system
> >> is broken, not because of the way the test suite works.
> >>
> >> Disclaimer: I do not know enough of KVM to make a judgement call. I
> >> just got triggered by the motivation in the mail thread.
> >>
> >> On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
> >> <ra...@citrix.com> wrote:
> >> > +1 Revert now and enable after complete full test in KVM
> >> >
> >> > KVM automation blocked more than 7 days due to this defect
> >> >
> >> > https://issues.apache.org/jira/browse/CLOUDSTACK-7051
> >> >
> >> > Regards,
> >> > Rayees
> >> >
> >> > -----Original Message-----
> >> > From: Edison Su [mailto:Edison.su@citrix.com]
> >> > Sent: Friday, July 11, 2014 2:49 PM
> >> > To: dev@cloudstack.apache.org
> >> > Subject: [DISCUSS]CLOUDSTACK-6191
> >> >
> >> > Move the discussion to the list about CloudStack-6191:
> >> > Automation test is blocked by this bug, we need to find a solution.
> >> > My
> >> suggestion is sort-of-revert-the-patch, but still give admin the
> >> opportunity to specify the way to optimize KVM volume creation. The
> reasons:
> >> >
> >> > 1. End user shouldn't care about how the volume is created, is it
> >> sparse,flat/thin-provisoned or whatever technology used by
> >> hypervisor. So we'd better not expose this option in disk offering.
> >> >
> >> > 2. It's true that admin does care about how the volume is created, so
> >> we can add a global configuration just for the kvm volume creation.
> >> For vmware, the option is already there(vmware.create.full.clone to
> >> control whether link clone or full clone is used to create volume).
> >> We can add an option, something like kvm.qcow2.volume.create.options.
> >> > Comments?
> >>
> >>
> >>
> >> --
> >> Daan
>
>
>
> --
> Daan
Re: [DISCUSS]CLOUDSTACK-6191
Posted by Daan Hoogland <da...@gmail.com>.
Edison,
You reverted a600d8408ea86782318139c17cf346c8497943d0 in the end. The
code in there is solving a lot of resource problems. Did you pinpoint
the exact problem? I can not imagine that there is not a side effect
that Santhosh didn't know about and is undesirable that is really the
really the root case. We should find that and not just revert because
an existing bug was uncovered.
On Mon, Jul 14, 2014 at 11:18 PM, Edison Su <Ed...@citrix.com> wrote:
> Yoshikazu fixed the issue related to qemu-img which introduced by his patch in cloudstack-6191.
> But there is another issue introduced by commit: a600d8408ea86782318139c17cf346c8497943d0, which causes starting vm failure.
> So I checked in a commit: e1095b0110f08fb0c7965f9cf293a6073bbce511, to fix CLOUDSTACK-7051.
> So I think, both CLOUDSTACK-6191 and CLOUDSTACK-7051 should be fixed now, no need to revert or change CLOUDSTACK-6191.
>
>> -----Original Message-----
>> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
>> Sent: Saturday, July 12, 2014 11:02 AM
>> To: dev
>> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>>
>> -0 What does it fix and is the solution bonifide. We should fix the test suite if
>> it is. The test suite not working is not enough reason to revert a commit, it
>> should block the test-suite because the system is broken, not because of the
>> way the test suite works.
>>
>> Disclaimer: I do not know enough of KVM to make a judgement call. I just got
>> triggered by the motivation in the mail thread.
>>
>> On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
>> <ra...@citrix.com> wrote:
>> > +1 Revert now and enable after complete full test in KVM
>> >
>> > KVM automation blocked more than 7 days due to this defect
>> >
>> > https://issues.apache.org/jira/browse/CLOUDSTACK-7051
>> >
>> > Regards,
>> > Rayees
>> >
>> > -----Original Message-----
>> > From: Edison Su [mailto:Edison.su@citrix.com]
>> > Sent: Friday, July 11, 2014 2:49 PM
>> > To: dev@cloudstack.apache.org
>> > Subject: [DISCUSS]CLOUDSTACK-6191
>> >
>> > Move the discussion to the list about CloudStack-6191:
>> > Automation test is blocked by this bug, we need to find a solution. My
>> suggestion is sort-of-revert-the-patch, but still give admin the opportunity to
>> specify the way to optimize KVM volume creation. The reasons:
>> >
>> > 1. End user shouldn't care about how the volume is created, is it
>> sparse,flat/thin-provisoned or whatever technology used by hypervisor. So
>> we'd better not expose this option in disk offering.
>> >
>> > 2. It's true that admin does care about how the volume is created, so
>> we can add a global configuration just for the kvm volume creation. For
>> vmware, the option is already there(vmware.create.full.clone to control
>> whether link clone or full clone is used to create volume). We can add an
>> option, something like kvm.qcow2.volume.create.options.
>> > Comments?
>>
>>
>>
>> --
>> Daan
--
Daan
RE: [DISCUSS]CLOUDSTACK-6191
Posted by Edison Su <Ed...@citrix.com>.
Yoshikazu fixed the issue related to qemu-img which introduced by his patch in cloudstack-6191.
But there is another issue introduced by commit: a600d8408ea86782318139c17cf346c8497943d0, which causes starting vm failure.
So I checked in a commit: e1095b0110f08fb0c7965f9cf293a6073bbce511, to fix CLOUDSTACK-7051.
So I think, both CLOUDSTACK-6191 and CLOUDSTACK-7051 should be fixed now, no need to revert or change CLOUDSTACK-6191.
> -----Original Message-----
> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> Sent: Saturday, July 12, 2014 11:02 AM
> To: dev
> Subject: Re: [DISCUSS]CLOUDSTACK-6191
>
> -0 What does it fix and is the solution bonifide. We should fix the test suite if
> it is. The test suite not working is not enough reason to revert a commit, it
> should block the test-suite because the system is broken, not because of the
> way the test suite works.
>
> Disclaimer: I do not know enough of KVM to make a judgement call. I just got
> triggered by the motivation in the mail thread.
>
> On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
> <ra...@citrix.com> wrote:
> > +1 Revert now and enable after complete full test in KVM
> >
> > KVM automation blocked more than 7 days due to this defect
> >
> > https://issues.apache.org/jira/browse/CLOUDSTACK-7051
> >
> > Regards,
> > Rayees
> >
> > -----Original Message-----
> > From: Edison Su [mailto:Edison.su@citrix.com]
> > Sent: Friday, July 11, 2014 2:49 PM
> > To: dev@cloudstack.apache.org
> > Subject: [DISCUSS]CLOUDSTACK-6191
> >
> > Move the discussion to the list about CloudStack-6191:
> > Automation test is blocked by this bug, we need to find a solution. My
> suggestion is sort-of-revert-the-patch, but still give admin the opportunity to
> specify the way to optimize KVM volume creation. The reasons:
> >
> > 1. End user shouldn't care about how the volume is created, is it
> sparse,flat/thin-provisoned or whatever technology used by hypervisor. So
> we'd better not expose this option in disk offering.
> >
> > 2. It's true that admin does care about how the volume is created, so
> we can add a global configuration just for the kvm volume creation. For
> vmware, the option is already there(vmware.create.full.clone to control
> whether link clone or full clone is used to create volume). We can add an
> option, something like kvm.qcow2.volume.create.options.
> > Comments?
>
>
>
> --
> Daan
Re: [DISCUSS]CLOUDSTACK-6191
Posted by Daan Hoogland <da...@gmail.com>.
-0 What does it fix and is the solution bonifide. We should fix the
test suite if it is. The test suite not working is not enough reason
to revert a commit, it should block the test-suite because the system
is broken, not because of the way the test suite works.
Disclaimer: I do not know enough of KVM to make a judgement call. I
just got triggered by the motivation in the mail thread.
On Sat, Jul 12, 2014 at 12:21 AM, Rayees Namathponnan
<ra...@citrix.com> wrote:
> +1 Revert now and enable after complete full test in KVM
>
> KVM automation blocked more than 7 days due to this defect
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-7051
>
> Regards,
> Rayees
>
> -----Original Message-----
> From: Edison Su [mailto:Edison.su@citrix.com]
> Sent: Friday, July 11, 2014 2:49 PM
> To: dev@cloudstack.apache.org
> Subject: [DISCUSS]CLOUDSTACK-6191
>
> Move the discussion to the list about CloudStack-6191:
> Automation test is blocked by this bug, we need to find a solution. My suggestion is sort-of-revert-the-patch, but still give admin the opportunity to specify the way to optimize KVM volume creation. The reasons:
>
> 1. End user shouldn't care about how the volume is created, is it sparse,flat/thin-provisoned or whatever technology used by hypervisor. So we'd better not expose this option in disk offering.
>
> 2. It's true that admin does care about how the volume is created, so we can add a global configuration just for the kvm volume creation. For vmware, the option is already there(vmware.create.full.clone to control whether link clone or full clone is used to create volume). We can add an option, something like kvm.qcow2.volume.create.options.
> Comments?
--
Daan
RE: [DISCUSS]CLOUDSTACK-6191
Posted by Rayees Namathponnan <ra...@citrix.com>.
+1 Revert now and enable after complete full test in KVM
KVM automation blocked more than 7 days due to this defect
https://issues.apache.org/jira/browse/CLOUDSTACK-7051
Regards,
Rayees
-----Original Message-----
From: Edison Su [mailto:Edison.su@citrix.com]
Sent: Friday, July 11, 2014 2:49 PM
To: dev@cloudstack.apache.org
Subject: [DISCUSS]CLOUDSTACK-6191
Move the discussion to the list about CloudStack-6191:
Automation test is blocked by this bug, we need to find a solution. My suggestion is sort-of-revert-the-patch, but still give admin the opportunity to specify the way to optimize KVM volume creation. The reasons:
1. End user shouldn't care about how the volume is created, is it sparse,flat/thin-provisoned or whatever technology used by hypervisor. So we'd better not expose this option in disk offering.
2. It's true that admin does care about how the volume is created, so we can add a global configuration just for the kvm volume creation. For vmware, the option is already there(vmware.create.full.clone to control whether link clone or full clone is used to create volume). We can add an option, something like kvm.qcow2.volume.create.options.
Comments?