[TESTBUG] runtime/containers/docker/TestCPUAwareness.java failed in docker not supporting --cpus

Ao Qi aoqi at loongson.cn
Tue Feb 5 16:25:21 UTC 2019 

On Tue, Feb 5, 2019 at 11:25 PM Bob Vandette <bob.vandette at oracle.com> wrote:
>
> You change has been integrated.

Thanks, Bob.

>
> Bob.
>
> > On Feb 3, 2019, at 10:46 PM, Ao Qi <aoqi at loongson.cn> wrote:
> >
> > Hi Bob,
> >
> > I added reviewer information:
> > http://cr.openjdk.java.net/~aoqi/8217597/webrev.03/
> >
> > I used hg export, so I create all the information needed to push into
> > jdk/jdk according to my understanding. If there is something wrong
> > about these info, it's ok to adjust. Thanks, Bob.
> >
> > On Mon, Feb 4, 2019 at 2:24 AM Bob Vandette <bob.vandette at oracle.com> wrote:
> >>
> >> Sure.  Can you send me a patch ready to hg import and I’ll submit for you.
> >>
> >> Bob.
> >>
> >>
> >>> On Feb 3, 2019, at 12:20 PM, Ao Qi <aoqi at loongson.cn> wrote:
> >>>
> >>> Hi,
> >>>
> >>> Bob Vandette and David Holmes have reviewed. Could someone help to sponsor?
> >>>
> >>> Thanks,
> >>> Ao Qi
> >>>
> >>>> On Fri, Feb 1, 2019 at 6:52 PM Ao Qi <aoqi at loongson.cn> wrote:
> >>>>
> >>>>> On Fri, Feb 1, 2019 at 11:58 AM David Holmes <david.holmes at oracle.com> wrote:
> >>>>>
> >>>>> Reviewed.
> >>>>>
> >>>>
> >>>> Thanks, David.
> >>>>
> >>>>> I assume you also need a sponsor?
> >>>>
> >>>> Yes :)
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> David
> >>>>>
> >>>>>> On 1/02/2019 11:56 am, Ao Qi wrote:
> >>>>>> Thanks, Bob. You help a lot.
> >>>>>>
> >>>>>> Could someone help to review this small change?
> >>>>>>
> >>>>>>> On Fri, Feb 1, 2019 at 12:30 AM Bob Vandette <bob.vandette at oracle.com> wrote:
> >>>>>>>
> >>>>>>> I’m not a “R” reviewer but I’m ok with your change.
> >>>>>>>
> >>>>>>> Bob.
> >>>>>>>
> >>>>>>>
> >>>>>>>> On Jan 31, 2019, at 11:00 AM, Ao Qi <aoqi at loongson.cn> wrote:
> >>>>>>>>
> >>>>>>>> I leave the change, and update copyright year:
> >>>>>>>> http://cr.openjdk.java.net/~aoqi/8217597/webrev.02/ Could you help to
> >>>>>>>> review this?
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Ao Qi
> >>>>>>>>
> >>>>>>>>> On Thu, Jan 31, 2019 at 10:08 PM Bob Vandette <bob.vandette at oracle.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Yes, that does provide a bit more unique testing so leave the change as you had it.
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> Bob.
> >>>>>>>>>
> >>>>>>>>>> On Jan 28, 2019, at 8:41 PM, Ao Qi <aoqi at loongson.cn> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi Bob,
> >>>>>>>>>>
> >>>>>>>>>> Thanks! I am not a containers expert and have one small question. The
> >>>>>>>>>> maximum amount of --cpus is 4 (equivalent to both setting —cpu-period
> >>>>>>>>>> and —cpu-quota) in the already existing test. Is it valuable to keep
> >>>>>>>>>> testCpus(i, i) according to the max num of available CPUs? If not, I
> >>>>>>>>>> would also prefer removing the lines. In addition, I think I forgot to
> >>>>>>>>>> update the copyright year, it will be fixed in the next version of
> >>>>>>>>>> wevrev.
> >>>>>>>>>>
> >>>>>>>>>>> On Mon, Jan 28, 2019 at 11:35 PM Bob Vandette <bob.vandette at oracle.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> There is already a test that verifies —cpu-period and —cpu-quota.
> >>>>>>>>>>> I would just remove these lines.
> >>>>>>>>>>>
> >>>>>>>>>>> 62             // leave one CPU for system and tools, otherwise this test may be unstable
> >>>>>>>>>>> 63             int maxNrOfAvailableCpus =  availableCPUs - 1;
> >>>>>>>>>>> 64             for (int i=1; i < maxNrOfAvailableCpus; i = i * 2) {
> >>>>>>>>>>> 65                 testCpus(i, i);
> >>>>>>>>>>> 66             }
> >>>>>>>>>>>
> >>>>>>>>>>> 129     private static void testCpus(int valueToSet, int expectedTraceValue) throws Exception {
> >>>>>>>>>>> 130         Common.logNewTestCase("test cpus: " + valueToSet);
> >>>>>>>>>>> 131         DockerRunOptions opts = Common.newOpts(imageName)
> >>>>>>>>>>> 132             .addDockerOpts("--cpus", "" + valueToSet);
> >>>>>>>>>>> 133         Common.run(opts)
> >>>>>>>>>>> 134             .shouldMatch("active_processor_count.*" + expectedTraceValue);
> >>>>>>>>>>> 135     }
> >>>>>>>>>>>
> >>>>>>>>>>> Bob.
> >>>>>>>>>>>
> >>>>>>>>>>> On Jan 28, 2019, at 10:22 AM, Ao Qi <aoqi at loongson.cn> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> Since —cpus is a shortcut for of setting both --cpu-period and
> >>>>>>>>>>> --cpu-quota and the test is not intended to verify that docker works
> >>>>>>>>>>> correctly, I did not check the docker version and just replaced
> >>>>>>>>>>> setting --cpus with setting both --cpu-period and --cpu-quota. What do
> >>>>>>>>>>> you think of this patch:
> >>>>>>>>>>> http://cr.openjdk.java.net/~aoqi/8217597/webrev.01/ ?
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>> Ao Qi
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Jan 23, 2019 at 11:37 PM Bob Vandette <bob.vandette at oracle.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Since —cpus is just a shortcut for of setting both --cpu-period and --cpu-quota”, I’d
> >>>>>>>>>>> be ok with removing this test.  The tests are intended to test the container/cgroup
> >>>>>>>>>>> configuration detection logic and not to verify that docker works correctly.
> >>>>>>>>>>>
> >>>>>>>>>>> An alternate solution would be to add version detection to the Docker test check in
> >>>>>>>>>>> DockerTestUtils.java .   We already exec “docker ps” to see if docker is available
> >>>>>>>>>>> and enabled.
> >>>>>>>>>>>
> >>>>>>>>>>> % docker --version
> >>>>>>>>>>> Docker version 17.03.1-ce, build 276fd32
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Bob.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Jan 22, 2019, at 10:19 PM, Ao Qi <aoqi at loongson.cn> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Jan 23, 2019 at 10:55 AM David Holmes <david.holmes at oracle.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On 23/01/2019 11:58 am, Ao Qi wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi David,
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Jan 23, 2019 at 5:24 AM David Holmes <david.holmes at oracle.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> cc'ing Bob as our containers expert ...
> >>>>>>>>>>>
> >>>>>>>>>>> On 23/01/2019 1:10 am, Ao Qi wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> --cpus is available in Docker 1.13 and higher [1], so
> >>>>>>>>>>> runtime/containers/docker/TestCPUAwareness.java failed in docker which
> >>>>>>>>>>> does not support --cpus.
> >>>>>>>>>>>
> >>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~aoqi/docker/webrev.00/
> >>>>>>>>>>>
> >>>>>>>>>>> This patch skips the test if --cpus is not supported. I tested
> >>>>>>>>>>> runtime/containers/docker/TestCPUAwareness.java on a Fedora 25 (Docker
> >>>>>>>>>>> version 1.12.6, build ae7d637/1.12.6, not supporting --cpus) and
> >>>>>>>>>>> Ubuntu 16.04 (Docker version 17.03.2-ce, build f5ec1e2, supporting
> >>>>>>>>>>> --cpus)
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> The patch causes the test to pass if launching Docker fails for any
> >>>>>>>>>>> reason so that is not good.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I tested two versions of docker which does not support --cpus. Their
> >>>>>>>>>>> exit values when using --cpus are 2 and 125, and outputs are:
> >>>>>>>>>>>
> >>>>>>>>>>> flag provided but not defined: --cpus
> >>>>>>>>>>> See 'docker run --help'.
> >>>>>>>>>>>
> >>>>>>>>>>> and
> >>>>>>>>>>>
> >>>>>>>>>>> unknown flag: --cpus
> >>>>>>>>>>> See 'docker run --help'.
> >>>>>>>>>>>
> >>>>>>>>>>> My initial thought was that the else condition of
> >>>>>>>>>>> "output.getExitValue() == 0" should match the condition of "--cpus not
> >>>>>>>>>>> supported". Firstly I used output.shouldMatch("docker run --help"),
> >>>>>>>>>>> but I am not sure if all the docker version behaves this way when
> >>>>>>>>>>> --cpus is not supported and "docker run --help" does not certainly
> >>>>>>>>>>> indicate "--cpus not supported", so I removed the else condition.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I think we need to try and find a way to clearly identifyt eh failing
> >>>>>>>>>>> condition. Is there are "docker --version" we coudl check?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I will do more research. Checking docker version may be one option,
> >>>>>>>>>>> and checking whether one option is support by docker may be also one
> >>>>>>>>>>> option. I will try them later, while waiting if there are some other
> >>>>>>>>>>> opinions :)
> >>>>>>>>>>>
> >>>>>>>>>>> I am not sure if this is a testbug, so I did not file it on JBS. In
> >>>>>>>>>>> fact, I am not quite sure what kind of issue can be filed on JBS. Is
> >>>>>>>>>>> there any guidance document?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Any/all issues can be filed on JBS. You don't need to pre-classify as a
> >>>>>>>>>>> testbug, simple create an issue that a test is failing under specific
> >>>>>>>>>>> conditions. Whomever works on the bug will then determine whether it is
> >>>>>>>>>>> a testbug or product issue or something else. (We don't seem to have any
> >>>>>>>>>>> docs on using JBS ...)
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> What if the issue is not a bug or no body cares the issue? The issue
> >>>>>>>>>>> will be open on JBS forever?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Possibly :) But each component team performs regular triage of the bugs
> >>>>>>>>>>> that get filed and eventually things will be examined enough to see if
> >>>>>>>>>>> they are indeed a bug, and if not they will be closed as not an issue.
> >>>>>>>>>>> If a bug but low priority it may eventually get closed as "will not fix"
> >>>>>>>>>>> just to keep the open bug count down.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I was a little afraid that filing issues that are not bugs or nobody
> >>>>>>>>>>> cares would increase the workload of others and frustrate myself, so I
> >>>>>>>>>>> was not sure what kind of issue should be filed. Now I basically
> >>>>>>>>>>> clear, thanks David.
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>> David
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for your explanation, and I filed this issue on JBS:
> >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8217597
> >>>>>>>>>>>
> >>>>>>>>>>> In this case I'm not sure whether we require a docker version that
> >>>>>>>>>>> supports --cpus, and the test should be skipped otherwise. Though
> >>>>>>>>>>> ideally this would involve an explicit version check so we don't just
> >>>>>>>>>>> pass if the docker process fails.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> David
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>> Ao Qi
> >>>>>>>>>>>
> >>>>>>>>>>> [1] https://docs.docker.com/config/containers/resource_constraints/#cpu
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>
>

More information about the hotspot-runtime-dev mailing list