Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker,ci,make: add Ceph-Dashboard support and other improvements #362

Merged

Conversation

epuertat
Copy link
Member

@epuertat epuertat commented Jan 12, 2024

This PR is to make it easier for developers to test Ceph-Dashboard when doing integration testing.
Fixes: #357
Fixes: #382

docker: add Ceph-Dashboard support

Include ceph-mgr-dashboard package and deps (grpcio).

Create new ceph-devel service to mount local ceph repo inside the container (for dev purposes).

docker: add ceph-devel container

Also:

  • Extend healthcheck retries to prevent random failures
  • Configure nvmeof gateway in Ceph
  • Fix nvmeof-cli aliases (the leading space prevented the shell from
    adding these to the history)
  • Add syntax highlight tag to the Dockerfile.spdk

docker: add custom Ceph repo

  • A custom Ceph YUM repo (usually a ceph-ci generated/Chacra one, like
    https://3.chacra.ceph.com/r/ceph/ceph-nvmeof-mon/c1ced49d133c2bcd1fa9311a35688bf641b93c76/centos/9/flavors/default/)
  • New make targer clean_cache to prune builder cache (required when
    changing the Ceph YUM Repo.
  • Repo/RPM GPG check disabled. Not ideal, but ceph-ci builds don't sign
    RPMs.
  • Specify ulimits (nofile 1024) for the Ceph container. In Docker CE,
    ceph-mon's ms_dispatch thread iterated through all available file
    descriptors and tried to close them. By setting nofile to 1024, this
    is heavily constrained. This behaviour is not exhibited with
    Fedora's Moby-engine.

docker: support target-arch in SPDK

  • Remove exising handling for AVX512*, VPCLMULQDQ and RDSEED
    instructions.
  • Replace it with spdk/configure --target-arc=
  • Tune SPDK's yum/dnf cache and settings.

ci: remove nvmeof-devel dependency

nvmeof-devel is not required for pytests, since they're mounted via run CLI command.

make: fix alias issue

Alias was not properly working for cephnvmf* as it was erased from the shell history.

docker: save SPDK target arch as label

SPDK/DPDK relies on pretty modern CPU instruction sets, which renders that incompatible with certain servers (Ivy Bridge and older). This new SPDK_TARGET_ARCH setting allows setting that. The new default (x86-64-v2) could result in a less performant SPDK.

ci: update upload-download artifact actions

The v4 of these actions provide 90% speed-ups by compressing artifacts.
Migration to this new action means zero cost.

Additional core clean-up performed.

ci: split ceph job and update checkout

There's no need for ceph to be built together with the rest of the nvmeof services. In fact this container could/should be pulled off to a separate repo/CI.

ci: cancel CI run when another starts

When a user pushes a change to a PR, if there was an ongoing CI run,
this will cancel any previous runs. This will save some CO2.

@epuertat epuertat linked an issue Jan 12, 2024 that may be closed by this pull request
@epuertat epuertat added the DevEx Improve the Developer Experience: coding, testing, troubleshooting, ... label Jan 12, 2024
@epuertat epuertat self-assigned this Jan 12, 2024
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from 9c5c5f5 to fa819e4 Compare January 12, 2024 21:14
Copy link
Collaborator

@baum baum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🖖

mk/misc.mk Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@epuertat epuertat linked an issue Jan 19, 2024 that may be closed by this pull request
@epuertat epuertat requested a review from gbregman January 19, 2024 14:17
@epuertat
Copy link
Member Author

@gbregman please review this PR as it removes part of the fixes you introduced for the missing CPU instruction.

@baum I pulled off the --target-arch configure param to a separate env. var, in order to make it easier to generate multiple SPDK versions from matrix job (e.g.: x86-64-v2 and x86-64-v4 might be enough to cover legacy and modern CPUs). Probably not required in every build, but just when publishing a release.

Copy link
Contributor

@gbregman gbregman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only I don't really know much about these files. One comment though, if you removed the SPDK_DISABLE_VPCLMULQDQ and SPDK_DISABLE_AVX512 vars, shouldn't you also remove them from .github/workflows/build-container.yml?

@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch 12 times, most recently from d645298 to 7bfac8b Compare January 19, 2024 21:48
@gbregman
Copy link
Contributor

@epuertat notice that on top of disabling the AVX512 and the MULQDQ instructions we also disable RDSEED. Only this id hardcoded in Dockerfile.spdk:

# Disable RDSEED, see https://github.com/ceph/ceph-nvmeof/issues/259
RUN \
    sed -i "s/^\( \+'RDSEED'\)/#\1/" dpdk/config/x86/meson.build

@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from d713622 to faa8fbd Compare January 22, 2024 20:27
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from dedcb06 to 0f0ca0c Compare January 22, 2024 21:00
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch 2 times, most recently from f28d751 to 5fdbf48 Compare January 23, 2024 10:10
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from 5fdbf48 to 010a045 Compare January 23, 2024 10:23
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from 010a045 to 15f8031 Compare January 23, 2024 10:24
The v4 of these actions provide 90% speed-ups by compressing artifacts.
Migration to this new action means zero cost.

Additional core clean-up performed.

Signed-off-by: Ernesto Puerta <[email protected]>
@epuertat epuertat changed the title docker: add Ceph-Dashboard support docker,ci,make: add Ceph-Dashboard support and other improvements Jan 23, 2024
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch from ad9abee to 7f89377 Compare January 23, 2024 18:33
When a user pushes a change to a PR, if there was an ongoing CI run,
this will cancel any previous runs. This will save some CO2.

Signed-off-by: Ernesto Puerta <[email protected]>
@epuertat epuertat force-pushed the 357-ceph-vstart-cluster-add-ceph-dashboard-support-and-more branch 11 times, most recently from 5751b3b to b6d3040 Compare January 24, 2024 10:35
@epuertat epuertat merged commit a732811 into devel Jan 24, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevEx Improve the Developer Experience: coding, testing, troubleshooting, ...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

dpdk: illegal instruction on Ivy Bridge CPU ceph-vstart-cluster: add Ceph-Dashboard support (and more)
3 participants