On Mon, Jan 23, 2023 at 10:58 AM Ales Musil <[email protected]> wrote:
>
> Hi Numan,
>

Hi Ales,  Thanks for the reviews.  Please see below for some comments.


>
> On Thu, Dec 1, 2022 at 8:26 PM <[email protected]> wrote:
>
> > From: Numan Siddique <[email protected]>
> >
> > This patch adds a couple of jobs using ovn-fake-multinode.
> > It first builds 2 ovn-fake-multinode container images
> >   - one with OVN 22.03
> >   - one with present main.
> >
> > The first job deploys ovn-fake-multinode with the main
> > OVN and runs simple tests provided by ovn-fake-multinode [1].
> >
> > The second job deploys ovn-fake-multinode setup with the
> > central image using OVN 22.03 and chassis image using main OVN.
> > This job tests the scenario
> >   - ovn-northd and OVN dbs are running the most recent LTS.
> >   - ovn-controller is running the latest commit from the branch.
> >
> > [1] -
> > https://github.com/ovn-org/ovn-fake-multinode/blob/main/.ci/test_basic.sh
> >
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
> >  .../workflows/ovn-fake-multinode-tests.yml    | 151 ++++++++++++++++++
> >  Makefile.am                                   |   1 +
> >  2 files changed, 152 insertions(+)
> >  create mode 100644 .github/workflows/ovn-fake-multinode-tests.yml
> >
> > diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
> > b/.github/workflows/ovn-fake-multinode-tests.yml
> > new file mode 100644
> > index 000000000..3727b9835
> > --- /dev/null
> > +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> > @@ -0,0 +1,151 @@
> > +name: System tests using ovn-fake-multinode
> > +
> > +on:
> > +  push:
> > +  pull_request:
> > +  schedule:
> > +    # Run Sunday at midnight
> > +    - cron: '0 0 * * 0'
> >
>
> I'm a little worried because our job queue is pretty crowded currently, do
> we know how long this takes?
> Would it be enough to run it just nightly?

I'm fine with nightly.  I've submitted v2 and I haven't changed to
nightly.  I want the ovnrobot to kick in and run these tests.

In the next version or before merging I'll change it to nightly.

I think we can make it a regular job (i.e for each patch pushed and
for each commit) if we can cache the ovn-fake-multinode container.
I'd like to revisit that as a follow up patch.

>
>
> > +
> > +concurrency:
> > +  group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
> > github.run_id }}
> > +  cancel-in-progress: true
> > +
> > +jobs:
> > +  build:
> > +    env:
> > +      RUNC_CMD: podman
> > +      OS_IMAGE: "fedora:36"
> > +      # https://github.com/actions/runner-images/issues/6282
> > +      XDG_RUNTIME_DIR: ''
> > +
> > +    name: Build ovn-fake-multinode image
> > +    runs-on: ubuntu-20.04
> > +    steps:
> > +    - name: Check out ovn-fake-multi-node
> > +      uses: actions/checkout@v3
> > +      with:
> > +        repository: 'ovn-org/ovn-fake-multinode'
> > +        path: 'ovn-fake-multinode'
> > +        ref: 'v0.1'
> > +
> > +    - name: Check out ovn
> > +      uses: actions/checkout@v3
> > +      with:
> > +        path: 'ovn-fake-multinode/ovn'
> > +        submodules: recursive
> > +
> > +    - name: Check out ovs master
> > +      uses: actions/checkout@v3
> > +      with:
> > +        path: 'ovn-fake-multinode/ovs'
> > +        repository: 'openvswitch/ovs'
> > +        ref: 'master'
> > +
> > +    - name: Install dependencies
> > +      run: |
> > +        sudo apt update
> > +        sudo apt-get install -y podman
> > +
> > +    - name: Build ovn-fake-multi-node main image
> > +      run: |
> > +        set -x
> > +        cd ovn-fake-multinode
> >
>
> No need to use cd, you can use "working-directory: ovn-fake-multinode".

Ack.  Done in v2.
>
>
> > +        sudo -E ./ovn_cluster.sh build
> > +        mkdir -p /tmp/_output
> > +        sudo podman save ovn/ovn-multi-node:latest >
> > /tmp/_output/ovn_main_image.tar
> > +
> > +    - name: Build ovn-fake-multi-node 22.03 image
> > +      run: |
> > +        set -x
> > +        cd ovn-fake-multinode/ovn
> >
>
> Same.
Ack.  Done in v2.
>
>
> > +        git fetch origin
> > +        git checkout -b branch-22.03 origin/branch-22.03
> > +        git status
> > +        cd ..
> >
>
> This switch back feels like it should be rather two separate commands.

