[OpenStack][stable-maint] In progress exceptions for Cinder backports
Hey, So I included this in a reply to a message about the freeze date, however I should probably send a separate message to facilitate better tracking etc. There are a number of in progress patches for Cinder that didn't make it before the freeze date and I'd like to request an exception for them. The first three items have been in progress reviews for stable. Each of them are viewed as safe and address valid customer reported issues. There is also one outstanding item that I've been waiting on to land in master to do a backport on [4] that I'd also like to see considered for acceptance as well. It would be a shame for these issues to slip out to the next stable release. I realize that this one might be pushing it in terms of the time-line but I'd be interested in raising it anyway. Thanks, John [1] https://review.openstack.org/#/c/28212/ [2] https://review.openstack.org/#/c/28207/ [3] https://review.openstack.org/#/c/28210/ [4] https://review.openstack.org/#/c/28208/
2013/5/6 John Griffith <john.griffith@solidfire.com>:
acked, but I'd like to see one more review
acked, but I'd like to see one more review
+76, -68 seems big but not that bad upon closer look and it's localized to huawei driver acked, but I'd like to see one more review
There is also one outstanding item that I've been waiting on to land in master to do a backport on [4] ... [4] https://review.openstack.org/#/c/28208/
+115, -65 seems big and touches a number drivers but changes look safe. When backport is ready, I'd like to see more than minimum 2 stable reviewers on it. Cheers, Alan
On Mon, May 6, 2013 at 5:43 PM, Alan Pevec <apevec@gmail.com> wrote:
2013/5/6 John Griffith <john.griffith@solidfire.com>:
acked, but I'd like to see one more review
acked, but I'd like to see one more review
+76, -68 seems big but not that bad upon closer look and it's localized to huawei driver acked, but I'd like to see one more review
There is also one outstanding item that I've been waiting on to land in master to do a backport on [4] ... [4] https://review.openstack.org/#/c/28208/
+115, -65 seems big and touches a number drivers but changes look safe. When backport is ready, I'd like to see more than minimum 2 stable reviewers on it.
Cheers, Alan
Great.. thanks Alan!
On 05/06/2013 04:43 PM, Alan Pevec wrote:
2013/5/6 John Griffith <john.griffith@solidfire.com>:
[1] https://review.openstack.org/#/c/28212/ acked, but I'd like to see one more review
[2] https://review.openstack.org/#/c/28207/ acked, but I'd like to see one more review
[3] https://review.openstack.org/#/c/28210/ +76, -68 seems big but not that bad upon closer look and it's localized to huawei driver acked, but I'd like to see one more review
Hey Guys- I'm probably being a stickler, and I trust John's judgment on proposing the patches as-is, but I'm a bit concerned about the following: https://review.openstack.org/#/c/28207/ https://review.openstack.org/#/c/28210/ Both commits contain a bug fix plus some other random stuff. The former seems trivial but the second not so much. I realize we've gotten comfortable with 'git cherry-pick' but should we be more strict about ensuring backported patches contain only the minimum required to fix the bug? IMHO, I think cherry-pick'ing only works for minimal patches from master and we should be diligent about NACKing anything into stable that brings unrelated/unwated changes. Adam
On Mon, May 6, 2013 at 7:44 PM, Adam Gandelman <adamg@canonical.com> wrote:
On 05/06/2013 04:43 PM, Alan Pevec wrote:
2013/5/6 John Griffith <john.griffith@solidfire.com>:
[1] https://review.openstack.org/#**/c/28212/<https://review.openstack.org/#/c/28212/>
acked, but I'd like to see one more review
[2] https://review.openstack.org/#**/c/28207/<https://review.openstack.org/#/c/28207/>
acked, but I'd like to see one more review
[3] https://review.openstack.org/#**/c/28210/<https://review.openstack.org/#/c/28210/>
+76, -68 seems big but not that bad upon closer look and it's localized to huawei driver acked, but I'd like to see one more review
Hey Guys-
I'm probably being a stickler, and I trust John's judgment on proposing the patches as-is, but I'm a bit concerned about the following:
https://review.openstack.org/#**/c/28207/<https://review.openstack.org/#/c/28207/>
https://review.openstack.org/#**/c/28210/<https://review.openstack.org/#/c/28210/>
Both commits contain a bug fix plus some other random stuff. The former seems trivial but the second not so much. I realize we've gotten comfortable with 'git cherry-pick' but should we be more strict about ensuring backported patches contain only the minimum required to fix the bug? IMHO, I think cherry-pick'ing only works for minimal patches from master and we should be diligent about NACKing anything into stable that brings unrelated/unwated changes.
Adam
Hey Adam, Thanks for the feed-back and I agree with your point. https://review.openstack.org/#/c/28207/ is the Cherry Pick and was in fact intended to include a fix for both items. In the future we (the Cinder team and more specifically me) should probably be better about separate commits for separate issues. That being said it's basically a one line change that needed to be made so it was included, but the issue here is the commit to master and not the commit to stable. https://review.openstack.org/#/c/28210/ is another story, and I agree the addition of the fix to the formatting of the log messages should probably not be included. It started off as a single log entry with a misspelling that earned it a -1 in the review and then as we looked closer we found more and more typos and formatting issues in the log statements. It's safe as it's isolated to the logging output, however you're probably right that it should not be in this particular patch. I'm happy to remove those *extras* and resubmit. Thanks, John
2013/5/7 John Griffith <john.griffith@solidfire.com>:
On Mon, May 6, 2013 at 7:44 PM, Adam Gandelman <adamg@canonical.com> wrote:
I'm probably being a stickler, and I trust John's judgment on proposing the patches as-is, but I'm a bit concerned about the following:
Thanks for the feedback Adam, that's why I said I want to see one more review before approving.
https://review.openstack.org/#/c/28207/ is the Cherry Pick and was in fact intended to include a fix for both items. In the future we (the Cinder team and more specifically me) should probably be better about separate commits for separate issues. That being
Definitely, when you're fixing an issue on master which is already tagged for backport in Launchpad, please do separate commits.
said it's basically a one line change that needed to be made so it was included, but the issue here is the commit to master and not the commit to stable.
https://review.openstack.org/#/c/28210/ is another story, and I agree the addition of the fix to the formatting of the log messages should probably not be included. It started off as a single log entry with a misspelling that earned it a -1 in the review and then as we looked closer we found more and more typos and formatting issues in the log statements. It's safe as it's isolated to the logging output, however you're probably right that it should not be in this particular patch. I'm happy to remove those *extras* and resubmit.
Yes, please resubmit only critical fix, as request by Adam. I'm not that concerned since changes looked sane and are localized in one driver only. Diverging from master is bad for future backports, but since we want to release in 2 days, let's make minimal change now and sync-up stable with master later if/when further backports are needed. Thanks, Alan
2013/5/7 Alan Pevec <apevec@gmail.com>:
There is also one outstanding item that I've been waiting on to land in master to do a backport on [4] ... [4] https://review.openstack.org/#/c/28208/
+115, -65 seems big and touches a number drivers but changes look safe. When backport is ready, I'd like to see more than minimum 2 stable reviewers on it.
stable/grizzly backport is https://review.openstack.org/28440 I'm convinced this is worth freeze exception, but we need one more stable-maint member to review it, in case John or I missed something. Thanks, Alan
participants (3)
-
Adam Gandelman
-
Alan Pevec
-
John Griffith