Emit events for deployment hooks #9456
Conversation
|
@Kargakis PTAL (not tested yet) |
| @@ -70,8 +77,14 @@ func (e *HookExecutor) Execute(hook *deployapi.LifecycleHook, deployment *kapi.R | |||
| var err error | |||
| switch { | |||
| case len(hook.TagImages) > 0: | |||
| tagEventMessages := []string{} | |||
| for _, t := range hook.TagImages { | |||
| tagEventMessages = append(tagEventMessages, fmt.Sprintf("container %s image to %s", t.ContainerName, t.To.Name)) | |||
There was a problem hiding this comment.
having a container image name here will make these images better :-)
mfojtik
force-pushed
the
hook-events
branch
5 times, most recently
from
4c6a1c4 to
d534d1a
Compare
| @@ -399,6 +399,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole { | |||
| authorizationapi.NewRule("get", "list", "update").Groups(kapiGroup).Resources("replicationcontrollers").RuleOrDie(), | |||
| authorizationapi.NewRule("get", "list", "watch", "create").Groups(kapiGroup).Resources("pods").RuleOrDie(), | |||
| authorizationapi.NewRule("get").Groups(kapiGroup).Resources("pods/log").RuleOrDie(), | |||
| authorizationapi.NewRule("create").Groups(kapiGroup).Resources("events").RuleOrDie(), | |||
There was a problem hiding this comment.
@liggitt policy PTAL (the client running inside the deployer pod needs ability to create events to report hooks/etc.).
There was a problem hiding this comment.
You probably need to run UPDATE_BOOTSTRAP_POLICY_FIXTURE_DATA=true make test-unit WHAT=pkg/cmd/server/bootstrappolicy and save the diff.
|
@Kargakis ready for review :-) |
| @@ -131,10 +131,10 @@ func NewDeployer(client kclient.Interface, oclient client.Interface, out, errOut | |||
| strategyFor: func(config *deployapi.DeploymentConfig) (strategy.DeploymentStrategy, error) { | |||
| switch config.Spec.Strategy.Type { | |||
| case deployapi.DeploymentStrategyTypeRecreate: | |||
| return recreate.NewRecreateDeploymentStrategy(client, oclient, kapi.Codecs.UniversalDecoder(), out, errOut, until), nil | |||
| return recreate.NewRecreateDeploymentStrategy(client, oclient, client.Events(""), kapi.Codecs.UniversalDecoder(), out, errOut, until), nil | |||
There was a problem hiding this comment.
Why do you have to pass the recorder as a separate arg? Can't you pick it while in the function from client?
There was a problem hiding this comment.
@Kargakis no, events has different interface (record.EventSink)
| fmt.Fprintf(e.out, "warning: %s hook failed, deployment will continue: %v\n", label, err) | ||
| e.recorder.Eventf(config, kapi.EventTypeNormal, "Failed", "The %s hook failed: %v (ignore), deployment %s/%s will continue", label, err, deployment.Namespace, deployment.Name) | ||
| // TODO: The event might not be send to server, remove the sleep when kubernetes(#27858) is resolved | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
@Kargakis will keep this here (kubernetes/kubernetes#27858) so we can see the event logs and will fix this as soon as the upstream issue is fixed.
There was a problem hiding this comment.
@Kargakis i can follow upstream pattern and make the sleep random :-)
There was a problem hiding this comment.
@Kargakis you can stop crying now
mfojtik
force-pushed
the
hook-events
branch
3 times, most recently
from
87f9d00 to
a9e5f08
Compare
| @@ -64,14 +70,49 @@ func NewHookExecutor(client kclient.PodsNamespacer, tags client.ImageStreamTagsN | |||
| } | |||
| } | |||
|
|
|||
| func (e *HookExecutor) event(deployment *kapi.ReplicationController, eventType, reason, msg string) { | |||
mfojtik
force-pushed
the
hook-events
branch
2 times, most recently
from
b44fae8 to
959fc1c
Compare
|
@Kargakis updated, the event is now using DC and fallback to RC in case we are not able to decode RC to DC. |
| case deployapi.LifecycleHookFailurePolicyIgnore: | ||
| fmt.Fprintf(e.out, "warning: %s hook failed, deployment will continue: %v\n", label, err) | ||
| e.emitEvent(deployment, kapi.EventTypeWarning, "Failed", fmt.Sprintf("The %s hook failed: %v (ignore), deployment %s/%s will continue", label, err, deployment.Namespace, deployment.Name)) |
There was a problem hiding this comment.
Make these messages consistent with the above (either "pre-hook" or "pre hook")
|
@Kargakis fixed, tested. |
|
@mfojtik I cannot find an event for a successful hook. Don't we want those events too? Otherwise, someone may think their hooks are running forever just by looking at the description of the config. |
|
@Kargakis added. |
|
LGTM [merge] |
|
@mfojtik a follow-up issue for trimming down events in |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 8a112b6 |
|
@Kargakis there is DescribeConfig (or something like that) that allows you to trim or toggle events displaying (it came with the rebase), I will check if it is something we can use to shave the event list here. And yes, will open a follow up. |
|
continuous-integration/openshift-jenkins/test ABORTED (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5493/) |
|
[merge] |
|
#9512, [merge] |
|
#9638, [merge] |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5607/) (Image: devenv-rhel7_4494) |
|
TestClusterQuotaFuzzer flake (known).... [merge] |
|
Evaluated for origin merge up to 8a112b6 |
Fixes: #9260