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

osd: set blkdevmapper capabilities #9158

Merged
merged 1 commit into from Nov 15, 2021
Merged

osd: set blkdevmapper capabilities #9158

merged 1 commit into from Nov 15, 2021

Conversation

Omar007
Copy link
Contributor

@Omar007 Omar007 commented Nov 12, 2021

Description of your changes:
The OSD blkdevmapper init container relies on the MKNOD capability, which it does not actually request.
As a result, deployments fail on Kubernetes clusters that do not happen to assign this capability to all containers by default.

This PR introduces a function that constructs a securityContext tailored to the blkdevmapper use-case instead of relying on the controller.PodSecurityContext function.

This change also implies blkdevmapper will no longer become privileged when ROOK_HOSTPATH_REQUIRES_PRIVILEGED is true but as far as I can tell this should never be happening/needed for the blkdevmapper use-case anyway. (unless I missed something?)

Which issue is resolved by this Pull Request:
Resolves #9156

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

@travisn
Copy link
Member

travisn commented Nov 12, 2021

See the unit tests results, there are a few tests that need to be updated.

@travisn travisn requested a review from leseb November 12, 2021 15:35
@Omar007
Copy link
Contributor Author

Omar007 commented Nov 12, 2021

@travisn yea I got the failure notification a moment ago. I was looking at those logs but I have no clue what I am looking and what it has to do with anything being changed here. The one that fails is TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile.

=== CONT  TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile
    integration_test.go:337: c.Start() error: context canceled
        failed to update/create OSDs
        github.com/rook/rook/pkg/operator/ceph/cluster/osd.(*Cluster).Start
        	/home/runner/go/src/github.com/rook/rook/pkg/operator/ceph/cluster/osd/osd.go:222
        github.com/rook/rook/pkg/operator/ceph/cluster/osd.testOSDIntegration.func4
        	/home/runner/go/src/github.com/rook/rook/pkg/operator/ceph/cluster/osd/integration_test.go:240
        runtime.goexit
        	/opt/hostedtoolcache/go/1.16.10/x64/src/runtime/asm_amd64.s:1371
    integration_test.go:339: 
        	Error Trace:	integration_test.go:339
        	Error:      	"[rook-ceph-osd-23]" should have 2 item(s), but has 1
        	Test:       	TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile
    integration_test.go:342: deployments updated: 1
=== CONT  TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile
    integration_test.go:353: 
        	Error Trace:	integration_test.go:353
        	Error:      	"[rook-ceph-osd-25 rook-ceph-osd-26 rook-ceph-osd-24 rook-ceph-osd-27]" should have 3 item(s), but has 4
        	Test:       	TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile
    integration_test.go:354: 
        	Error Trace:	integration_test.go:354
        	Error:      	"[rook-ceph-osd-0 rook-ceph-osd-1 rook-ceph-osd-10 rook-ceph-osd-11 rook-ceph-osd-12 rook-ceph-osd-13 rook-ceph-osd-14 rook-ceph-osd-15 rook-ceph-osd-16 rook-ceph-osd-17 rook-ceph-osd-18 rook-ceph-osd-19 rook-ceph-osd-2 rook-ceph-osd-20 rook-ceph-osd-21 rook-ceph-osd-22 rook-ceph-osd-23 rook-ceph-osd-3 rook-ceph-osd-4 rook-ceph-osd-5 rook-ceph-osd-6 rook-ceph-osd-7 rook-ceph-osd-8 rook-ceph-osd-9]" should have 25 item(s), but has 24
        	Test:       	TestOSDIntegration/mixed_create_and_update,_cancel_reconcile,_and_continue_reconcile

(for the record, it passed locally when I did make test before I pushed)

@travisn
Copy link
Member

travisn commented Nov 12, 2021

It may be an intermittent test unrelated, I retriggered the tests now to check...

@travisn
Copy link
Member

travisn commented Nov 12, 2021

It may be an intermittent test unrelated, I retriggered the tests now to check...

The unit tests are passing now.

Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

One small request, otherwise LGTM.

pkg/operator/ceph/cluster/osd/spec.go Show resolved Hide resolved
The OSD blkdevmapper init container relies on the MKNOD capability,
which it does not actually request.
As a result, deployments fail on Kubernetes clusters that do not
happen to assign this capability to all containers by default.
Solve this by updating the container spec securityContext to
explicitly request the capability it relies on.

Closes: #9156
Signed-off-by: Omar Pakker <Omar007@users.noreply.github.com>
@leseb leseb requested a review from travisn November 15, 2021 14:19
@travisn travisn merged commit 716baf7 into rook:master Nov 15, 2021
@Omar007 Omar007 deleted the fix/blkdevmapper-capabilities branch November 15, 2021 15:28
mergify bot added a commit that referenced this pull request Nov 15, 2021
osd: set blkdevmapper capabilities (backport #9158)
@y1r y1r mentioned this pull request Dec 16, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OSD prepare job blkdevmapper missing required capabilities
3 participants