-
Notifications
You must be signed in to change notification settings - Fork 983
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
make node names unique in tests #1598
Conversation
- make node names unique with a sequential ID - if the node already exists, log a debug message
✅ Deploy Preview for karpenter-docs-prod canceled.
|
@@ -158,7 +158,9 @@ func (p *Provisioner) launch(ctx context.Context, node *scheduling.Node) error { | |||
// ourselves to enforce the binding decision and enable images to be pulled | |||
// before the node is fully Ready. | |||
if _, err := p.coreV1Client.Nodes().Create(ctx, k8sNode, metav1.CreateOptions{}); err != nil { | |||
if !errors.IsAlreadyExists(err) { | |||
if errors.IsAlreadyExists(err) { | |||
logging.FromContext(ctx).Debugf("node %s already registered", k8sNode.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.
Added this log as it would have saved me a few hours this morning. In a test if you somehow happen to get a duplicate node name, there are no errors reported since it could happen normally if kubelet comes up. The end result is that your test doesn't fail with some "node already exists" error and if it does fail it will look like something is really wrong. In my case, a zonal topology spread was trying to schedule a node in "test-zone-2", the node already existed in "test-zone-1", no error occurred but the test for zone skew failed.
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.
I would guess the comment is correct that this is rare, but did you check this in a real cluster to make sure that debug doesn't spam in the common case?
otherwise, lgtm
Yup, nothing normally logs in a real cluster in my testing, I'm not sure how often kubelet would beat us to the node creation, but it doesn't seem likely:
If it does log, it will be Debug only. |
1. Issue, if available:
N/A
2. Description of changes:
3. How was this change tested?
Unit tests & scaling up/down inflate on EKS.
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.