NETOBSERV-1203: Added option to add zones in k8s transform rule #575
Conversation
|
@OlivierCazade: This pull request references NETOBSERV-1203 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
OlivierCazade
force-pushed
the
1203-zones
branch
from
fe39e14
to
795778b
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
- Coverage 65.80% 65.54% -0.26%
==========================================
Files 102 102
Lines 7445 7483 +38
==========================================
+ Hits 4899 4905 +6
- Misses 2256 2288 +32
Partials 290 290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -148,6 +148,7 @@ func fillInK8s(outputEntry config.GenericMap, rule api.NetworkTransformRule) { | |||
| outputEntry[rule.Output+"_HostName"] = kubeInfo.HostName | |||
| } | |||
| } | |||
| fillInK8sZone(outputEntry, rule, *kubeInfo) | |||
There was a problem hiding this comment.
We should also manage the open telemetry case in the else statement below this line.
- In case of a node we can add
k8s.node.zone - Other cases
k8s.host.zone
Maybe filling the zones directly inside fetchInformers function in kubernetes.go file would actually be better ?
You can pass the rule to it and query the info accordingly.
Then in transform_network.go you conditionally fill the output as same as what's done for HostIP and HostName
WDYT ?
There was a problem hiding this comment.
I thought about adding this in fetchInformers, but this mean querying the informer for nodes for each flow even when the feature is disabled.
I prefered keeping it separated for performances reasons.
There was a problem hiding this comment.
About telemetry this is now supported with a uniq tag: k8s.zone
| @@ -184,6 +185,40 @@ func fillInK8s(outputEntry config.GenericMap, rule api.NetworkTransformRule) { | |||
| } | |||
| } | |||
|
|
|||
| const nodeZoneLabelName = "topology.kubernetes.io/zone" | |||
There was a problem hiding this comment.
Would this be configurable ? I see topology.kubernetes.io/region & topology.kubernetes.io/zone and I wonder if some clusters have only region for example 🤔
https://kubernetes.io/docs/reference/labels-annotations-taints/#topologykubernetesioregion
There was a problem hiding this comment.
Zone and region are not exactly the same thing, generally a region has multiple zones.
In your example us-east-1 vs us-east-1c
There was a problem hiding this comment.
so are we sure we always have zones when we have region ?
There was a problem hiding this comment.
A region represents a larger domain, made up of one or more zones.
If I understand correctly there is at least one zone for each region. And since they are not exactly the same thing it would be incorrect to tag the region instead of the zone anyway.
If needed we can add a second feature to also tag the region, but I think we should not mix both.
There was a problem hiding this comment.
Ok sounds legit. Thanks !
| case kubernetes.TypeNode: | ||
| zone, ok := kubeInfo.Labels[nodeZoneLabelName] | ||
| if ok { | ||
| outputEntry[rule.Output+"_Zone"] = zone |
There was a problem hiding this comment.
Since this is node related, I would suggest HostZone for consistency with HostIP & HostName
| outputEntry[rule.Output+"_Zone"] = zone | |
| outputEntry[rule.Output+"_HostZone"] = zone |
There was a problem hiding this comment.
IMO this is not host related the same way. HostIP and HostName belongs to the host, the pod doest not have this IP or name.
But the container is in the zone, we juste get the information through the node.
| if nodeInfo != nil { | ||
| zone, ok := nodeInfo.Labels[nodeZoneLabelName] | ||
| if ok { | ||
| outputEntry[rule.Output+"_Zone"] = zone |
There was a problem hiding this comment.
Same as previous comm here
|
As discussed in scrum today, this will be tested post merge. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade 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 |
Description
Zones are added to the flows if the rule is configured with the new addZone bool.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.