Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ovn-controller] Don't create ovn-controller ds if no NicMappings set #337

Closed
wants to merge 1 commit into from
Closed

[ovn-controller] Don't create ovn-controller ds if no NicMappings set #337

wants to merge 1 commit into from
+671 −201

Conversation

averdagu
Copy link
Contributor

@averdagu averdagu commented Aug 6, 2024 •
edited
Loading

Currently there's no way to disable ovn-controller/ovn-controller-ovs on oc nodes (workarround is to set nodeSelector).
Since if there's no nicMappings set there's no use for ovn-controllers pods on oc nodes, this commit will set this behavior.

Resolves: OSPRH-7463

Things to do:

  • Cleanup code
  • Expose getDaemonSetWithName on lib-commons (PR)
  • Create new functional test to ensure if nicMappings are removed, daemonsets and configmaps are deleted
  • Fix kuttl
  • Delete NAD
  • Rebase

@openshift-ci openshift-ci bot requested review from karelyatin and lewisdenny August 6, 2024 15:47
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 6, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: averdagu
Once this PR has been reviewed and has the lgtm label, please assign stuggi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch from 654c295 to 146cb76 Compare August 7, 2024 08:28
@averdagu averdagu marked this pull request as draft August 7, 2024 09:02
@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 2 times, most recently from 356465e to 2c35719 Compare August 7, 2024 16:20
@averdagu averdagu marked this pull request as ready for review August 8, 2024 07:31
@openshift-ci openshift-ci bot requested review from dprince and olliewalsh August 8, 2024 07:31
@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 9 times, most recently from 76a3b67 to ef131e8 Compare August 9, 2024 12:55
@averdagu averdagu marked this pull request as draft August 9, 2024 12:55
@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 6 times, most recently from 3373548 to 9005e1e Compare August 9, 2024 15:38
averdagu added a commit to averdagu/lib-common that referenced this pull request Aug 9, 2024
@averdagu
[daemonset] Make get method public
3d6c472 
Needed-by: openstack-k8s-operators/ovn-operator#337
Related: OSPRH-7463
@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch from 2d7852f to 35d94bb Compare August 18, 2024 15:59
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/061a329e176641b2a3bf3c6810ac62c7

❌ openstack-k8s-operators-content-provider FAILURE in 7m 28s
⚠️ ovn-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch from 35d94bb to 484eb3b Compare August 18, 2024 18:00
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/2b201907b5cd47c18fdf17e5cdea1193

❌ openstack-k8s-operators-content-provider FAILURE in 7m 48s
⚠️ ovn-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 3 times, most recently from 1e47d79 to 2e8d704 Compare August 20, 2024 16:25
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/167f03c966a74b1298eedf75972dacee

✔️ openstack-k8s-operators-content-provider SUCCESS in 49m 39s
❌ ovn-operator-tempest-multinode FAILURE in 24m 12s

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 2 times, most recently from a83afe9 to 3ddb099 Compare August 22, 2024 19:38
karelyatin
karelyatin reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@karelyatin karelyatin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we really need all these cleanup logic considering the user interface is on openstackcontrolplane CR and we could just handle it there[1] as when we don't have nicMappings configured we don't need even OvnController CR and that should take care of cleaning up all child resources(daemonsets/cms etc).

https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310

@averdagu
Copy link
Contributor Author

/test ovn-operator-build-deploy-kuttl

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch 3 times, most recently from 0b0d702 to 8fbbe89 Compare August 27, 2024 13:28
@averdagu
Copy link
Contributor Author

/retest

@averdagu averdagu force-pushed the fix/OSPRH-7463-NicMappings branch from 8fbbe89 to a9ead03 Compare August 28, 2024 12:41
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

@averdagu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ovn-operator-build-deploy-kuttl a9ead03 link true /test ovn-operator-build-deploy-kuttl

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.

@slawqo
Copy link
Contributor

slawqo commented Sep 4, 2024

