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

e2e: refactor NRO deployment #1071

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Nov 13, 2024

At the beginning of the u/s tests we're deploying the NRO/Scheduler objects.
This PR introduce a refactor of the deployment code, so each k8s flavor will be able to implement it for its need.

At the moment we're having 3 flavors:

  • Kuberenetes (vanilla)
  • OpenShift
  • HyperShift

In this PR the HyperShift logic is not implemented because there're missing details from the CI perspective such as the KUBECONFIG of the MNG cluster. we'll implement the missing code in future PR.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2024
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

partial review. I like the general direction, some initial comments inside

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 13, 2024

partial review. I like the general direction, some initial comments inside

Yea, I used the copied the code verbatim, refactoring the actual logic for k8s and ocp wasn't part of the plan, but hey why not

@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch from 441712b to b54e389 Compare November 13, 2024 12:11
@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch 3 times, most recently from 621270d to eafabf4 Compare November 17, 2024 15:58
@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch 2 times, most recently from 9563ef1 to 9cb9a52 Compare November 19, 2024 18:23
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

With this PR in place, HyperShift CI should be happy. if it's not, it's an actual bug in tests/functionality that we need to fix.

@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch from 9cb9a52 to 70e7a8a Compare November 19, 2024 18:27
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

With this PR in place, HyperShift CI should be happy. if it's not, it's an actual bug in tests/functionality that we need to fix.

On second thought we need #1027 first to make CI happy

@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch 3 times, most recently from 0a028dc to 8e76a20 Compare November 19, 2024 18:32
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 20, 2024

/test ci/prow/ci-e2e-install-hypershift

Copy link
Contributor

openshift-ci bot commented Nov 20, 2024

@Tal-or: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test ci-e2e
  • /test ci-e2e-install-hypershift
  • /test ci-index-tested-numaresources-operator-bundle
  • /test ci-install-e2e
  • /test ci-unit
  • /test images

The following commands are available to trigger optional jobs:

  • /test security

Use /test all to run all jobs.

In response to this:

/test ci/prow/ci-e2e-install-hypershift

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.

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 20, 2024

/test ci-e2e-install-hypershift

Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@ffromani
Copy link
Member

/lgtm cancel

issue found offline

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch from 8e76a20 to 28b87f1 Compare November 20, 2024 09:18
@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch 6 times, most recently from 3843f0d to fcbfd07 Compare November 20, 2024 13:43
New API for deploying NROP CRs that will be implemented by different k8s flavor/platforms

Signed-off-by: Talor Itzhak <[email protected]>
This is the main flow which implements the deployer
interface for OpenShift platform.

Signed-off-by: Talor Itzhak <[email protected]>
This is the main flow which implements the deployer
interface for Kubernetes platform.

It mostly based on the OpenShift implementation.

Signed-off-by: Talor Itzhak <[email protected]>
Creating a placeholder for implementing the deployer interface
for HyperShift.

The reason we can't implement it for now is because
we need to figure out where is the KUBECONFIG file
for the HyperShift MNG cluster is located and providing it
to the test.

This is needed because we need to create a KubeletConfig ConfigMap
against the MNG cluster and add it under the NodePool spec.

Signed-off-by: Talor Itzhak <[email protected]>
Those function are exposed by the `Deployer` interface from now
and are implemented differently for each platform.

The user should call `NewForPlatform` to get
the correct deployer flavor

Signed-off-by: Talor Itzhak <[email protected]>
Besides standard renaming, the interface doesn't expose the
NRO object anymore, so the test should retrieve it from the API server
directly.

This is done only for the tests so we prefer maintainability and readablity
at the cost of some several API calls.

Signed-off-by: Talor Itzhak <[email protected]>
Add context to the Deployer's functions and pass it
from the caller.

Signed-off-by: Talor Itzhak <[email protected]>
Some cosmetic changes to make the ginkgo tests more readable

Signed-off-by: Talor Itzhak <[email protected]>
Implement the deployer interface for HyperShift platform.

Signed-off-by: Talor Itzhak <[email protected]>
So far we were relaying on an env variable to be set
in order to determine whether we're running on hypershift.

There is a clever way to detect the platform for "free"
(without having the need to set any env variable)
so lets use it instead.

Signed-off-by: Talor Itzhak <[email protected]>
NRO test object always sets the MCP selector.
This is not the case for hypershift, so let's make
the label selector optional.

Signed-off-by: Talor Itzhak <[email protected]>
In hypershift case if withSELinuxPolicy set to `true`
then the manifests will contain a `MachineConfig` object
which NROP while running on HyperShift doesn't install.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or Tal-or force-pushed the refactor_e2e_deployment branch from fcbfd07 to 7de8e4b Compare November 20, 2024 15:53
Copy link
Contributor

openshift-ci bot commented Nov 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Tal-or

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

@ffromani
Copy link
Member

/lgtm

silly me missed that change previously :\

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 20, 2024

/lgtm

silly me missed that change previously :\

I would wait with that but let's see

@openshift-merge-bot openshift-merge-bot bot merged commit 86a59b8 into openshift-kni:main Nov 20, 2024
15 checks passed
@Tal-or Tal-or deleted the refactor_e2e_deployment branch November 20, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants