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.

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

Fixes config vars in nova.conf #823

Merged

Conversation

auniyal61
Copy link
Contributor

@auniyal61 auniyal61 commented Jul 16, 2024

  • moves allow_resize_to_same_host under nvoa-api
  • moves [workarounds].disable_fallback_pcpu_query=true under scheduler only and rest under nova-compute
  • moves [libvirt] conf under libvirt.LibvirtDriver driver

Closes #144

@openshift-ci openshift-ci bot requested review from olliewalsh and viroel July 16, 2024 08:37
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://review.rdoproject.org/zuul/buildset/88f265c45dda4df79363394cf502c286

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 08m 51s
✔️ nova-operator-kuttl SUCCESS in 41m 36s
nova-operator-tempest-multinode FAILURE in 2h 03m 01s
nova-operator-tempest-multinode-ceph FAILURE in 2h 34m 06s

@auniyal61
Copy link
Contributor Author

recheck unrelated keystone 500
tempest.lib.exceptions.IdentityError: Got identity error
Details: Unexpected status code 500

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://review.rdoproject.org/zuul/buildset/051746defb7d4b469fd3400ac85b9510

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 54m 24s
✔️ nova-operator-kuttl SUCCESS in 39m 02s
✔️ nova-operator-tempest-multinode SUCCESS in 2h 11m 09s
nova-operator-tempest-multinode-ceph FAILURE in 1h 35m 25s

@auniyal61
Copy link
Contributor Author

recheck unrelated fail, network create cmd failed with error:

openstack network create --external --default --provider-network-type flat --provider-physical-network datacentre --no-share public

Error while executing command: HttpException: 502, The server returned an invalid or incomplete response.: 502 Bad Gateway

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://review.rdoproject.org/zuul/buildset/29baff62dd7743c39141dd15fc44bf24

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 10m 32s
✔️ nova-operator-kuttl SUCCESS in 41m 08s
nova-operator-tempest-multinode FAILURE in 1h 39m 22s
nova-operator-tempest-multinode-ceph FAILURE in 1h 21m 17s

templates/nova.conf Show resolved Hide resolved
state_path = /var/lib/nova
instance_name_template = instance-%08x
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the default value and i dont think we should be setting it here explicitly
its also used on the nova-api i think in some cases.
inanycase we should just remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

templates/nova.conf Show resolved Hide resolved
@SeanMooney SeanMooney dismissed their stale review July 17, 2024 12:04

done, lest see how this goes with end ot end ci over all i think im happy with the current state

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://review.rdoproject.org/zuul/buildset/ba710462a36e438f826e5dd3a84f6ecc

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 45m 57s
✔️ nova-operator-kuttl SUCCESS in 42m 15s
nova-operator-tempest-multinode FAILURE in 2h 11m 36s
nova-operator-tempest-multinode-ceph FAILURE in 1h 43m 24s

@auniyal61
Copy link
Contributor Author

recheck unrealted

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://review.rdoproject.org/zuul/buildset/a99dac01908644a69000c018424dbbdc

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 00m 57s
✔️ nova-operator-kuttl SUCCESS in 39m 39s
nova-operator-tempest-multinode FAILURE in 1h 40m 36s
nova-operator-tempest-multinode-ceph FAILURE in 1h 41m 39s

templates/nova.conf Show resolved Hide resolved
templates/nova.conf Show resolved Hide resolved
templates/nova.conf Show resolved Hide resolved
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://review.rdoproject.org/zuul/buildset/7c720c20b925474db8f081ed1b2819e4

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 36m 14s
✔️ nova-operator-kuttl SUCCESS in 42m 01s
nova-operator-tempest-multinode FAILURE in 1h 41m 13s
✔️ nova-operator-tempest-multinode-ceph SUCCESS in 2h 35m 39s

@auniyal61
Copy link
Contributor Author

recheck
tempest.lib.exceptions.IdentityError: Got identity error

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://review.rdoproject.org/zuul/buildset/4024c441027e479c8ecd6dfcda2b4894

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 31m 06s
✔️ nova-operator-kuttl SUCCESS in 42m 36s
nova-operator-tempest-multinode FAILURE in 1h 37m 44s
✔️ nova-operator-tempest-multinode-ceph SUCCESS in 2h 32m 04s

@auniyal61
Copy link
Contributor Author

recheck
tempest.lib.exceptions.IdentityError: Got identity error
Details: Unexpected status code 503

@gibizer
Copy link
Contributor

gibizer commented Jul 19, 2024

#826 shows that we generate [libvirt] config section for the ironic compute config. So that config can be cleaned up too.

@gibizer
Copy link
Contributor

gibizer commented Jul 19, 2024

Here is the edpm compute conf from the test run: https://review.rdoproject.org/zuul/build/852d057b604c431596707e27c941d80e/log/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/nova-cell1-compute-config.yaml-01-nova.conf

The content looks OK to me, but as a follow up we should improve the whitespacing of the templating as there are a lot of empty lines in the config file which looks strange.


I also checked the nova-api service config
https://review.rdoproject.org/zuul/build/852d057b604c431596707e27c941d80e/log/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/nova-api-config-data.yaml-01-nova.conf

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://review.rdoproject.org/zuul/buildset/4cdbd27d5da342e187e61a3d004a0ac4

✔️ openstack-k8s-operators-content-provider SUCCESS in 56m 44s
✔️ nova-operator-kuttl SUCCESS in 39m 09s
nova-operator-tempest-multinode FAILURE in 27m 58s
nova-operator-tempest-multinode-ceph FAILURE in 35m 14s

@SeanMooney
Copy link
Contributor

#826 shows that we generate [libvirt] config section for the ironic compute config. So that config can be cleaned up too.

by the way what is the motivation for this cleaning in general.
i intentionally didn't make these condition originally because you were orgainly worried about the complexity of the template.

we can make them conditional but I'm just wondering why the change of heart?

@gibizer
Copy link
Contributor

gibizer commented Jul 19, 2024

#826 shows that we generate [libvirt] config section for the ironic compute config. So that config can be cleaned up too.

by the way what is the motivation for this cleaning in general. i intentionally didn't make these condition originally because you were orgainly worried about the complexity of the template.

we can make them conditional but I'm just wondering why the change of heart?

good point. My goal is not to have misleading / unused config options in the service configs. If this leads to a complex template then I will probably propose to split the template to per service templates.

@SeanMooney
Copy link
Contributor

#826 shows that we generate [libvirt] config section for the ironic compute config. So that config can be cleaned up too.

by the way what is the motivation for this cleaning in general. i intentionally didn't make these condition originally because you were orgainly worried about the complexity of the template.
we can make them conditional but I'm just wondering why the change of heart?

good point. My goal is not to have misleading / unused config options in the service configs. If this leads to a complex template then I will probably propose to split the template to per service templates.

ok because i really don't want to see use split this into multiple templates.
i don't see the generation as misleading even fi that section is not used.

its why we are intentionally not filtering which service config (cinder, neutorn) are generated to only some services.

@gibizer
Copy link
Contributor

gibizer commented Jul 22, 2024

#826 shows that we generate [libvirt] config section for the ironic compute config. So that config can be cleaned up too.

by the way what is the motivation for this cleaning in general. i intentionally didn't make these condition originally because you were orgainly worried about the complexity of the template.
we can make them conditional but I'm just wondering why the change of heart?

good point. My goal is not to have misleading / unused config options in the service configs. If this leads to a complex template then I will probably propose to split the template to per service templates.

ok because i really don't want to see use split this into multiple templates. i don't see the generation as misleading even fi that section is not used.

its why we are intentionally not filtering which service config (cinder, neutorn) are generated to only some services.

The cinder / neutron sections are less likely to cause a misunderstanding as they not expected to be configured by the user much. However something like force_config_drive appearing in our nova-api config https://review.rdoproject.org/zuul/build/f777d3130e1446b5bef440d01dde82a4/log/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/nova-api-config-data.yaml-01-nova.conf#17 is more problematic in my eyes. If the user looks at our current config they might thing that it needs to be set in the nova-api config, as it is set there today.

@auniyal61
Copy link
Contributor Author

@gibizer

  • there are a lot of empty lines in the config file which looks strange.

fixed

  • disable_fallback_pcpu_query

moved to scheduler.
I could not find it in docs

  • max_concurrent_live_migrations

removed

@auniyal61
Copy link
Contributor Author

recheck
deployment failed while subnet creation
BadRequestException: 400: Client Error for url: https://neutron-public-openstack.apps-crc.testing/v2.0/subnets, Invalid input for operation: Requested subnet with cidr: 192.168.122.0/24 for network: 6efe365e-45fa-40ec-afdf-aeb639d08bba overlaps with another subnet.

