Don't force ig image change on cluster upgrade if it is custom. #3232
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @KashifSaadat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/assign @chrislovecnm |
|
/ok-to-test This seems like a good change. Do you think we could In general I think we need to figure out how to do upgrades for people using different images... which image type are you using, out of interest? |
k8s-ci-robot
removed
the
needs-ok-to-test
|
Yeah of course, more logging is always good! :) I'll add this in. (@gambol99 FYI) |
KashifSaadat
force-pushed
the
cluster-upgrade-persist-custom-image
branch
from
b728517
to
7654f86
Compare
|
I logged it as an info. Do you think it should specifically be a warning, in case of potential compatibility issues with an unsupported image? |
cmd/kops/upgrade_cluster.go
Outdated
| }) | ||
| } | ||
| } else { | ||
| glog.Infof("Custom image (%s) has been provided for Instance Group %q; not prompting for image upgrade", ig.Spec.Image, ig.GetName()) |
There was a problem hiding this comment.
nit pick on the message, and it should be a Infof, as user is not doing something odd.
not prompting
Is kops rather not updating the image automatically since the user is not using the kope.io image? You have it as not prompting, which to me says CLI interaction.
Like I mentioned nit pick on english, and I am happy to get this in either way.
There was a problem hiding this comment.
Good spot, I'll just correct that now! 👍
KashifSaadat
force-pushed
the
cluster-upgrade-persist-custom-image
branch
from
7654f86
to
6a3df8f
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KashifSaadat, chrislovecnm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
|
@justinsb we are using a centos baseline, which includes all of our standard monitoring, ids, change detection, and security baseline changes. We build our baselines weekly, so in our case we have additionally had to automate finding the latest approved Ami and editing the cluster to use it. Since kops edit is manual, we worked around it by manipulating the specs in the s3 bucket manually |
Fixes #3160