Wondering if we really need all these cleanup logic considering the user interface is on openstackcontrolplane CR and we could just handle it there[1] as when we don't have nicMappings configured we don't need even OvnController CR and that should take care of cleaning up all child resources(daemonsets/cms etc).

https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310

But instance.Spec.Ovn.Enabled may still be true as we need ovn dbs and northd to be deployed, right? We don't have separate knob to do this. Are you saying that we should simpy do in https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310 somehting like:

if !instance.Spec.Ovn.Enabled || len(instance.Spec.Ovn.NicMappings) == 0

? And that would do all the cleanup for us?

slawqo
slawqo reviewed Sep 4, 2024
View reviewed changes
controllers/ovncontroller_controller.go
return dset, nil
}

func (r *OVNControllerReconciler) ensureCMDeleted(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitty nit: just for readability IMHO this could be named "ensureConfigMapDeleted" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up ensuring the deletion on the ovn-controller I'll modify this func name.

controllers/ovncontroller_controller.go
return nil
}

func ensureDSDeleted(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar nit here too

@karelyatin
Copy link
Contributor

Wondering if we really need all these cleanup logic considering the user interface is on openstackcontrolplane CR and we could just handle it there[1] as when we don't have nicMappings configured we don't need even OvnController CR and that should take care of cleaning up all child resources(daemonsets/cms etc).
https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310

But instance.Spec.Ovn.Enabled may still be true as we need ovn dbs and northd to be deployed, right? We don't have separate knob to do this. Are you saying that we should simpy do in https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310 somehting like:

if !instance.Spec.Ovn.Enabled || len(instance.Spec.Ovn.NicMappings) == 0

? And that would do all the cleanup for us?

@slawqo yes that what i mean

stuggi pushed a commit to stuggi/lib-common that referenced this pull request Sep 4, 2024
@averdagu @stuggi
[daemonset] Make get method public
7b4b555 
Needed-by: openstack-k8s-operators/ovn-operator#337
Related: OSPRH-7463
(cherry picked from commit 321e10b)
@slawqo
Copy link
Contributor

slawqo commented Sep 4, 2024

Wondering if we really need all these cleanup logic considering the user interface is on openstackcontrolplane CR and we could just handle it there[1] as when we don't have nicMappings configured we don't need even OvnController CR and that should take care of cleaning up all child resources(daemonsets/cms etc).
https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310

But instance.Spec.Ovn.Enabled may still be true as we need ovn dbs and northd to be deployed, right? We don't have separate knob to do this. Are you saying that we should simpy do in https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/ovn.go#L310 somehting like:
if !instance.Spec.Ovn.Enabled || len(instance.Spec.Ovn.NicMappings) == 0
? And that would do all the cleanup for us?

@slawqo yes that what i mean

Sounds like a good idea for me and would make this patch much smaller probably :)

stuggi pushed a commit to stuggi/lib-common that referenced this pull request Sep 6, 2024
@averdagu @stuggi
[daemonset] Make get method public
e5bbad6 
Needed-by: openstack-k8s-operators/ovn-operator#337
Related: OSPRH-7463
(cherry picked from commit 321e10b)
@booxter booxter marked this pull request as draft September 13, 2024 16:32
@booxter
Copy link
Contributor

booxter commented Sep 13, 2024

Switched it to draft since I believe you will instead try to merge this in openstack-operator: openstack-k8s-operators/openstack-operator#1070

@averdagu
Copy link
Contributor Author

averdagu commented Dec 4, 2024

This was handled by the openstack-operator

@averdagu averdagu closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karelyatin karelyatin karelyatin left review comments

@slawqo slawqo slawqo left review comments

@lewisdenny lewisdenny Awaiting requested review from lewisdenny

@dprince dprince Awaiting requested review from dprince

@olliewalsh olliewalsh Awaiting requested review from olliewalsh

@abays abays Awaiting requested review from abays

@beagles beagles Awaiting requested review from beagles

Assignees
No one assigned
Labels
do-not-merge/work-in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@averdagu @slawqo @karelyatin @booxter