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

Consume Topology CR by reference #304

Merged

Conversation

fmount
Copy link
Contributor

@fmount fmount commented Feb 7, 2025

This patch provides more granular control over Pod placement and scheduling through the Topology CR integration introduced in [1]. In particular it provides:

  • a new API parameter (TopologyRef) defined for each Component that allows to reference an existing Topology CRs in the same namespace

  • the operator logic that retrieves and processes the referenced Topology CR through the functions provided by lib-common [2]

  • enhanced Deployment configuration that incorporates the processed Topology

  • kuttl test to verify a referenced topology has been applied to the Deployment and the resulting Pods

Note that webhooks are in place to prevent referencing a Topology from a different namespace (which is not a supported scenario).

Jira: https://issues.redhat.com/browse/OSPRH-13836

@fmount fmount requested a review from stuggi February 7, 2025 12:49
@openshift-ci openshift-ci bot requested review from dprince and lewisdenny February 7, 2025 12:49
@fmount fmount requested a review from olliewalsh February 7, 2025 12:50
@fmount fmount requested a review from lmiccini February 7, 2025 14:20
@lmiccini lmiccini requested a review from dciabrin February 7, 2025 14:23
)
if err != nil {
instance.Status.Conditions.Set(condition.FalseCondition(
condition.TopologyReadyCondition,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to reorder/amend the other Conditions checks in the existing unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to change any existing unit test because if .Spec.TopologyRef doesn't exist (which is the case of the tests already in place) the order is preserved. This provides fully backward compatibility and TopologyCondition is not part of the default conditions (see L455), but only added if the current reconciliation has this parameter in its spec.
We can improve the newly introduced topology kuttl test (when I see a working execution) to assert all the conditions including Topology.

@fmount fmount force-pushed the topologyref branch 2 times, most recently from 3bafdf2 to 8660efb Compare February 7, 2025 15:06
@fmount
Copy link
Contributor Author

fmount commented Feb 10, 2025

We need openstack-k8s-operators/install_yamls#1015 to make topology CRD available in kuttl.

@stuggi
Copy link
Contributor

stuggi commented Feb 10, 2025

/retest

@fmount
Copy link
Contributor Author

fmount commented Feb 10, 2025

Build step failed:

--> Creating resources with label build=openstack-operator ...
    error: the server is currently unable to handle the request
--> Failed

re-kicking kuttl.

@fmount
Copy link
Contributor Author

fmount commented Feb 10, 2025

/test mariadb-operator-build-deploy-kuttl

This patch provides more granular control over Pod placement and
scheduling through the Topology CR integration introduced in [1].
In particular it provides:

- a new API parameter (TopologyRef) defined for each Component that
  allows to reference an existing Topology CRs in the same namespace

- the operator logic that retrieves and processes the referenced
  Topology CR through the functions provided by lib-common [2]

- enhanced StatefulSet and Deployment configuration that incorporates
  the processed Topology

- kuttl test to verify a referenced topology has been applied to the
  StatefulSet and the resulting Pods

Note that webhooks are in place to prevent referencing a Topology from a
different namespace (which is not a supported scenario).

Signed-off-by: Francesco Pantano <[email protected]>
Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fmount, stuggi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit a4fdf76 into openstack-k8s-operators:main Feb 11, 2025
6 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.

3 participants