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

Add an E2E test for NodeConfig mount degraded condition propagation for mounts over corrupted xfs filesystems #2178

Conversation

rzetelskik
Copy link
Member

@rzetelskik rzetelskik commented Nov 4, 2024

Description of your changes: This PR adds an e2e test verifying that NodeConfig ends up with a degraded status condition when trying to mount over a corrupted xfs filesystem. The behaviour itself, as requested by #1158, was already there since merging #2134.

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

/priority important-longterm

@scylla-operator-bot scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2024
@rzetelskik rzetelskik force-pushed the xfs-corrupted-mount-propagation branch from a6d93ea to 9e48f16 Compare November 4, 2024 17:18
@scylla-operator-bot scylla-operator-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2024
@rzetelskik rzetelskik force-pushed the xfs-corrupted-mount-propagation branch from 9e48f16 to 89ea299 Compare November 4, 2024 17:42
@scylla-operator-bot scylla-operator-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2024
@rzetelskik rzetelskik added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 5, 2024
@rzetelskik rzetelskik changed the title [WIP] Add an E2E test for NodeConfig mount degraded condition propagation for mounts over corrupted xfs filesystems Add an E2E test for NodeConfig mount degraded condition propagation for mounts over corrupted xfs filesystems Nov 5, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2024
@rzetelskik
Copy link
Member Author

@zimnx @tnozicka ptal

Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

I think this could be done simpler by creating a broken XFS initially using a pod in the preNodeConfigCreationFunc and reusing more of the existing code (avoids the extra rollout wait and patch) but I don't feel strong

/approve
/assign @zimnx

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2024
@rzetelskik
Copy link
Member Author

I think this could be done simpler by creating a broken XFS initially using a pod in the preNodeConfigCreationFunc and reusing more of the existing code (avoids the extra rollout wait and patch) but I don't feel strong

Depends on what you consider simpler - I started by trying to do it this way but at some point I realised the client pod started to turn into the nodeconfig controller because it had to perform the exact same steps to set it up. I think this way is clearer in terms of highlighting the broken setup. In terms of the test entry complexity it's admittedly worse - perhaps the test table could use a rehaul.

Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot merged commit 0963a6e into scylladb:master Nov 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure xfs mount that won't mount because it needs repair is manifested on NodeConfig
3 participants