Ack.  Done in v2.

>
>
> > +        sudo -E ./ovn_cluster.sh build
> > +        mkdir -p /tmp/_output
> > +        sudo podman tag ovn/ovn-multi-node:latest ovn/ovn-multi-node:22.03
> > +        sudo podman save ovn/ovn-multi-node:22.03 >
> > /tmp/_output/ovn_22_03_image.tar
> > +
> > +    - uses: actions/upload-artifact@v3
> > +      with:
> > +        name: test-main-image
> > +        path: /tmp/_output/ovn_main_image.tar
> > +
> > +    - uses: actions/upload-artifact@v3
> > +      with:
> > +        name: test-22-03-image
> > +        path: /tmp/_output/ovn_22_03_image.tar
> > +
> > +  multinode-tests:
> > +    runs-on: ubuntu-20.04
> > +    timeout-minutes: 120
> > +    env:
> > +      RUNC_CMD: podman
> > +      OS_IMAGE: "fedora:36"
> > +      CENTRAL_IMAGE: ${{ matrix.cfg.central_image }}
> > +      ENABLE_SSL: no
> >
>
> What's the reason for excluding SSL?
>
>
> > +      # https://github.com/actions/runner-images/issues/6282
> > +      XDG_RUNTIME_DIR: ''
> > +
> > +    name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
> > +    needs: [build]
> > +    strategy:
> > +      fail-fast: false
> > +      matrix:
> > +        cfg:
> > +        - { central_image: "ovn/ovn-multi-node:latest" }
> > +        - { central_image: "ovn/ovn-multi-node:22.03" }
> > +
> > +    steps:
> > +
> > +    - name: Free up disk space
> > +      run: sudo eatmydata apt-get remove --auto-remove -y aspnetcore-*
> > dotnet-* libmono-* mono-* msbuild php-* php7* ghc-* zulu-*
> > +
> > +    - uses: actions/download-artifact@v3
> > +      with:
> > +        name: test-main-image
> > +
> > +    - uses: actions/download-artifact@v3
> > +      with:
> > +        name: test-22-03-image
> > +
> > +    - name: Load podman image
> > +      run: |
> > +        sudo podman load --input ovn_main_image.tar
> > +        sudo podman load --input ovn_22_03_image.tar
> > +
> > +    - name: Check out ovn-fake-multi-node
> > +      uses: actions/checkout@v3
> > +      with:
> > +        repository: 'ovn-org/ovn-fake-multinode'
> > +        path: 'ovn-fake-multinode'
> > +        ref: 'v0.1'
> > +
> > +    - name: Install dependencies
> > +      run: |
> > +        sudo apt update
> > +        sudo apt-get install -y podman openvswitch-switch
> > +        sudo systemctl start openvswitch-switch
> > +        sudo ovs-vsctl show
> > +
> > +    - name: Start basic cluster
> > +      run: |
> > +        pwd
> > +        ls -l
> > +        cd ovn-fake-multinode
> >
>
> Same as before with cd.
>
>
> > +        sudo -E ./ovn_cluster.sh start
> > +        sudo podman exec -it ovn-central ovn-nbctl show
> > +        sudo podman exec -it ovn-central ovn-appctl -t ovn-northd version
> > +        sudo podman exec -it ovn-chassis-1 ovn-appctl -t ovn-controller
> > version
> > +
> > +    - name: Run basic test script
> > +      run: |
> > +        cd ovn-fake-multinode
> >
>
> Same.
>
>
> > +        sudo ./.ci/test_basic.sh
> > +
> > +    - name: Stop cluster
> > +      run: |
> > +        cd ovn-fake-multinode
> > +        sudo -E ./ovn_cluster.sh stop
> >
>
> We should probably attempt to stop the cluster always:
>
> if: always()

I missed this one and will address it in the next version or before
merging if v2 looks good.

Thanks.
Numan

>
>
> > diff --git a/Makefile.am b/Makefile.am
> > index 3b0df8393..6c3baa21c 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -93,6 +93,7 @@ EXTRA_DIST = \
> >         .ci/ovn-kubernetes/Dockerfile \
> >         .github/workflows/test.yml \
> >         .github/workflows/ovn-kubernetes.yml \
> > +       .github/workflows/ovn-fake-multinode-tests.yml \
> >         boot.sh \
> >         $(MAN_FRAGMENTS) \
> >         $(MAN_ROOTS) \
> > --
> > 2.38.1
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> The change looks good overall, I have just some smaller questions/comments.
>
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> [email protected]    IM: amusil
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to