-
Notifications
You must be signed in to change notification settings - Fork 33
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-32580: Do not create pre-flight storage node objects for nodes wh… #1222
PWX-32580: Do not create pre-flight storage node objects for nodes wh… #1222
Conversation
…ich have label px/enabled=false. Signed-off-by: Jose Rivera <jose@portworx.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1222 +/- ##
==========================================
+ Coverage 75.77% 75.82% +0.04%
==========================================
Files 65 65
Lines 18410 18422 +12
==========================================
+ Hits 13951 13968 +17
+ Misses 3464 3455 -9
- Partials 995 999 +4
☔ View full report in Codecov by Sentry. |
…e-flight storage node based on this api check. Signed-off-by: Jose Rivera <jose@portworx.com>
Latest runs on OCP worked
...
|
shouldRun, _, err := c.nodeShouldRunStoragePod(&node, toUpdate) | ||
if err == nil { | ||
if shouldRun { | ||
logrus.Infof("Create pre-flight storage node entry for node: %s", node.Name) | ||
c.createStorageNode(toUpdate, node.Name) | ||
} else { | ||
logrus.Infof("Skipping pre-flight storage node entry for node: %s", node.Name) |
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.
Please add UTs to cover the diff here.
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.
@jrivera-px does this also take care of infra
nodes in OCP scenario?
… unit test. Signed-off-by: Jose Rivera <jose@portworx.com>
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.
Thanks for adding the UTs!
for _, node := range k8sNodeList.Items { | ||
shouldRun, _, err := c.nodeShouldRunStoragePod(&node, toUpdate) | ||
if err != nil { | ||
logrus.Infof("Skipping pre-flight storage node entry for %s node. Error: %v", node.Name, err) |
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.
The Infof
should be an warnf instead as this is not expected. Not error since we aren't failing the operator due to this.
Signed-off-by: Jose Rivera <jose@portworx.com>
@nrevanna sorry didn't answer this "does this also take care of infra nodes in OCP scenario?".... Yes it does. |
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
#1222) * PWX-32580: Do not create pre-flight storage node objects for nodes which have label px/enabled=false. Signed-off-by: Jose Rivera <jose@portworx.com> * PWX-32580: Use correct api to check if a node will run px. Create pre-flight storage node based on this api check. Signed-off-by: Jose Rivera <jose@portworx.com> * PWX-32580: break out pre-flight storage node create/delete funcs. Add unit test. Signed-off-by: Jose Rivera <jose@portworx.com> * PWX-32580: PR suggestion change infof to warnf on skips. Signed-off-by: Jose Rivera <jose@portworx.com> --------- Signed-off-by: Jose Rivera <jose@portworx.com>
#1222) (#1233) * PWX-32580: Do not create pre-flight storage node objects for nodes which have label px/enabled=false. * PWX-32580: Use correct api to check if a node will run px. Create pre-flight storage node based on this api check. * PWX-32580: break out pre-flight storage node create/delete funcs. Add unit test. * PWX-32580: PR suggestion change infof to warnf on skips. --------- Signed-off-by: Jose Rivera <jose@portworx.com>
…ich have label px/enabled=false.
What this PR does / why we need it:
If pre-flight storage node exists it should have status results provided by the pre-flight pod run. If not we will assume pre-flight failed to run on that node and we will fail pre-flight. We create storage nodes objects for all nodes except for master. However when label px/enabled=false is supplied on a node, pre-flight will not run on this node. This resulted in that node not having any results and we fail pre-flight. So don't create storage node object for nodes with PX disabled label."
Which issue(s) this PR fixes (optional)
Closes #
PWX-32580
Special notes for your reviewer: