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

scripts: Add Additional Kustomization For Baremetal Deployment #494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vkhitrin
Copy link
Contributor

@vkhitrin vkhitrin commented Aug 26, 2023

Adding an option to inject additional EDPM variables for baremetal
deployments using Kustomize.
The following variables were added:

  • EDPM_BAREMETAL_KERNEL_ARGS, uses edpm-kernel role to modify kernel
    arugments
  • EDPM_BAREMETAL_KERNEL_HUGEPAGES, uses edpm-kernel role to
    configure hugepages
  • EDPM_BAREMETAL_TUNED_PROFILE, uses edpm_tuned
    role to configure a tuned profile
  • EDPM_BAREMETAL_TUNED_ISOLATED_CORES, uses edpm_tuned role to
    configured tuned isolated cores
  • EDPM_BAREMETAL_NETWORK_CONFIG_OVERRIDE, uses edpm_network_config
    role to allow providing custom network configuration

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vkhitrin
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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2023

Hi @vkhitrin. Thanks for your PR.

I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@vkhitrin
Copy link
Contributor Author

This might not be the ideal place to store these parameters. They may also be worthwhile to have in virtualized deployments.

In general, is there any mechanism that allows passing additional variables to EPDM?

Adding an option to inject additional EDPM variables for baremetal
deployments using Kustomize.
The following variables were added:
  - EDPM_BAREMETAL_KERNEL_ARGS, uses `edpm-kernel` role to modify kernel
    arugments
  - EDPM_BAREMETAL_KERNEL_HUGEPAGES, uses `edpm-kernel` role to
    configure hugepages
  - EDPM_BAREMETAL_TUNED_PROFILE, uses `edpm_tuned`
    role to configure a tuned profile
  - EDPM_BAREMETAL_TUNED_ISOLATED_CORES, uses `edpm_tuned` role to
    configured tuned isolated cores
  - EDPM_BAREMETAL_NETWORK_CONFIG_OVERRIDE, uses `edpm_network_config`
    role to allow providing custom network configuration
@fao89
Copy link
Contributor

fao89 commented Aug 29, 2023

/ok-to-test

@vkhitrin
Copy link
Contributor Author

@rabi, @fao89, when working on a baremetal NFV deployment, I injected even more variables not mentioned in this PR.
Perhaps I should refactor this to allow generic input that will populate variables instead of hard-codding Kustomizations.

@rabi
Copy link
Contributor

rabi commented Aug 30, 2023

@rabi, @fao89, when working on a baremetal NFV deployment, I injected even more variables not mentioned in this PR. Perhaps I should refactor this to allow generic input that will populate variables instead of hard-codding Kustomizations.

IMO, we should not add all these to install_yamls. All NFV kustomizations for downstream testing should preferably be in a downstream repo. If we still want to keep these in install_yamls, they should be in separate kustomization scripts.

@vkhitrin
Copy link
Contributor Author

@rabi, what about a requirement of allowing to Kustomize further unrelated to NFV use cases?
I think having a mechanism where we would be able to provide further Kustomizations via install_yamls is beneficial.

Also, on the scope of this project, as part of the ci-framework that it intended to be used, which wraps around this project, having to maintain internal repositories will cause our squad to deviate, which was already a pain point in TripleO deployments.

@Jaganathancse
Copy link

@rabi, what about a requirement of allowing to Kustomize further unrelated to NFV use cases? I think having a mechanism where we would be able to provide further Kustomizations via install_yamls is beneficial.

Also, on the scope of this project, as part of the ci-framework that it intended to be used, which wraps around this project, having to maintain internal repositories will cause our squad to deviate, which was already a pain point in TripleO deployments.

@vkhitrin As Rabi mentioned, no need to do the changes in install_yaml. we can have custom OpenstackDataplane CR for NFV use cases and also we can have specific kustomization for our requirements in downstream repo's.
I am also working on OpenstackDataplane Samples for NFV feature roles. will discuss and finalize.

@rabi
Copy link
Contributor

rabi commented Aug 30, 2023

I think having a mechanism where we would be able to provide further Kustomizations via install_yamls is beneficial.

We also have to keep them maintainable and not make them messy with tons of vars, I don't mind if we've kuystomization scripts for specific scenarios maintained in install_yamls and then a var to pick one for a scenario.

@vkhitrin
Copy link
Contributor Author

vkhitrin commented Sep 4, 2023

My question is, would it be reasonable to drop the content of this patch and rewrite it in a way where we will use only a single generic variable that will contain a JSON that will contain all extra Ansible variables for EDPM.

I guess that could be better. But can probably result in duplication of vars and confusion.

@vkhitrin
Copy link
Contributor Author

vkhitrin commented Sep 5, 2023

@ rabi, can you share a recommendation on proceeding with my last comment?
From how I see things,

  1. We will use ci-framework to invoke install_yamls.
  2. We will want to use install_yamls natively without maintaining a fork.
  3. We must provide custom Kustomizations (mainly Ansible variables) to configure various hardware and topologies.

Is the way forward will be through ci-framework https://ci-framework.readthedocs.io/en/latest/roles/edpm_kustomize.html?

@rabi
Copy link
Contributor

rabi commented Sep 5, 2023

Is the way forward will be through ci-framework

Probably yes. Looks like there is some work happening to leverage multiple kustomizations vi ci-framework. The following[1][2] could be relevant, though the kustomizations or the scripts generating them can still live in install_yamls.

[1] https://issues.redhat.com/browse/OSP-28087
[2] openstack-k8s-operators/ci-framework#511

@pablintino
Copy link
Contributor

Is the way forward will be through ci-framework

Probably yes. Looks like there is some work happening to leverage multiple kustomizations vi ci-framework. The following[1][2] could be relevant, though the kustomizations or the scripts generating them can still live in install_yamls.

[1] https://issues.redhat.com/browse/OSP-28087 [2] openstack-k8s-operators/ci-framework#511

@vkhitrin @rabi I've just spotted this PR. Correct, we are working on providing support for multiple kustomizations in ci-framework, and yes, it'll be perfectly fine if install_yamls generates them in the CR dir, we will pick them UP.

Expect that feature to be in in the early days of next week.

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

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.

6 participants