TASK [os_net_setup : Create subnets _raw_params={{ lookup('ansible.builtin.template', _template_file) }}

it was using openstackclient for this, not sure, why it was creating a subnet it was not a tempest test.
may be some other deployment tests.

@auniyal61
Copy link
Contributor Author

  • there are a lot of empty lines in the config file which looks strange.

fixed

there are still lot of empty lines, because of how its rendered.
normaly in jinja2 templates we put condition inside {% %}and expression or statement inside {{ }}, but here everything is inside {{ }}, so right now its just printing "\S" (space) on every failed condition.

as of now, I tried to use {% %}, but its just printing the whole thing and not parsing it as condition, so no change there.

another way I tried for my tests is to refactor configData string to replace "\n\n\n+" with "\n", before writing to a final file, and it did the what we want, but this may not be a best way.

@SeanMooney
Copy link
Contributor

  • there are a lot of empty lines in the config file which looks strange.

fixed

there are still lot of empty lines, because of how its rendered. normaly in jinja2 templates we put condition inside {% %}and expression or statement inside {{ }}, but here everything is inside {{ }}, so right now its just printing "\S" (space) on every failed condition.

as of now, I tried to use {% %}, but its just printing the whole thing and not parsing it as condition, so no change there.

another way I tried for my tests is to refactor configData string to replace "\n\n\n+" with "\n", before writing to a final file, and it did the what we want, but this may not be a best way.

we are not using jinia templating this is golang templating.

there is a way to trim using - i.e.

{{- -}} i belive see https://pkg.go.dev/text/template#hdr-Text_and_spaces for details.

@SeanMooney
Copy link
Contributor

i woudl perfer to not include removing of extra white space in this pr

Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

I have one more nit to fix otherwise the generated config files looks good to me.
Also I'm OK to try to remove the excessive whitespaces from the generated config files in a separate commit.

templates/nova.conf Outdated Show resolved Hide resolved
@auniyal61
Copy link
Contributor Author

/retest
Nova reconfiguration [BeforeEach] reconfigures memcached service and check works locally

@auniyal61
Copy link
Contributor Author

/retest
Samples when nova_v1beta1_nova-compute-fake.yaml sample is applied passed locally

Ran 11 of 323 Specs in 8.195 seconds
SUCCESS! -- 11 Passed | 0 Failed | 0 Pending | 312 Skipped

@mrkisaolamb
Copy link
Contributor

I rebased patch to contain fix for unstable tests, this should stabilize reconfiguration tests like "Nova reconfiguration [BeforeEach] reconfigures memcached service and check"

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/95fe2dc54e4c48f8821a2dab5b9e2bf9

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 34m 36s
✔️ nova-operator-kuttl SUCCESS in 38m 20s
nova-operator-tempest-multinode FAILURE in 1h 45m 29s
nova-operator-tempest-multinode-ceph FAILURE in 1h 45m 50s

@auniyal61
Copy link
Contributor Author

/retest

@auniyal61
Copy link
Contributor Author

/retest
nova_v1beta1_nova-compute-fake.yaml sample test works locally

Copy link
Contributor

@mrkisaolamb mrkisaolamb left a comment

Choose a reason for hiding this comment

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

Looks like CI hiccups on timeouts in functionall test ended. I'm ok with this changes but @gibizer -1 so it will be nice to have his approve

Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

looks good thanks.

@openshift-ci openshift-ci bot added the lgtm label Aug 7, 2024
Copy link
Contributor

openshift-ci bot commented Aug 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: auniyal61, gibizer, mrkisaolamb

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:
  • OWNERS [gibizer,mrkisaolamb]

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

@auniyal61
Copy link
Contributor Author

/retest pod pending timeout, seems deployment issue
/logs/process-log.txt: no such file or directory

Copy link
Contributor

openshift-ci bot commented Aug 8, 2024

@auniyal61: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test functional
  • /test images
  • /test precommit-check

Use /test all to run all jobs.

In response to this:

/retest pod pending timeout, seems deployment issue
/logs/process-log.txt: no such file or directory

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.

@auniyal61
Copy link
Contributor Author

/test precommit-check
/test images

@openshift-merge-bot openshift-merge-bot bot merged commit 2789586 into openstack-k8s-operators:main Aug 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not render non nova-api specific config for nova-api server
4 participants