-
Notifications
You must be signed in to change notification settings - Fork 89
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
PWX-30276: Do not delete pods running on storage down nodes. #1385
Conversation
Can one of the admins verify this patch? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1385 +/- ##
==========================================
+ Coverage 67.76% 67.82% +0.06%
==========================================
Files 42 42
Lines 4889 4889
==========================================
+ Hits 3313 3316 +3
+ Misses 1242 1240 -2
+ Partials 334 333 -1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good.
Some nitpicks.
One comment in monitor.go needs to be addressed before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. just one nit that you missed addressing. ok to merge after CBTs pass.
Thanks @pp511 ! Will fix the missed comment. Currently trying to get a successful integration test run with the newly added tests. Will merge it once I get a success. |
7f6c49d
to
f142f33
Compare
- Introduce a new state in Stork for storage down nodes. - Remap portworx StorageDown state to Stork's StorageDown state - For degraded node state: - filter out the nodes from extender requests. - monitor should delete the pods running on them - For storage down state: - do not filter out such nodes. - give a low score to nodes which are storage down. - monitor should not delete pods running on them Signed-off-by: Aditya Dani <aditya@portworx.com>
require.NotEqual(t, poolMaintenanceNode.Name, scheduledNodes[0].Name, "Pod should not be scheduled on node in PoolMaintenance state") | ||
|
||
err = volumeDriver.ExitPoolMaintenance(poolMaintenanceNode) | ||
require.NoError(t, err, "Error starting driver on Node %+v", scheduledNodes[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit..Error exiting pool maintenance
verifyScheduledNode(t, scheduledNodes[0], volumeNames) | ||
|
||
err = volumeDriver.EnterPoolMaintenance(scheduledNodes[0]) | ||
require.NoError(t, err, "Error stopping driver on scheduled Node %+v", scheduledNodes[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit..Error stopping driver -> Error entering pool maintenance
Integration tests look good. Just two nits that need to be addressed |
- Added integration tests for extender and health-monitor. - Simulating a StorageDown state by using Portworx driver's Pool Maintenance operation.
What type of PR is this?
What this PR does / why we need it:
Does this PR change a user-facing CRD or CLI?:
No
Is a release note needed?:
Does this change need to be cherry-picked to a release branch?:
Yes - 23.4
Testing Notes