UPSTREAM: Do not mutate pods on update that are scheduled #15190
Conversation
smarterclayton
force-pushed
the
fail_mutation
branch
from
187bc09 to
1fc5fb1
Compare
| @@ -53,7 +55,11 @@ func (a *alwaysPullImages) Admit(attributes admission.Attributes) (err error) { | |||
| if !ok { | |||
| return apierrors.NewBadRequest("Resource was marked with kind Pod but was unable to be converted") | |||
| } | |||
|
|
|||
| // already scheduled pods cannot be mutated | |||
| if attributes.GetOperation() == admission.Update && len(pod.Spec.NodeName) != 0 { | |||
There was a problem hiding this comment.
I don't know why this plugin is looking at update at all... ImagePullPolicy is never mutable
There was a problem hiding this comment.
I'll remove it.
smarterclayton
force-pushed
the
fail_mutation
branch
3 times, most recently
from
d6e21cc to
c9a6d22
Compare
| @@ -75,6 +77,12 @@ func (p *plugin) Admit(attributes admission.Attributes) (err error) { | |||
| return errors.NewBadRequest(fmt.Sprintf("expected *api.Pod but got %T", attributes.GetObject())) | |||
| } | |||
|
|
|||
| // already scheduled pods cannot be mutated | |||
| if attributes.GetOperation() == admission.Update { | |||
There was a problem hiding this comment.
why not just remove Update from Handler: admission.NewHandler(admission.Create, admission.Update)?
There was a problem hiding this comment.
Tolerations are mutable post create
There was a problem hiding this comment.
crap, they're only addable.... GRRRRRRRRRRR
They fail validation
smarterclayton
force-pushed
the
fail_mutation
branch
from
c9a6d22 to
43e60df
Compare
|
Evaluated for origin test up to 43e60df |
|
Stripped down to the minimal change to prevent alwayspullimages for 3.6, and will deal with defaulttolerationseconds upstream and later. |
|
LGTM |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3183/) (Base Commit: e1d18a3) (PR Branch Commit: 43e60df) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 1 |
|
Evaluated for origin merge up to 43e60df |
That is an illegal operation in validation, which means these cannot be turned on after a cluster is already running.
[test] @deads2k
Fixes #15188
Will update commit with correct upstream shortly