We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

CLAs look good, thanks!

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

Labelling this PR as size/XS

cc @nikhiljindal

LGTM. Thanks for submitting the fix!

@k8s-bot ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

GCE e2e build/test passed for commit d27a30d.

• Build Log
• Test Artifacts
• Internal Jenkins Results

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e build/test passed for commit d27a30d.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e build/test passed for commit d27a30d.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e build/test passed for commit d27a30d.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from submit-queue

@jlowdermilk @Kargakis @ironcladlou: Are you ok with this change?

This check was actually added specifically to disable :latest for rollingupdate. See #15908 (comment) for context.

I would be ok with supporting :latest as a special case, but that means we need additional handling. Specifically, if the tag is :latest we should require deployment-label-key and check that the imagePullPolicy is Always, otherwise the update will not do anything.

To reiterate, we disabled :latest on purpose. If we want to supporting it, we need special handling.

@jlowdermilk @Kargakis @ironcladlou: Are you ok with this change?

It only affects kubectl rolling-update and not the rolling update logic so it's up to @jlowdermilk

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@jlowdermilk Should we revert this PR, if we believe it doesn't work?

My vote is for reverting and re-adding with proper support for the :latest tag outlined above. It's a commonly-used case, but it is broken without additional configuration that we currently don't check for.

@jlowdermilk also noted, if there is no tag, it should be considerred as latest.

I'll take a look to see what's left, as you said, check the deployment-label-key and imagePullPolicy.

That's true, the imagePullPolicy could only be Always.

@janetkuo Please revert this.

@janetkuo @zhouhaibing089 If we're able to fix it tomorrow, I'm also fine with that.

@bgrant0607 I'll take some time to look at it. :)

@bgrant0607 @jlowdermilk after a little study, I think it is good to generate a warning when the file hash is the same, but for the rolling update, I would suggest using another value, timestamp may be a consideration.. even the file is the same, rolling update should still proceed on just as before.

@zhouhaibing089, the hash is generated not for the file, but for the full api object as returned from the server. This includes the resourceVersion field, which is guaranteed to change when the object is updated (see objectMeta docs).

I mentioned this can be resolved by requiring user to specify a deployment-label-key. That is less convenient tho, so I suggest is that if the hash is the same (i.e. because we are using image:latest), you just append -new to the new rc's deployment-label value. That should satisfy requirement for deployment label uniqueness.

@jlowdermilk if i understand correctly, we do not update it, just get the old rc, and replace the image field, and create a new.. so when we replacing the image field, we have to replace the deployment-label-key, too, and the default value for deployment-label-key is the hash for the full api object(sorry that I think it is the file, :|), that's the problem, if the deployment-label-key is the same, we are not able to create that new RC, right?

@zhouhaibing089, correct, the issue is that value of deployment label is not unique if they hash is different. The label must be distinct otherwise the new rc and old rc will fight over pods, so we enforce that distinctness with a check in the command. As I suggested above, you can resolve this by adding -new (or any other suffix) to the hash when you create the new rc.