-
Notifications
You must be signed in to change notification settings - Fork 232
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
CFE-888: Enable DNSNameResolver feature-gate #2131
Conversation
@chiragkyal: This pull request references CFE-888 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.15.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 kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
/retest |
1 similar comment
/retest |
/retest |
@chiragkyal: This pull request references CFE-888 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.15.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 kubernetes/test-infra repository. |
/hold until CRD is available |
/hold until ovn-org/ovn-kubernetes#4045 is meged downstream. |
/assign @npinaeva |
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.
generally looks good, we just need to wait for ovn-k PR to be merged to finalize it
verbs: | ||
- create | ||
- delete | ||
- get | ||
- list | ||
- patch | ||
- update | ||
- watch |
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'll need to split these rights between node and control pane once we finalize the ovn-k implementation. It is likely to be node can only read, and control-plane can create/update/delete
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.
Sure, for now, I've dropped create, delete, patch and update permissions from node. Let me know if it needs further refinement.
pkg/network/ovn_kubernetes_test.go
Outdated
@@ -189,7 +199,7 @@ func TestRenderedOVNKubernetesConfig(t *testing.T) { | |||
disableGRO bool | |||
disableMultiNet bool | |||
enableMultiNetPolicies bool | |||
enableAdminNetPolicies bool | |||
featureGates configv1.CustomFeatureGates |
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.
I am thinking, maybe to make less changes in the future, we could replace this parameter with enableFeatureGates []FeatureGateName
and then build a real config by iterating over all known feature gates and adding the ones that are listed to Enabled
and others to Disabled
. In that case you don't need to change tests that don't enable any feature gates, and only make changes when a new feature gate should be enabled. What do you think?
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.
Agreed, and that's a good suggestion. I've updated the structure, hope it looks cleaner now.
resources: | ||
- dnsnameresolvers | ||
verbs: | ||
- create |
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.
Does the control-plane really require all of these verbs?
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.
I think so, because cluster manager creates and deletes these objects (similar to cloudprivateipconfigs I suppose)
@@ -565,6 +565,11 @@ data: | |||
admin_network_policy_enabled_flag="--enable-admin-network-policy" | |||
fi | |||
|
|||
dns_name_resolver_enabled_flag= |
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.
start-ovnkube-node
is only used by ovnkube-node, this means that the ovnkube-cluster-manager
won't rollout if DNS_NAME_RESOLVER_ENABLE
changes.
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.
I think it will, as it uses 004-config.yaml
file for features setup
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.
Lest consider a scenario in which the featuregate is enabled during cluster runtime(e.g cluster is moved to techpreview):
- FeatureGate gets enabled
- CNO restarts
- CNO renders the new 008-script-lib.yaml and 004-config.yaml confimaps
- ovnkube-node daemonset is rolled out because it is annotated with the has of the script:
cluster-network-operator/pkg/network/ovn_kubernetes.go
Lines 356 to 378 in 6aa2f0f
// Many ovnkube config options are stored in ConfigMaps; the ovnkube // daemonsets need to know when those ConfigMaps change so they can // restart with the new options. Render those ConfigMaps first and // embed a hash of their data into the ovnkube-node daemonsets. h := sha1.New() for _, path := range cmPaths { manifests, err := render.RenderTemplate(path, &data) if err != nil { return nil, progressing, errors.Wrapf(err, "failed to render ConfigMap template %q", path) } // Hash each rendered ConfigMap object's data for _, m := range manifests { bytes, err := json.Marshal(m) if err != nil { return nil, progressing, errors.Wrapf(err, "failed to marshal ConfigMap %q manifest", path) } if _, err := h.Write(bytes); err != nil { return nil, progressing, errors.Wrapf(err, "failed to hash ConfigMap %q data", path) } } } data.Data["OVNKubeConfigHash"] = hex.EncodeToString(h.Sum(nil)) - ovnkube-controller is not rolled out and it doesn't reload the configmap dynamically on its own.
does this make sense?
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 agreed it is a bigger problem and created a card for it https://issues.redhat.com/browse/SDN-4832
{{- if .DNS_NAME_RESOLVER_ENABLE }} | ||
- apiGroups: ["network.openshift.io"] | ||
resources: | ||
- dnsnameresolvers |
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.
Where is the dnsnameresolvers
CRD applied?
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.
The CRD will be created when along with the cluster-dns-operator: https://github.com/openshift/cluster-dns-operator/pull/394/files#diff-d5348dedaad45595f0bd40f1b4730932489203aa511b1432951f2bfad8ab8a79
@@ -819,7 +826,50 @@ logfile-maxage=0`, | |||
controlPlaneReplicaCount: 2, | |||
disableMultiNet: true, | |||
enableMultiNetPolicies: true, | |||
enableAdminNetPolicies: true, | |||
enabledFeatureGates: []configv1.FeatureGateName{configv1.FeatureGateAdminNetworkPolicy}, |
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.
configv1.FeatureGateDNSNameResolver
is not set in the featuregates here
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.
I think this test is specific to ANP. The enableAdminNetPolicies
field is changed to enabledFeatureGates
for using the same field for different feature gates in different test cases. The test case for DNSNameResolver
is added below.
/retest-required |
when DNSNameResolver feature-gate is enabled Signed-off-by: chiragkyal <ckyal@redhat.com>
/retest-required |
All other dev PRs are merged /hold cancel |
/retest-required |
manual CI-bot report
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chiragkyal, kyrtapz, npinaeva 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 |
/acknowledge-critical-fixes-only |
/label acknowledge-critical-fixes-only |
/retest-required |
@chiragkyal: 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-sigs/prow repository. I understand the commands that are listed here. |
build failed :( |
019fa77
into
openshift:master
[ART PR BUILD NOTIFIER] This PR has been included in build cluster-network-operator-container-v4.16.0-202405161711.p0.g019fa77.assembly.stream.el9 for distgit cluster-network-operator. |
Enable
DNSNameResolver
feature-gate for ovn-kubernetesRelated to EP : openshift/enhancements#1335