-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] [Attestation-Manager] Adding drain node feature #49
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sholapur, Prashant <[email protected]>
// } | ||
config, err := rest.InClusterConfig() | ||
if err != nil { | ||
logging.Error("Error building in-cluster config: %s", err.Error()) |
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.
return false due to error? also after line 46 also
} else { | ||
return true | ||
} | ||
|
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.
delete empty line
} | ||
logging.Debug("Node cordoned successfully") | ||
// Sleep for a while before the next attestation check | ||
logging.Debug("Sleeping for 1 minutes and calling drainNode()") |
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.
why is there a need to sleep for 1 minute before draining the node? seems arbitrary
} | ||
|
||
func cordonNode(clientset *kubernetes.Clientset, nodeName string) error { | ||
node, err := clientset.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) |
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.
will these context.TODO() be replaced with other more appropriate context to handle cancellations, timeouts, and data passing, etc? It's recommended to never use context.TODO() in production code
Merge Checklist
All boxes should be checked before merging the PR
Description
Fixes # (issue)
Any Newly Introduced Dependencies
How Has This Been Tested?