New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDN-4434: Network diagnostics scheduling #2339
SDN-4434: Network diagnostics scheduling #2339
Conversation
@kyrtapz: This pull request references SDN-4434 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. |
9c4c305
to
ba6f97f
Compare
/retest |
/hold |
ba6f97f
to
8fe5415
Compare
/retest |
@@ -131,6 +131,27 @@ func add(mgr manager.Manager, r *ReconcileOperConfig) error { | |||
return err | |||
} | |||
|
|||
// Watch for changes to networkDiagnostics in network.config | |||
err = c.Watch(source.Kind(mgr.GetCache(), &configv1.Network{}), &handler.EnqueueRequestForObject{}, predicate.Funcs{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come we dont follow this pattern of adding a watcher for other related config changes? at least, i dont see other config options in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller doesn't read other config fields from configv1.Network
.
if err != nil { | ||
return nil, progressing, err | ||
} | ||
objs = append(objs, o...) | ||
|
||
// render network diagnostics | ||
o, err = renderNetworkDiagnostics(conf, manifestDir) | ||
o, err = renderNetworkDiagnostics(operConf, clusterConf, manifestDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need the cluster conf here? I thought our pattern was to get config, update oper, then render with oper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not follow that pattern here and the networkDiagnostics
is configured through clusterConf only so we don't need to copy it over. operConf has the legacy disableNetworkDiagnostics
field which we fallback to(backwards compatibility) and the plan is to deprecate it.
// network diagnostics feature is disabled when clusterConf.NetworkDiagnostics.Mode is set to "Disabled" | ||
// or when clusterConf.NetworkDiagnostics is empty and the legacy operConf.DisableNetworkDiagnostics is true | ||
if clusterConf.NetworkDiagnostics.Mode == configv1.NetworkDiagnosticsDisabled || | ||
reflect.DeepEqual(clusterConf.NetworkDiagnostics, configv1.NetworkDiagnostics{}) && operConf.DisableNetworkDiagnostics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Mode is empty, where are you defaulting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the mode is empty and operConf.DisableNetworkDiagnostics
is not set then we render the diagnostics.
Empty is an allowed value in the API, from the API docs:
this means the user has no opinion and the platform is left
to choose reasonable defaults. These defaults are subject to change over time.
The current default is All.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold cancel
/lgtm |
Commands ran: go get github.com/openshift/api go mod tidy go mod vendor Signed-off-by: Patryk Diak <pdiak@redhat.com>
Co-authored-by: Benjamin Pickard <bpickard@redhat.com> Signed-off-by: Patryk Diak <pdiak@redhat.com>
Signed-off-by: Patryk Diak <pdiak@redhat.com>
…heck controller Signed-off-by: Patryk Diak <pdiak@redhat.com>
Introduce a way to exclude a resource from the status manger and use it for the network diagnostics daemonset and deployment. Signed-off-by: Patryk Diak <pdiak@redhat.com>
8fe5415
to
44c7f1d
Compare
Had to do a manual rebase because even though the automatic one was successful the code did not build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpickard22, kyrtapz, trozet 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 |
154c095
into
openshift:master
@kyrtapz: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
[ART PR BUILD NOTIFIER] This PR has been included in build cluster-network-operator-container-v4.17.0-202404231810.p0.g154c095.assembly.stream.el9 for distgit cluster-network-operator. |
Implements: openshift/enhancements#1585
Note: The API is currently in TechPreview, the plan is to enable it in GA in the current release.