-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Add a new neverTerminate job behavior just for upgrade #122643
Conversation
/sig apps |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh 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 |
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
// this job is being used in an upgrade job see test/e2e/upgrades/apps/job.go | ||
// it should never be optimized, as it always has to restart during an upgrade | ||
// and continue running | ||
job.Spec.Template.Spec.Containers[0].Command = []string{"sleep", "1000000"} |
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.
non-blocking nit: we could have TerminationGracePeriodSeconds: 1
to make it faster :)
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.
Great idea, updated.
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.
You probably need = ptr.To[int64](1)
LGTM label has been added. Git tree hash: 79190a25a9c2cc7d4dd3c3c30baf96ff244c595a
|
60d19f3
to
e6083d4
Compare
e6083d4
to
f8abe71
Compare
@@ -70,7 +73,7 @@ func (t *JobUpgradeTest) Teardown(ctx context.Context, f *framework.Framework) { | |||
// rely on the namespace deletion to clean up everything | |||
} | |||
|
|||
// ensureAllJobPodsRunning uses c to check in the Job named jobName in ns | |||
// ensureAllJobPodsRunning uses c to check if the Job named jobName in ns |
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 know this is an old function, but I'm wondering how do we know the pods are recreated already (and running)? I'm wondering if we are missing a wait here, but even if so this is a separate issue.
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.
This is being already handled at the upgrade framework level (see here: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/upgrades/upgrade_suite.go). These jobs here are implementing upgrades.Test
interface, so you only focus on the Setup
(create the job and make sure it's running) and Test
(check the job after the upgrade) steps.
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
LGTM label has been added. Git tree hash: 8c04d42974a6f18fd47dbef03a5ce31f4cfad7bf
|
What type of PR is this?
/kind failing-test
/kind regression
What this PR does / why we need it:
In #121538 the command for
notTerminate
job behavior was modified, from using asleep 1000000
to a pause image. The problem is that this behavior is also used in the upgrade job, to ensure the job continues to run during an upgrade, especially when nodes are being rolled over, ie. terminated and the job controller has to keep the job running for the entire time.Since the goal of that other PR was to optimize executions, I'm adding a new
neverTerminate
behavior which will be used only during upgrade (with a comment explaining why we need it that way).Special notes for your reviewer:
/assign @mimowo @sairameshv
Does this PR introduce a user-facing change?