[PA-225] Restart PX on Nodes where pds pods have attached volumes #916
Conversation
…d volumes while running load updated the cleanup function to delete load gen pods or deployments
| err = wait.Poll(maxtimeInterval, timeOut, func() (bool, error) { | ||
| ss, err = k8sApps.GetStatefulSet(deployment.GetClusterResourceName(), namespace) | ||
| if err != nil { | ||
| logrus.Warnf("An Error Occured while getting statefulsets %v", err) |
There was a problem hiding this comment.
use new log instance
There was a problem hiding this comment.
what do you mean?
| for _, ds := range params.DataServiceToTest { | ||
| isDeploymentsDeleted = false | ||
| dataServiceDefaultResourceTemplateID, err = pdslib.GetResourceTemplate(tenantID, ds.Name) | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
use dash for the verifications
There was a problem hiding this comment.
what do you mean?
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(dataServiceDefaultAppConfigID).NotTo(BeEmpty()) | ||
|
|
||
| logrus.Infof(" dataServiceDefaultAppConfigID %v ", dataServiceDefaultAppConfigID) |
There was a problem hiding this comment.
use log instance for logging
There was a problem hiding this comment.
what do you mean?
…d volumes while running load updated the cleanup function to delete load gen pods or deployments
| func LabelK8sNode(node *corev1.Node, label string) error { | ||
| keyval := strings.Split(label, "=") | ||
| err := k8sCore.AddLabelOnNode(node.Name, keyval[0], keyval[1]) | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't understand the point of creating this function if it doesn't do anything different than the function you are using inside. Just use the k8sCore.AddLabelOnNode() and remove this func
| func RemoveLabelFromK8sNode(node *corev1.Node, label string) error { | ||
| err := k8sCore.RemoveLabelOnNode(node.Name, label) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Same, here just use the sched-ops func k8sCore.RemoveLabelOnNode(), wrapping it into another func doesn't add any value
| func UnCordonK8sNode(node *corev1.Node) error { | ||
| err = wait.Poll(maxtimeInterval, timeOut, func() (bool, error) { | ||
| err = k8sCore.UnCordonNode(node.Name, timeOut, maxtimeInterval) | ||
| if err != nil { | ||
| logrus.Errorf("Failed uncordon node %v due to %v", node.Name, err) | ||
| return false, nil | ||
| } | ||
| return true, nil | ||
| }) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Same here, why not just use already existing sched-ops func k8sCore.UnCordonNode()? and give it a timeout and interval instead of wrapping it into a whole new func?
| logrus.Errorf("Failed to get pods from node %v due to %v", nodeName, err) | ||
| return false, nil |
There was a problem hiding this comment.
If err is not nil, you need to return it and please remove the logrus.Errorf() and just return the verbose error like so
if err != nil {
return false, fmt.Errorf("Failed to get pods from node [%s], Err %v", nodeName, err)
}
| if err != nil { | ||
| logrus.Errorf("Could not fetch pods running on the given node %v", err) | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Same here, just return and add nodeName
if err != nil {
return false, fmt.Errorf("Could not fetch pods running on the given node [%s], Err: %v", nodeName, err)
}
| if err != nil { | ||
| logrus.Errorf("An Error Occured while getting statefulsets %v", err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Return verbose error, don't print it, we will print it in the initial place where this func is called
| if err != nil { | ||
| logrus.Errorf("An error occured while getting the pods belonging to this statefulset %v", err) | ||
| return nil, err |
There was a problem hiding this comment.
Same, just return verbose error, don't print it, we will print it in the initial place where this func is called
| if err != nil { | ||
| logrus.Errorf("Could not get the node object for node %v because %v", nodeName, err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Same, just return verbose error, don't print it, we will print it in the initial place where this func is called
| if err != nil { | ||
| logrus.Errorf("Could not fetch pods running on the given node %v", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Same, just return verbose error, don't print it, we will print it in the initial place where this func is called
| if err != nil { | ||
| logrus.Errorf("Could not drain the node %v", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Same, just return verbose error, don't print it, we will print it in the initial place where this func is called
What this PR does / why we need it:
Part of the system test automation suite
Which issue(s) this PR fixes (optional)
Closes # PA-225
Special notes for your reviewer: