Bug 1829072: Fix all of the k8s_event module instances. #1203
Conversation
openshift-ci-robot
added
approved
|
@timflannagan1: This pull request references Bugzilla bug 1829072, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
Still need to verify some edge cases |
openshift-ci-robot
added
the
do-not-merge/hold
timflannagan
force-pushed
the
fix-events-module
branch
from
c095667
to
14e2ae5
Compare
There was a problem hiding this comment.
It looks like the only thing that changed was the component to metering-ansible-operator. This is alot of change in many files when only 1 line in each needed to change. How about just changing component? Is that an option here?
I see reportingComponent key is not longer required, but this is a subtraction of 1 line, wondering why the repositioning in each file is needed rather than 2 line alterations?
images/metering-ansible-operator/roles/meteringconfig/tasks/configure_storage.yml
Show resolved
Hide resolved
|
@EmilyM1 I think you're missing the point of this change - It's possible I need to be more explicit in the PR's description. The problem stems from all of the instances of the |
timflannagan
changed the title
timflannagan
changed the title
timflannagan
force-pushed
the
fix-events-module
branch
2 times, most recently
from
47181a2
to
23f6a5b
Compare
|
Oh I know see the name changes as well! I checked the k8 object name link. It does make sense. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilyM1, timflannagan1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
images/metering-ansible-operator/roles/meteringconfig/tasks/validate.yml
Show resolved
Hide resolved
timflannagan
force-pushed
the
fix-events-module
branch
2 times, most recently
from
03274a3
to
bd3dcf9
Compare
Previously, all instances of the `k8s_event` module were using an invalid `k8s_event.name` configuration. This produced an error like the following:
```yaml
TASK [meteringconfig : Log Events for validating configurations] ***************
task path: /opt/ansible/roles/meteringconfig/tasks/validate.yml:91
Tuesday 28 April 2020 18:09:29 +0000 (0:00:00.365) 0:00:14.305 *********
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: MaxRetryError: HTTPConnectionPool(host='localhost', port=8888): Max retries exceeded with url: /api/v1/namespaces/openshift-metering/events/Validate Configuration (Caused by ProtocolError('Connection aborted.', InvalidURL("URL can't contain control characters. '/api/v1/namespaces/openshift-metering/events/Validate Configuration' (found at least ' ')",)))
fatal: [localhost]: FAILED! => {"changed": false, "msg": "HTTPConnectionPool(host='localhost', port=8888): Max retries exceeded with url: /api/v1/namespaces/openshift-metering/events/Validate Configuration (Caused by ProtocolError('Connection aborted.', InvalidURL(\"URL can't contain control characters. '/api/v1/namespaces/openshift-metering/events/Validate Configuration'
```
This `k8s_event.name` field directly corresponds to the [traditional `metadata.name`](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) for a k8s resource, which is how kubernetes refers to an object in a resource URL. This error is produced as the previous configuration(s) had invalid control characters being passed to the `k8s_event` module implementation.
On top of this, I changed the `k8s_event.reportingComponent` to be an optional field as it's not directly applicable to our use case in the metering-ansible-operator, and many of the core events often don't utilize this field.
Other changes that were made:
- Substituting the `source.component` to be the metering-ansible-operator, as "Metering Component" doesn't provide much value and it's difficult to interpret from that source what controller is responsible for firing off this event.
- Reorganizing the `k8s_event.reason`, `k8s_event.message`, and `k8s_event.state` to better reflect the current state of the metering-ansible-operator reconciliation effort.
- In the case of an error, i.e. a user-provided configuration was invalid, only the status field of the `MeteringConfig` custom resource was updated, and no event was fired off warning a user that they might need to take manually intervene.
timflannagan
force-pushed
the
fix-events-module
branch
from
bd3dcf9
to
d846095
Compare
|
/test all |
|
/lgtm |
|
We should probably file a BZ for the many-to-many errors when trying to query Prometheus for the pvc-related queries despite its reproducibility. |
|
@timflannagan1: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:
In response to this:
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. |
Previously, all instances of the
k8s_eventmodule were using an invalidk8s_event.nameconfiguration. This produced an error like the following:This
k8s_event.namefield directly corresponds to the traditionalmetadata.namefor a k8s resource, which is how kubernetes refers to an object in a resource URL. This error is produced as the previous configuration(s) had invalid control characters being passed to thek8s_eventmodule implementation.On top of this, I changed the
k8s_event.reportingComponentto be an optional field as it's not directly applicable to our use case in the metering-ansible-operator, and many of the core events often don't utilize this field.Other changes that were made:
source.componentto be the metering-ansible-operator, as "Metering Component" doesn't provide much value and it's difficult to interpret from that source what controller is responsible for firing off this event.k8s_event.reason,k8s_event.message, andk8s_event.stateto better reflect the current state of the metering-ansible-operator reconciliation effort.MeteringConfigcustom resource was updated, and no event was fired off warning a user that they might need to take manually intervene.