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
Convert TaintBasedEvictions e2e to integration test #81856
Conversation
/sig scheduling |
/retest |
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, @damemi . Some comments below, and for reviewers' conveniences please make changes in a new commit, we can squash later.
/assign |
515b26a
to
e767790
Compare
@Huang-Wei with master reopened now, do you have a chance to look at this again? If it looks good I can squash down |
TerminationGracePeriodSeconds: &gracePeriod, | ||
}, | ||
} | ||
tolerationSeconds := []int64{200, 300, 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.
How about defining tolerationSeconds
in each test struct like
{
name: "Taint based evictions for NodeNotReady and 200 tolerationseconds",
nodeTaints: []v1.Taint{{Key: schedulerapi.TaintNodeNotReady, Effect: v1.TaintEffectNoExecute}},
nodeConditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}},
pod: testPod,
waitForPodCondition: "updated with tolerationSeconds of 200",
tolerationSeconds: 200,
for the readability?
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.
TolerationSeconds needs to be a pointer value https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2814
So, we need to declare it in a variable somewhere (can't just do a pointer to an int literal) so we decided this was the way to do it (see #81856 (comment))
/cc @oomichi |
e767790
to
c74512e
Compare
@Huang-Wei pushed your suggestions in a new commit. If this looks good let me know and I will squash |
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 updating,
/approve
) | ||
if err != nil { | ||
t.Errorf("Failed to create node controller: %v", err) | ||
return |
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: This return
might be unnecessary.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damemi, oomichi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
09f8a51
to
7e1794d
Compare
Since this has an /approve, I'm going to squash |
/lgtm Thanks @damemi for the efforts! |
/retest |
What type of PR is this?
What this PR does / why we need it:
Rebases and updates the changes from #80817 with the feedback provided in that PR
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: