10 Feb
2023
10 Feb
'23
7:39 p.m.
As part of an effort to build and test ceph in containers I posted a PR in
November:
https://github.com/ceph/ceph/pull/48697
As noted in the PR there are two approvals but no one seems willing or has the
time to shepherd the PR through the merge process.
I'm not sure what my next steps on this PR can be. I had planned on having
this PR serve as an intermediate step towards being able to build from source
and run 'make tests' in containers. This is an effort Enesto started and I've
been working on for a while. I'm currently blocked because of the uncertainty
around this PR.
If anyone has any thoughts or recommendations for me I'd appreciate it. I did
ping ceph/core in the PR as well, but I figured I might get some more attention
here on the list.
Thanks!
10 Feb
10 Feb
8:07 p.m.
Hi John,
Really sorry about the wait. Unfortunately I think the reality is that
there are more people trying to contribute code than there are people
that can review code right now. Even in the core OSD code we have cases
where Adam, Igor and I all have PRs we've made that we should be
reviewing for each other and they can end up spending months in queue
because none of us have time to properly review and merge them. It's
not personal, just the reality of everyone trying to balance writing
code, reviewing code, and still having some kind of life outside work
(It's easy to be completely consumed and burn out if you let it, I've
been there).
I see Kefu approved your PR, but doesn't want to merge it himself as
he's not a maintainer anymore. That's fair. He used to maintain quite
a bit of the build script stuff but since he left we haven't really had
any specific point person take over afaik. I looked over the PR but I'm
not super familiar with the internals of install-deps.sh or run-make.sh.
I'd really feel better if someone that's worked on those files before
did another review and approved the merge. If that doesn't happen,
maybe we can merge based on Kefu's approval alone.
Please don't hesitate to keep poking us. :)
Mark
On 2/10/23 10:39, John Mulligan wrote:
> As part of an effort to build and test ceph in containers I posted a PR in
> November:
>
> https://github.com/ceph/ceph/pull/48697
>
> As noted in the PR there are two approvals but no one seems willing or has the
> time to shepherd the PR through the merge process.
>
> I'm not sure what my next steps on this PR can be. I had planned on having
> this PR serve as an intermediate step towards being able to build from source
> and run 'make tests' in containers. This is an effort Enesto started and
I've
> been working on for a while. I'm currently blocked because of the uncertainty
> around this PR.
>
> If anyone has any thoughts or recommendations for me I'd appreciate it. I did
> ping ceph/core in the PR as well, but I figured I might get some more attention
> here on the list.
> Thanks!
>
> _______________________________________________
> Dev mailing list -- dev(a)ceph.io
> To unsubscribe send an email to dev-leave(a)ceph.io
8:42 p.m.
Hi John,
Thanks for bringing this PR to our attention!
The general process to merge PRs in Ceph involves code reviews and making
sure relevant tests are passing. Once a PR gets approved, a "needs-qa"
label is added to PRs to indicate that the PR is ready for testing. In most
cases, this testing means running relevant test suites in teuthology. In
cases where teuthology testing is not needed, it is fine to summarize the
validation that has been done for the PR as a comment to indicate that the
PR is ready for merge. This process is more formally described in
https://docs.ceph.com/en/quincy/dev/developer_guide/basic-workflow/.
As far as your PR is concerned, I am not seeing a "needs-qa" label on it.
Do you think the PR has gone through adequate testing and can be merged as
is?
Thanks,
Neha
On Fri, Feb 10, 2023 at 9:08 AM Mark Nelson <mark.a.nelson(a)gmail.com> wrote:
Hi John,
Really sorry about the wait. Unfortunately I think the reality is that
there are more people trying to contribute code than there are people
that can review code right now. Even in the core OSD code we have cases
where Adam, Igor and I all have PRs we've made that we should be
reviewing for each other and they can end up spending months in queue
because none of us have time to properly review and merge them. It's
not personal, just the reality of everyone trying to balance writing
code, reviewing code, and still having some kind of life outside work
(It's easy to be completely consumed and burn out if you let it, I've
been there).
I see Kefu approved your PR, but doesn't want to merge it himself as
he's not a maintainer anymore. That's fair. He used to maintain quite
a bit of the build script stuff but since he left we haven't really had
any specific point person take over afaik. I looked over the PR but I'm
not super familiar with the internals of install-deps.sh or run-make.sh.
I'd really feel better if someone that's worked on those files before
did another review and approved the merge. If that doesn't happen,
maybe we can merge based on Kefu's approval alone.
Please don't hesitate to keep poking us. :)
Mark
On 2/10/23 10:39, John Mulligan wrote:
As part of an effort to build and test ceph in
containers I posted a PR
in
November:
https://github.com/ceph/ceph/pull/48697
As noted in the PR there are two approvals but no one seems willing or
has the
time to shepherd the PR through the merge
process.
I'm not sure what my next steps on this PR can be. I had planned on
having
this PR serve as an intermediate step towards
being able to build from
source
and run 'make tests' in containers. This
is an effort Enesto started and
I've
been working on for a while. I'm currently
blocked because of the
uncertainty
around this PR.
If anyone has any thoughts or recommendations for me I'd appreciate it.
I did
ping ceph/core in the PR as well, but I figured I
might get some more
attention
here on the list.
Thanks!
_______________________________________________
Dev mailing list -- dev(a)ceph.io
To unsubscribe send an email to dev-leave(a)ceph.io
_______________________________________________
Dev mailing list -- dev(a)ceph.io
To unsubscribe send an email to dev-leave(a)ceph.io
11 Feb
11 Feb
6:04 p.m.
On Friday, February 10, 2023 12:42:06 PM EST Neha Ojha wrote:
Hi John,
Thanks for bringing this PR to our attention!
The general process to merge PRs in Ceph involves code reviews and making
sure relevant tests are passing. Once a PR gets approved, a "needs-qa"
label is added to PRs to indicate that the PR is ready for testing. In most
cases, this testing means running relevant test suites in teuthology. In
cases where teuthology testing is not needed, it is fine to summarize the
validation that has been done for the PR as a comment to indicate that the
PR is ready for merge. This process is more formally described in
https://docs.ceph.com/en/quincy/dev/developer_guide/basic-workflow/.
Yes, thanks. Most of my other collaborations on the ceph repo have been
through the orchestration team and have worked with team lead Adam King who
generally merges orchestration focused PRs. Most of my other contributions
have been labeled with "needs-qa" and run through teuthology by Adam. With
regards to this PR which lies outside the scope of orchestration, Adam's in a
similar situation to Mark in the previous message... not familiar enough with
the scripts to want to get directly involved.
On a technical level I am 90% confident that these scripts are only executed by
the jenkins CI jobs (and manually by some people) and are not parts of the
build and test runs executed by teuthology.
As far as your PR is concerned, I am not seeing a
"needs-qa" label on it.
Do you think the PR has gone through adequate testing and can be merged as
is?
I'd like to think so, but this is where I had hoped to get the guidance of a
more experienced ceph contributor... especially one who is familiar with these
scripts. For these patches I do think the jenkins CI jobs should be largely
sufficient but I've only been hacking on them a couple of months. What I can
promise is that if they are merged and anyone finds issues I *will* support
them and provide fixes, etc. as promptly as possible.
I appreciate the thoughts provided so far. I'm still not sure what my
actionable "next steps" should be.
Thanks,
Neha
On Fri, Feb 10, 2023 at 9:08 AM Mark Nelson <mark.a.nelson(a)gmail.com> wrote:
> Hi John,
>
> Really sorry about the wait. Unfortunately I think the reality is that
> there are more people trying to contribute code than there are people
> that can review code right now. Even in the core OSD code we have cases
> where Adam, Igor and I all have PRs we've made that we should be
> reviewing for each other and they can end up spending months in queue
> because none of us have time to properly review and merge them. It's
> not personal, just the reality of everyone trying to balance writing
> code, reviewing code, and still having some kind of life outside work
> (It's easy to be completely consumed and burn out if you let it, I've
> been there).
>
> I see Kefu approved your PR, but doesn't want to merge it himself as
> he's not a maintainer anymore. That's fair. He used to maintain quite
> a bit of the build script stuff but since he left we haven't really had
> any specific point person take over afaik. I looked over the PR but I'm
> not super familiar with the internals of install-deps.sh or run-make.sh.
>
> I'd really feel better if someone that's worked on those files before
>
> did another review and approved the merge. If that doesn't happen,
> maybe we can merge based on Kefu's approval alone.
>
> Please don't hesitate to keep poking us. :)
>
> Mark
>
> On 2/10/23 10:39, John Mulligan wrote:
> > As part of an effort to build and test ceph in containers I posted a PR
>
> in
>
> > November:
> >
> > https://github.com/ceph/ceph/pull/48697
> >
> > As noted in the PR there are two approvals but no one seems willing or
>
> has the
>
> > time to shepherd the PR through the merge process.
> >
> > I'm not sure what my next steps on this PR can be. I had planned on
>
> having
>
> > this PR serve as an intermediate step towards being able to build from
>
> source
>
> > and run 'make tests' in containers. This is an effort Enesto started
and
>
> I've
>
> > been working on for a while. I'm currently blocked because of the
>
> uncertainty
>
> > around this PR.
> >
> > If anyone has any thoughts or recommendations for me I'd appreciate it.
>
> I did
>
> > ping ceph/core in the PR as well, but I figured I might get some more
>
> attention
>
> > here on the list.
> > Thanks!
> >
> > _______________________________________________
> > Dev mailing list -- dev(a)ceph.io
> > To unsubscribe send an email to dev-leave(a)ceph.io
>
> _______________________________________________
> Dev mailing list -- dev(a)ceph.io
> To unsubscribe send an email to dev-leave(a)ceph.io
13 Feb
13 Feb
10:15 p.m.
I've added my test tag to the PR. Will make sure builds aren't affected and
maybe run it through a few tests (on top of the orch run I'll use the build
for anyway, but that only uses the containers which only use the centos 8
packages currently). If all is well, I'll probably end up merging it. As
John said, he's willing to actually support the patches if there are issues
created and we don't have anyone really maintaining these scripts
currently, so that seems like the most reasonable way forward.
- Adam King
On Sat, Feb 11, 2023 at 10:07 AM John Mulligan <phlogistonjohn(a)asynchrono.us>
wrote:
On Friday, February 10, 2023 12:42:06 PM EST Neha Ojha
wrote:
Hi John,
Thanks for bringing this PR to our attention!
The general process to merge PRs in Ceph involves code reviews and making
sure relevant tests are passing. Once a PR gets approved, a "needs-qa"
label is added to PRs to indicate that the PR is ready for testing. In
most
cases, this testing means running relevant test
suites in teuthology. In
cases where teuthology testing is not needed, it is fine to summarize the
validation that has been done for the PR as a comment to indicate that
the
PR is ready for merge. This process is more
formally described in
https://docs.ceph.com/en/quincy/dev/developer_guide/basic-workflow/.
Yes, thanks. Most of my other collaborations on the ceph repo have been
through the orchestration team and have worked with team lead Adam King
who
generally merges orchestration focused PRs. Most of my other contributions
have been labeled with "needs-qa" and run through teuthology by Adam. With
regards to this PR which lies outside the scope of orchestration, Adam's
in a
similar situation to Mark in the previous message... not familiar enough
with
the scripts to want to get directly involved.
On a technical level I am 90% confident that these scripts are only
executed by
the jenkins CI jobs (and manually by some people) and are not parts of the
build and test runs executed by teuthology.
As far as your PR is concerned, I am not seeing a
"needs-qa" label on it.
Do you think the PR has gone through adequate testing and can be merged
as
is?
I'd like to think so, but this is where I had hoped to get the guidance of
a
more experienced ceph contributor... especially one who is familiar with
these
scripts. For these patches I do think the jenkins CI jobs should be
largely
sufficient but I've only been hacking on them a couple of months. What I
can
promise is that if they are merged and anyone finds issues I *will*
support
them and provide fixes, etc. as promptly as possible.
I appreciate the thoughts provided so far. I'm still not sure what my
actionable "next steps" should be.
Thanks,
Neha
On Fri, Feb 10, 2023 at 9:08 AM Mark Nelson <mark.a.nelson(a)gmail.com>
wrote:
> Hi John,
>
> Really sorry about the wait. Unfortunately I think the reality is that
> there are more people trying to contribute code than there are people
> that can review code right now. Even in the core OSD code we have
cases
> where Adam, Igor and I all have PRs
we've made that we should be
> reviewing for each other and they can end up spending months in queue
> because none of us have time to properly review and merge them. It's
> not personal, just the reality of everyone trying to balance writing
> code, reviewing code, and still having some kind of life outside work
> (It's easy to be completely consumed and burn out if you let it, I've
> been there).
>
> I see Kefu approved your PR, but doesn't want to merge it himself as
> he's not a maintainer anymore. That's fair. He used to maintain quite
> a bit of the build script stuff but since he left we haven't really had
> any specific point person take over afaik. I looked over the PR but
I'm
> not super familiar with the internals of
install-deps.sh or
run-make.sh.
>
> I'd really feel better if someone that's worked on those files before
>
> did another review and approved the merge. If that doesn't happen,
> maybe we can merge based on Kefu's approval alone.
>
> Please don't hesitate to keep poking us. :)
>
> Mark
>
> On 2/10/23 10:39, John Mulligan wrote:
> > As part of an effort to build and test ceph in containers I posted a
PR
>
> in
>
> > November:
> >
> > https://github.com/ceph/ceph/pull/48697
> >
> > As noted in the PR there are two approvals but no one seems willing
or
>
> has the
>
> > time to shepherd the PR through the merge process.
> >
> > I'm not sure what my next steps on this PR can be. I had planned on
>
> having
>
> > this PR serve as an intermediate step towards being able to build
from
>
> source
>
> > and run 'make tests' in containers. This is an effort Enesto started
and
>
> I've
>
> > been working on for a while. I'm currently blocked because of the
>
> uncertainty
>
> > around this PR.
> >
> > If anyone has any thoughts or recommendations for me I'd appreciate
it.
>
> I did
>
> > ping ceph/core in the PR as well, but I figured I might get some more
>
> attention
>
> > here on the list.
> > Thanks!
> >
> > _______________________________________________
> > Dev mailing list -- dev(a)ceph.io
> > To unsubscribe send an email to dev-leave(a)ceph.io
>
> _______________________________________________
> Dev mailing list -- dev(a)ceph.io
> To unsubscribe send an email to dev-leave(a)ceph.io
_______________________________________________
Dev mailing list -- dev(a)ceph.io
To unsubscribe send an email to dev-leave(a)ceph.io
461
days inactive
464
days old
4 comments
4 participants
participants (4)
-
Adam King -
John Mulligan -
Mark Nelson -
Neha Ojha