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

Add redhat_ocp_virt platform to sap_hypervisor_node_preconfigure #468

Merged
merged 45 commits into from
Dec 11, 2023
Merged

Add redhat_ocp_virt platform to sap_hypervisor_node_preconfigure #468

merged 45 commits into from
Dec 11, 2023

Conversation

newkit
Copy link
Contributor

@newkit newkit commented Sep 22, 2023

This PR adds functionality to configure an OCP cluster for SAP workloads.

@berndfinger berndfinger changed the title added redhat_ocp_virt platform to sap_hypervisor_node_preconfigure Add redhat_ocp_virt platform to sap_hypervisor_node_preconfigure Nov 16, 2023
@sean-freeman
Copy link
Member

@newkit @0xFelix Is this PR complete? If not then please click convert to draft which is listed in the right sidebar under 'Reviewers'.

@Klaas-
Copy link

Klaas- commented Nov 23, 2023

OCP is unsupported by SAP, right? We still had some hope for the certification to happen but I was told last week it's off the table and will never be discussed again (by SAP).

@newkit
Copy link
Contributor Author

newkit commented Nov 23, 2023

@newkit @0xFelix Is this PR complete? If not then please click convert to draft which is listed in the right sidebar under 'Reviewers'.

I'm done with the OCP part which I am focusing on ATM. Will address the RHV comments later.

@newkit
Copy link
Contributor Author

newkit commented Nov 23, 2023

OCP is unsupported by SAP, right? We still had some hope for the certification to happen but I was told last week it's off the table and will never be discussed again (by SAP).

Well, I'd say never say never, but you are right, ATM it does not look like there will be a validation/support from SAP for customers to run this on-prem. Still we hope to enable and encourage ppl to use this for testing and developing purposes.

@sean-freeman
Copy link
Member

@newkit please acknowledge this PR is ready for final review and merge; there have been too many commits appended since the PR was created and we need confirmation before merge.

@Klaas- draw your own conclusion from the following personal points of view.... in Android upstream source code new device codenames are always visible and reported on news websites + any support circumstance can be negotiated if a software vendor wants to make a deal with a customer.

@sean-freeman
Copy link
Member

@newkit please acknowledge this PR is ready for final review and merge; there have been too many commits appended since the PR was created and we need confirmation before merge.

I still see 51 review comments ('Resolve Conversation' buttons) that have not be marked as resolved.

Copy link
Contributor Author

@newkit newkit left a comment

Choose a reason for hiding this comment

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

Review done, not all proposals could be addressed now but are in backlog, e.g. remove static waits.

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@sean-freeman sean-freeman merged commit 0aa151d into sap-linuxlab:dev Dec 11, 2023
2 checks passed
Copy link
Contributor

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Added some comments. IMO not all previous comments have been addressed but unfortunately this was merged already. Can you please create issues to track the follow up work?

Comment on lines +7 to +27
#- name: Label the node with cpumanager=true
# ansible.builtin.k8s:
# definition:
# apiVersion: v1
# kind: Node
# metadata:
# name: "{{ __sap_hypervisor_node_preconfigure_register_worker.name }}"
# labels:
# cpumanager: true
# state: present
#
#- name: Label the node with invtsc=true
# ansible.builtin.k8s:
# definition:
# apiVersion: v1
# kind: Node
# metadata:
# name: "{{ __sap_hypervisor_node_preconfigure_register_worker.name }}"
# labels:
# invtsc: true
# state: present
Copy link
Contributor

Choose a reason for hiding this comment

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

These tasks are commented out.

Comment on lines +72 to +81
- name: Render template
ansible.builtin.template:
src: 99-kargs-worker.yml.j2
dest: "{{ __sap_hypervisor_node_preconfigure_register_tmpdir.path }}/99-kargs-worker.yml"
mode: "0644"

- name: Enable hugepages
kubernetes.core.k8s:
state: present
src: "{{ __sap_hypervisor_node_preconfigure_register_tmpdir.path }}/99-kargs-worker.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use lookup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants