Skip to content
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 replaceUnready annotation for Jobs #1575

Merged
merged 4 commits into from
May 13, 2021
Merged

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented May 13, 2021

Proposed changes

Add the pulumi.com/replaceUnready annotation that allows
the user to specify that a Job resource should be replaced
during an update if it is not ready. This annotation could be
extended to support other resource types as well, but currently
only supports Jobs.

Related issues (optional)

Fix #856

Add the `pulumi.com/replaceUnready` annotation that allows
the user to specify that a Job resource should be replaced
during an update if it is not ready. This annotation could be
extended to support other resource types as well, but currently
only supports Jobs.
@lblackstone
Copy link
Member Author

lblackstone commented May 13, 2021

Based on feedback from #1563, I decided to add an annotation to force replacement of a resource if it is also unready. We may also want to add the replaceOnUpdate annotation, but that can be handled separately.

Here's what this looks like in practice:

pulumi.com/replaceUnready: "true"
Screen Shot 2021-05-13 at 11 25 08 AM

pulumi.com/replaceUnready: "false"
Screen Shot 2021-05-13 at 11 25 45 AM

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@lblackstone lblackstone marked this pull request as ready for review May 13, 2021 19:05
@viveklak
Copy link
Contributor

Do we have a way to add a test for this?

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@pulumi pulumi deleted a comment from github-actions bot May 13, 2021
@lblackstone lblackstone merged commit de129e3 into master May 13, 2021
@pulumi-bot pulumi-bot deleted the lblackstone/replaceUnready branch May 13, 2021 21:31
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to fix this by handling it as part of the existing support for updating resources that have initialization failures?

I’m a little more nervous about the implications of handling this in Diff. Is there precedent for that, and have we validated the experience this results in?

case "Job":
jobChecker := states.NewJobChecker()
job, err := clients.FromUnstructured(newInputs)
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that the error is swallowed here? Shouldn't it be returned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this error would only occur from a malformed Job object and is unrelated to the diff, I thought it made sense to ignore. I can add a comment to that effect.

jobChecker := states.NewJobChecker()
job, err := clients.FromUnstructured(newInputs)
if err == nil {
jobChecker.Update(job)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to do this wait operation from Diff? Do we normally do that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not actually running the full await logic; this is a point in time check based on the current status of the Job.

I'm not sure if there's precedent for that, but it shouldn't have a noticeable effect on performance, and Diff is the only place where we can indicate a replacement is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating failed job should replace job instead of refreshing status
3 participants